perf: remove two hot-path memory indirections (Callgrind rec_array lookup, allocator free-list scan) - #32
Conversation
Two dependent-load stalls sit on the paths every Callgrind run takes: one per basic block execution, one per arena allocation. CLG_(setup_bbcc): --separate-recs defaults to 2, so the separate_recursions > 1 block runs on every basic block execution. It ends with a lookup in rec_array, which lives in a separate allocation, plus an assertion that adds two more dependent loads. Every BBCC satisfies rec_array[rec_index] == itself, so when the wanted level already is the BBCC's own one the lookup provably returns the same BBCC. Guarding it with `idx != bbcc->rec_index` skips the pointer chase entirely in the normal, non-recursive case. VG_(arena_malloc): the allocator started with a linear walk over the 112 free-list heads to find the lowest non-empty list at or above the requested size class, touching up to 14 mostly-NULL cache lines per allocation. Track the non-empty lists in a two-word bitmap maintained by mkFreeBlock() and unlinkBlock() -- the only two sites that transition a list between empty and non-empty -- and answer the query with a mask plus a count-trailing-zeros. The former `if (NULL == b) continue;` becomes a vg_assert(b), a live self-check that bitmap and lists agree. Callgrind output is byte-for-byte identical to the unpatched build on 8 configurations (default, --separate-recs=1/3/10, --skip-direct-rec=no, --separate-callers=3, /bin/echo, and --cache-sim=yes --read-inline-info=yes), including all event counts and the summary line. The Callgrind regression tests pass, as do the Memcheck ones, which stress the allocator change directly.
Merging this PR will not alter performance
Comparing Footnotes
|
Greptile SummaryThe PR removes two hot-path pointer scans while preserving existing selection behavior.
Confidence Score: 5/5The PR appears safe to merge, with no concrete behavioral, build, or security regression identified. The recursion fast path relies on an invariant established by BBCC construction, while every current freelist empty-state transition updates the new bitmap and the lookup arithmetic remains valid across supported word widths.
|
| Filename | Overview |
|---|---|
| callgrind/bbcc.c | Avoids a redundant recursion-array dereference when the requested recursion index already equals the BBCC's self index; the established BBCC initialization paths preserve that invariant. |
| coregrind/m_mallocfree.c | Adds and consistently maintains a per-arena non-empty-free-list bitmap, replacing the allocator's linear list-head scan without changing block selection semantics. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Allocation request] --> B[Map size to starting free-list index]
B --> C[Mask freelist_used bitmap]
C --> D{Non-empty list found?}
D -- No --> E[Allocate a new superblock]
D -- Yes --> F[Scan selected free list]
F --> G{Suitable block found?}
G -- Yes --> H[Unlink and allocate block]
G -- No --> I[Query next set bitmap bit]
I --> D
J[mkFreeBlock: empty to non-empty] --> K[Set bitmap bit]
L[unlinkBlock: non-empty to empty] --> M[Clear bitmap bit]
Reviews (1): Last reviewed commit: "perf: remove two hot-path memory indirec..." | Re-trigger Greptile
Summary
Flamegraph analysis of the Callgrind walltime benchmarks pointed at two dependent-load stalls that Valgrind pays on its two hottest paths — once per basic block execution, and once per arena allocation. Both are removed without changing any observable behaviour.
1.
CLG_(setup_bbcc): skip the recursion-level lookup when it is a no-op--separate-recsdefaults to2, so theseparate_recursions > 1block inCLG_(setup_bbcc)runs on every basic block execution. It ended with:rec_arraylives in a separate allocation (CLG_(new_recursion)), sorec_array[idx]is a cold pointer chase, and the assertion adds two more dependent loads on the result. In the profile ofpython3 testdata/test.py, no-inlinethose three instructions carried 9.4 % of total run time (4.09 % + 2.98 % + 2.37 % self time), and 8 % infull-with-inline.Every BBCC satisfies the invariant
rec_array[rec_index] == itself— established inclone_bbcc(both branches), inCLG_(get_bbcc)'s fresh-BBCC path and inCLG_(setup_bbcc)'s ownrec_array[0] = bbcc. So whenidx == bbcc->rec_index— the normal, non-recursive case — the lookup provably returnsbbccagain. Guarding the block withif (idx != bbcc->rec_index)skips the pointer chase and the assertion entirely;rec_indexsits in the same cache line asrec_array, in a struct already touched a few lines above.2.
VG_(arena_malloc): replace the linear free-list scan with a bitmapThe allocator's first step is "find the lowest non-empty free list at or above my size class", implemented as a linear walk over the
N_MALLOC_LISTS == 112list heads — up to 14 cache lines touched per allocation, almost all of themNULL. That loop cost 4.4 % of run time in the same profile.The source comment already proposed fixing this with a shortcut array. This patch uses the simpler and cheaper variant: a two-word bitmap (
freelist_used) with bit i set ifffreelist[i] != NULL. Only two sites transition a list between empty and non-empty (mkFreeBlockandunlinkBlock), so maintenance is two masked bit operations; the query becomes a mask plus__builtin_ctzll. The other two writers offreelist[...](swizzle, and the "step one along" path inside the search) keep the head non-NULL, so the bitmap stays exact. The oldif (NULL == b) continue;becomes avg_assert(b)— a live self-check that the bitmap and the lists agree.Correctness
Verified against an unpatched build of the same commit, configured and built identically:
--separate-recs=1,--separate-recs=3,--separate-recs=10,--skip-direct-rec=no,--separate-callers=3,/bin/echo, and--cache-sim=yes --read-inline-info=yes— including all event counts and thesummary:line. The recursion configurations are exercised with a fixture using both direct (fib) and mutual (ping/pong) recursion. (The only textual difference is each build's own install path appearing in acob=line, as expected.)gone_abrt_xml,sem,vcpu_bz2), all pre-existing sandbox/environment artefacts. This is a direct stress test of the allocator change, since every Memcheck run hammersVG_(arena_malloc)/VG_(arena_free).Measurement
Base and head were measured in this sandbox with
codspeed run --mode walltimeover 12valgrind.codspeedbenchmarks (20 measured rounds each, 2 s warmup), coveringecho,python3 testdata/test.py,stress-ngandllsc_tzconvert_benchacross theno-inline,inline,full-with-inlineandcycle-estimationconfigurations.The improvement is consistent across the whole matrix rather than concentrated in one benchmark, which matches the fact that both changes sit on paths every Callgrind run takes. The two same-build controls bound this sandbox's noise floor well below the measured effect. Individual benchmarks stay under CodSpeed's per-benchmark significance threshold; the aggregate impact is the meaningful signal here.