Repository navigation
perf(frequencies): skip key comparisons when rehashing - #331
Conversation
…rehashing Growing the map re-inserted distinct keys through `adjust_or_put_value`, which compares keys along each probe path. Rehashing now uses `insert_absent`, which follows the same probe sequence and drift counting without the comparisons, so the resulting table is identical. Add tests that grow the map through several doublings and check every estimate. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Integrate the current main branch before validating the frequencies rehash optimization and its regression coverage.
Growth-only estimate checks cannot detect incorrect probe distances until a later purge deletes entries from a collision chain. Exercise a non-Clone key type with a constant hash through repeated growth, duplicate updates, a weighted purge, and reinsertion. Verify retained counts, error bounds, active entries, and total stream weight through the public sketch API. Check all existing items at each actual growth boundary instead of arbitrary batches, and remove the unused Clone bound and identical estimate callbacks from the growth helper. Keep the growth serialization round trip in the dedicated serialization test target, including its retained-count and zero-error checks. Validation: cargo x check, cargo x test, cargo x lint, and cargo x bench -- --test pass with the pinned TCK fixtures. Temporarily replacing rehash probe distances with 1 makes the collision test fail after purge, confirming that the regression test detects the intended invariant violation.
Document the user-visible improvement to FrequentItemsSketch updates for distinct-item streams while the map grows. Scope the claim to the measured workload; repeated-key streams and sketches at their maximum map size do not generally benefit from skipping comparisons during rehash. Compared main at 4618600 with the updated PR at 6185c87 on an Apple M4 Max running macOS 26.6.2 and rustc 1.101.0-nightly (8006091eb). Ran the unchanged frequencies::update workloads with --repeat 5 --raw in three baseline/PR pairs, reversing order for the middle pair. Each revision has 15 samples at the registered fixed round counts (2000 for u64, 900 for bytes_32). Aggregate median and full range in milliseconds: u64 301 [264, 362] to 176 [168, 198], ratio 0.58; bytes_32 323 [318, 329] to 268 [265, 285], ratio 0.83. These are whole-workload elapsed times, including construction and cleanup, on one machine. No production code or benchmark workload changed after the measured PR revision.
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
The optimization is sound: the source map contains distinct keys, and rehashing into an empty destination can find the first empty slot without equality checks. The probe sequence, displacement accounting, insertion order, and counters are preserved. Keeping this as a small private insertion path is appropriate; a more general probing abstraction would not solve an additional problem here.
Pushed three follow-up commits:
6185c87: merge currentmain(4618600) into the branch.892b2b4: verify growth at actual capacity changes; exercise non-Clone colliding keys through growth, duplicate updates, purge, and reinsertion; move the growth serialization round trip into the serialization test target. A mutation that writes an incorrect rehash displacement passes initial lookups but fails the new test after purge, confirming coverage of the important invariant.1507af2: document the performance improvement with a scope limited to distinct-item updates while the map grows.
Validation passed: cargo x prepare-testdata, cargo x check, cargo x test, cargo x lint, and cargo x bench -- --test.
I also compared 4618600 against 6185c87 using the unchanged benchmark runner and workloads on Apple M4 Max, macOS 26.6.2, rustc 1.101.0-nightly (8006091eb, aarch64-apple-darwin). Both were built with cargo x bench --no-run; the resulting binaries were invoked with frequencies::update --repeat 5 --raw, in base/PR, PR/base, base/PR order. No benchmark compilation or tests ran concurrently with measurement. The registered fixed round counts were retained: 2,000 for u64 and 900 for bytes_32. The table aggregates all 15 samples per revision, including the first samples and outliers; times include construction, updates, and cleanup.
| Workload | Base median [min, max], ms | PR median [min, max], ms | PR/base |
|---|---|---|---|
frequencies::update::u64 |
301 [264, 362] | 176 [168, 198] | 0.58 |
frequencies::update::bytes_32 |
323 [318, 329] | 268 [265, 285] | 0.83 |
The follow-up commits after the measured revision change only tests and the changelog. These results support the optimization on this machine, but do not establish a universal percentage improvement. Both scenarios build sketches from 10,000 distinct keys; repeated-key streams and sketches whose maps have stopped growing should not be expected to see the same benefit.
Summary
When
FrequentItemsSketchgrows its map, it re-inserted every key throughadjust_or_put_value, which compares the key against each occupied slot on its probe path. The old keys are distinct by construction, so rehashing now usesinsert_absent, which follows the same probe sequence and drift counting but skips the key comparisons. The resulting table is identical.On the benchmark machine this makes updates that include map growth take 5% to 10% less time: a time ratio of 0.95 for integer keys and 0.90 for 32-byte keys. Peak heap, allocation counts, and retained heap are unchanged.
Results
Main benchmark suite, 6 rounds. The ratio is the time after this change divided by the time before; below 1 is faster.
frequencies::update::bytes_32frequencies::update::u64Scope of the results
The change affects only the rehash that runs when the map grows. The benchmarks insert 10,000 distinct keys into a map that can hold up to 16,384, so they include the whole growth phase, which is the best case for this change. A stream with many repeated keys, or a sketch whose map has stopped growing, rehashes rarely or never and gains little. The saving grows with the cost of comparing keys: it is larger for 32-byte keys than for integers. It removes work rather than relying on a CPU feature, but the numbers come from one machine and I have not measured others.
Checks
cargo x check,cargo x lint,cargo x test, andcargo +1.86.0 check -p datasketches --all-featurespass.Measured with Divan on one machine (2x Xeon E5-2660 v3, Linux 6.8) against
bb6ba57, using paired rounds that alternate sockets; ratios are the geometric mean of per-round median ratios. The branch is based on upstreammainat4ba130e; the numbers were not repeated there, because the commits in between change HLL and XOR code, benchmark timing units, and two variable renames in KLL and t-digest.