feat: add opt-in subtractTimerOverhead option#568
Conversation
Add a new BenchOptions.subtractTimerOverhead boolean (default false). When enabled, the cost of one timestamp provider call is calibrated once at construction time via the new exported calibrateTimerOverhead helper, and subtracted from each raw latency sample (clamped to zero) before the statistics are computed. The correction is applied post-loop, in Task#processRunResult, so the sampling loop's exit condition keeps using raw timings and converges naturally even when the function under test runs at the timer grain. Only location statistics (mean, percentiles) are corrected; variance and relative margin of error are unchanged since subtracting a constant does not reduce dispersion. The estimate is exposed as bench.timerOverhead (undefined when the option is off, 0 on coarse-timer runtimes).
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ab8fc7a18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (delta > 0) { | ||
| deltas.push(delta) |
There was a problem hiding this comment.
For quantized providers such as the documented Date.now() custom provider, most back-to-back calls return the same value, but any pair that straddles a clock tick yields a positive delta, typically 1 ms. Because zero deltas are discarded here, one tick during calibration makes the median positive delta 1 ms instead of the intended coarse-timer no-op, so subtractTimerOverhead can subtract a full millisecond from every sample and clamp sub-ms tasks to zero. The calibration should account for the zero samples / clock resolution rather than using only positive deltas.
Useful? React with 👍 / 👎.
| if (overhead !== undefined && overhead > 0) { | ||
| for (let i = 0; i < latencySamples.length; i++) { | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| latencySamples[i] = Math.max(0, latencySamples[i]! - overhead) |
There was a problem hiding this comment.
When subtractTimerOverhead is enabled, mutating latencySamples here also changes the samples later summed into TaskResult.totalTime and period, even though the run loop stopped using the uncorrected timings. For fast tasks where raw samples are close to the calibrated overhead, the benchmark can run until the raw total reaches bench.time while the reported totalTime is far below that limit (or even 0), which breaks the existing semantics that totalTime is the actual benchmark cycle time rather than just a corrected statistic.
Useful? React with 👍 / 👎.
|
Closed in favor of #571 which consolidates this feature with #569 and #570 into a single coherent change. The 8-agent cross-validated audit identified that naively composing the three PRs would have caused the diagnostics in #569 and #570 to operate on the corrected-and-clamped sample set, producing artificially small detected-resolution values and false-positive saturation warnings whenever overhead subtraction was active. The consolidated PR also folds in the fixes for the issues surfaced by the same audit: bigint-precision-preserving subtraction in calibration ( |
1 participant
Summary
Add a new
BenchOptions.subtractTimerOverheadboolean (defaultfalse). When enabled, the cost of one timestamp provider call is calibrated once at construction time via the new exportedcalibrateTimerOverheadhelper, and subtracted from each raw latency sample (clamped to zero) before statistics are computed.The estimate is exposed as
bench.timerOverhead(number | undefined).Implementation
calibrateTimerOverhead(provider, samples = 1024)returns the median of the strictly positive deltas of back-to-backnow()calls, or0on coarse-timer runtimes.Task#processRunResult, just beforesortSamples. The sampling loop's exit condition keeps using raw timings and converges naturally even when the function under test runs at the timer grain — applying the correction inside#measure/#measureSyncwould zero-out samples on sub-µs functions and cause the OR-loop'stotalTime < timecondition to never be met.overriddenDurationsamples are still collected via the same path and corrected uniformly; the correction is opt-in so a benchmark relying on overridden durations should leave it disabled.Caveats (documented in JSDoc)
Only location statistics (mean, percentiles) are corrected. Variance, sd, sem, moe and rme are unchanged because subtracting a constant does not reduce dispersion (
Var(X − C) = Var(X)for a constantC).bench.timerOverheadisundefinedwhen the option is off,0on runtimes whose timer resolution exceeds its own call cost.Risk
None for existing benchmarks — option defaults to
false, behavior strictly unchanged when omitted. No new runtime dependency. Bundle size impact is minimal (calibration helper + 5 LOC in the hot path).