Skip to content

Conversation

eguguchkin
Copy link
Contributor

@eguguchkin eguguchkin commented Sep 24, 2025

Description

Increase cache capacity to reduce thrashing and speed up long-running tests.


  • I have read and followed all requirements in CONTRIBUTING.md;
  • I used LLM/AI assistance to make this pull request;

Summary by CodeRabbit

  • Tests
    • Refined stress tests for the cache to better simulate realistic workloads and stability.
    • Introduced named constants for object count, get operations, and cache size.
    • Switched to deterministic key generation for repeatable results.
    • Increased mock value size to reflect larger payloads.
    • Added brief pauses between cleanup attempts to reduce tight-loop churn.
    • Simplified stat handling within tests to avoid reuse across iterations.

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.28%. Comparing base (8d4e256) to head (56efddc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   71.42%   71.28%   -0.14%     
==========================================
  Files         200      200              
  Lines       18143    18143              
==========================================
- Hits        12958    12933      -25     
- Misses       4464     4487      +23     
- Partials      721      723       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

coderabbitai bot commented Sep 24, 2025

📝 Walkthrough

Walkthrough

  • In cache/cache_test.go, the cleanup loop now creates a fresh CleanStat per iteration and passes it directly to Cleaner.Cleanup, removing reuse of a single stat.
  • A 10 microsecond delay was added between cleanup attempts.
  • TestStress now uses named constants (objCount=1000, getCount=100000, cacheSize=128 KiB).
  • Key generation in the stress test is deterministic within objCount instead of span-based.
  • The Get callback returns a larger value (32 elements instead of 8).
  • TestStress was updated to use the new constants and key generation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "perf(cache): reduce duration of long-running tests" succinctly and accurately summarizes the primary intent of the changes—performance adjustments to cache-related tests to shorten their runtime—and aligns with the PR objectives and the test-file edits (cache sizing, iteration tweaks, and timing). It is concise, specific, and readable, so a teammate scanning history can understand the main change at a glance.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
cache/cache_test.go (3)

67-69: Avoid per-iteration allocation; reuse CleanStat (and optionally use a ticker).

Allocating a new CleanStat each loop can create GC pressure in this hot path. Reuse a single instance and reset it.

Apply within the loop:

-            cleaner.Cleanup(&CleanStat{})
-            time.Sleep(10 * time.Microsecond)
+            stat = CleanStat{} // reset fields
+            cleaner.Cleanup(&stat)
+            time.Sleep(10 * time.Microsecond)

Additionally, declare the reusable variable before the loop:

var stat CleanStat
for !done.Load() {
    stat = CleanStat{}
    cleaner.Cleanup(&stat)
    time.Sleep(10 * time.Microsecond)
}

(Optional) Replace Sleep with a ticker to pace cleanups without a tight loop:

ticker := time.NewTicker(10 * time.Microsecond)
defer ticker.Stop()
for !done.Load() {
    <-ticker.C
    stat = CleanStat{}
    cleaner.Cleanup(&stat)
}

95-96: Use per-goroutine RNG and drop unused parameter.

The global math/rand adds lock contention under high concurrency, and the get callback no longer uses i.

  • Per-goroutine RNG sketch:
for g := 0; g < workers; g++ {
    seed := time.Now().UnixNano() + int64(g)
    go func(seed int64) {
        defer wgGet.Done()
        r := rand.New(rand.NewSource(seed))
        for i := 0; i < records; i++ {
            get(c, i) // or update signature as below
        }
    }(seed)
}
  • If i is no longer needed, simplify signatures:
-func testStress(size, workers, records int, get func(*Cache[[]uint64], int)) {
+func testStress(size, workers, records int, get func(*Cache[[]uint64])) {
...
-                get(c, i)
+                get(c)

And update the call site accordingly.


120-120: Confirm size accounting matches intent (32 vs 1×uint64).

You report 32 bytes but return a slice of length 1. If this is intentional to stress accounting without extra allocations, fine. If not, align payload and size.

Example alignment:

-            return []uint64{uint64(key)}, 32
+            b := make([]uint64, 4) // 4 * 8B = 32B
+            b[0] = uint64(key)
+            return b, 32
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d4e256 and 56efddc.

📒 Files selected for processing (1)
  • cache/cache_test.go (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cache/cache_test.go (2)
cache/cleaner.go (1)
  • CleanStat (20-29)
cache/cache.go (1)
  • Cache (60-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test
🔇 Additional comments (1)
cache/cache_test.go (1)

90-94: LGTM: constants and larger cache size improve stability and reduce thrash.

Clearer intent and better control over test parameters.

@eguguchkin eguguchkin modified the milestones: v0.61.2, v0.62.0 Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants