Perf-regression: cut sampler hot-path overhead (partial closure of 86aj0hfmc) - #31
Merged
Merged
Conversation
…6aj0hfmc) - src/snmalloc/profile/sampler.h: hoist the per-thread `sampler_reentered()` check from `Sampler::record_alloc` into `record_alloc_slow`. The hot countdown is now a single TLS decrement plus a signed compare; the reentrancy check only runs on the ~1-in-512-KiB fraction of allocations that already cost a slow-path transition. Sample weighting unchanged -- the `rate - hot_.bytes_until_sample + requested_size` formula already absorbs the overshoot when the counter ticks negative under re-entry. - src/snmalloc/profile/record.h: reorder `record_dealloc<Config>` so the cheap slab-metadata probe and atomic-slot peek run before the `ReentrancyGuard` is constructed. The common-case (object on a slab with no installed lazy backing, or slab installed but specific object never sampled) now skips the TLS store-store-load round-trip from the guard. - docs/heap-profiling-benchmarks.md: re-publish bench numbers after the fix. Idle ratios dropped from a max of 1.0493 to 1.0128 on this host, with two of three groups under 1.01. Documented the cross-run bimodal variance (20-80% on individual variants between back-to-back runs) that prevents this harness on this host from credibly resolving the remaining <3% gap on mixed/active. ClickUp: 86aj0hfmc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two hot-path simplifications in the SNMALLOC_PROFILE=ON path:
Sampler::record_alloc— hoistedsampler_reentered()check out of the fast path intorecord_alloc_slow. Saves one TLS load + one mispredictable branch per allocation. Slow path detects re-entry on entry and bails without resetting the countdown; sample weighting(rate - bytes_until_sample + requested_size)absorbs the overshoot.record_dealloc<Config>— reordered so the slab-metadata probe + atomic-slot peek run BEFOREReentrancyGuardconstruction. Common case (object never sampled) now skips the TLS store-store-load round-trip.Results
Status — PARTIAL closure
The literal acceptance band
<= 1.01on all 6 cells is met on 4 of 6.medium/idle(1.28%) andmixed/active(2.93%) remain over target. The harness variance is larger than the residual measurement gap (other back-to-back runs of the same code showed swings to 1.198, 1.252, 1.281 — bimodal under macOS scheduler).Closing the last <3% gap requires either:
taskset+cpufreq=performance, ORsample_size/measurement_timein the bench harness so it resolves sub-1% (harness change — out of scope for this PR)Both options flagged as follow-up in
docs/heap-profiling-benchmarks.mdand the ClickUp ticket.Closes the dominant gap on 86aj0hfmc (max overhead halved). The ticket stays open until Linux pinned re-run lands.
Test plan