Skip to content

fix(benchmark.js): collect garbage before synchronous measurements - #112

Closed
not-matthias wants to merge 1 commit into
mainfrom
cod-4028-use-predictable-gc-schedule-in-codspeed-node-for-memory
Closed

not-matthias wants to merge 1 commit into
mainfrom
cod-4028-use-predictable-gc-schedule-in-codspeed-node-for-memory

Conversation

@not-matthias

@not-matthias not-matthias commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

We do the same already in the other frameworks

Refs COD-4028

Match the asynchronous path by collecting warmup garbage before the start marker. Keep forced collection outside the measured window.

Refs COD-4028
@not-matthias
not-matthias marked this pull request as ready for review October 29, 2026 18:18
@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The change is limited to synchronous runs in the Benchmark.js plugin.

The PR appears safe to merge, with a non-blocking request for regression tests.

Fix All in Claude CodeFindings

  1. P2 GC ordering has no test ▶
Fix with agent prompt
### Issue 1
packages/benchmark.js-plugin/src/index.ts:210
The new collection has no regression test. Existing tests check that `startBenchmark` runs, but not whether `global.gc` runs before it. Removing this fix or moving collection into the measured window would still pass those tests.

Add a temporary `global.gc` mock and check that collection runs after warmup and before `startBenchmark`. Also cover missing GC and fallback execution, and restore the original global afterward.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Adds forced garbage collection before synchronous Benchmark.js measurement, matching the asynchronous path.

  • Synchronous Benchmark.js runs collect warmup garbage before instrumentation starts.

Reviews (1) · Last reviewed commit: "fix(benchmark.js): collect garbage befor..." · Reviewed by Greptile

} else {
optimizeFunctionSync(benchPayload);
await mongoMeasurement.start(uri);
global.gc?.();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 GC ordering has no test

The new collection has no regression test. Existing tests check that startBenchmark runs, but not whether global.gc runs before it. Removing this fix or moving collection into the measured window would still pass those tests.

Add a temporary global.gc mock and check that collection runs after warmup and before startBenchmark. Also cover missing GC and fallback execution, and restore the original global afterward.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/benchmark.js-plugin/src/index.ts
Line: 210

Comment:
**GC ordering has no test**

The new collection has no regression test. Existing tests check that `startBenchmark` runs, but not whether `global.gc` runs before it. Removing this fix or moving collection into the measured window would still pass those tests.

Add a temporary `global.gc` mock and check that collection runs after warmup and before `startBenchmark`. Also cover missing GC and fallback execution, and restore the original global afterward.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code Fix in Codex

@codspeed

codspeed Bot commented Oct 29, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 3.68%

3 warnings, 1 note

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ 14 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

ℹ️ 16 benchmarks have no flamegraph

The flamegraph wasn't generated: most of the time was spent in runtime internals, which would leave nothing useful to analyze.

See affected benchmarks

⚡ 9 improved benchmarks
❌ 8 regressed benchmarks
✅ 247 untouched benchmarks
⏩ 56 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ CPU Simulation test_recursive_fibo_10 339.8 µs 691.8 µs -50.88%
❌ CPU Simulation wait 500ms 29.5 ms 52 ms -43.22%
❌ CPU Simulation wait 1sec 59 ms 93.8 ms -37.06%
❌ CPU Simulation test_recursive_cached_fibo_10 49.3 µs 65.4 µs -24.71%
❌ CPU Simulation test_recursive_cached_fibo_20 64.3 µs 80.1 µs -19.72%
❌ CPU Simulation test_recursive_cached_fibo_30 72 µs 87.8 µs -17.94%
❌ Walltime switch 2 84 ns 96 ns -12.5%
❌ Walltime switch 2 108 ns 120 ns -10%
⚡ Memory one 2 KB 1.3 KB +48.24%
⚡ Memory count is back to zero 2.1 KB 1.5 KB +44.09%
⚡ Memory wait 1sec 28.7 KB 22.1 KB +29.49%
⚡ CPU Simulation wait 500ms 38.7 ms 30.1 ms +28.53%
⚡ Memory wait 1sec 27.5 KB 22 KB +24.96%
⚡ Walltime test sync baz 10 144 ns 120 ns +20%
⚡ CPU Simulation wait 1sec 72.9 ms 61.3 ms +19.02%
⚡ Walltime test sync baz 100 252 ns 228 ns +10.53%
⚡ Walltime test_iterative_fibo_10 132 ns 120 ns +10%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing cod-4028-use-predictable-gc-schedule-in-codspeed-node-for-memory (6925940) with main (727b808)

Open in CodSpeed

Footnotes

  1. 56 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

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.

1 participant