Skip to content

fix(kll): keep compaction out of the insert path; doc follow-ups - #246

Open
zzylol wants to merge 2 commits into
mainfrom
fix/kll-lazy-buffer-followups
Open

zzylol wants to merge 2 commits into
mainfrom
fix/kll-lazy-buffer-followups

Conversation

@zzylol

@zzylol zzylol commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Why

Review follow-ups to the lazily grown KLL item buffer that #242 brought to main (from #238), plus one doc fix on UnivMon::check_shape:

  • Stream-path regression. A 50M-update stream ran about 10% slower on 0.4.0 than before. The lazy buffer moved the full-buffer path into a #[cold] make_room, which left compress_while_updating with a single call site in push_value. The compiler inlined it there. push_value grew to about 2.3 KB and stopped inlining into update's callers. nm on the two release builds shows push_value as its own 2,348-byte symbol on 0.4.0, and absent (inlined) before.
  • A stale comment. The KLL ASAPv1 decoder says a decoded sketch's level array is identical to the source's. Since the buffer is sized to the retained items, only the boundaries relative to the first retained item match.
  • UnivMon::check_shape's doc. More than MATRIX_MAX_ROWS rows pass the check and build in memory, but to_asapv1 refuses them. The doc did not say so.

What

  • Kll::compress_while_updating is #[inline(never)]. It runs once per level-0 fill, so keeping it out of line costs nothing on the hot path, and push_value inlines again.
  • src/sketches/kll/wire.rs: the decoder comment states what matches.
  • UnivMon::check_shape doc: a pyramid with more than MATRIX_MAX_ROWS rows builds but cannot be encoded. Callers that need an encodable state must check the row count.
  • CHANGELOG entry under Unreleased.

The other review items are already on main: the decoder's dead offset is gone (cursor starts at 0), and lazy_allocation_matches_eager_across_lifecycle compares clear(), decode-then-update and merge against the eager reference. The CHANGELOG blank-line and line-length nits were in #238's own text, which #242 did not carry over.

How

One attribute and doc text; no behavior change. Retained items, coin tosses and ASAPv1 bytes are unchanged; the existing eager-equality and golden tests cover them.

Before

nm --size-sort kll_bench (0.4.0):  0x92c  Kll::push_value   (out of line, compaction inlined)
stream, k = 469:                   34.7 ns/update

After

nm --size-sort kll_bench (this PR): Kll::push_value absent (inlined), Kll::compress_while_updating 0x84f
stream, k = 469:                    32.5 ns/update

Verification

Benchmark: a local example, not committed. It runs a 50M-update stream with xorshift input and with_seed(k, 8, 7), and creates 1,000,000 KLLs (k = 469) with 10 updates each, held 10,000 at a time. Release build, pinned to one core (taskset -c 50), the three builds alternated in each round, 7 rounds. The host was shared and its load swung from 22 to 70 during the session. Rounds 2–4 had the lowest load, and all three builds were stable within 2% in them; the numbers are their ranges:

before #242 (60101c6) main (eb04e7f) this PR
stream, k = 469 31.1–31.5 ns/update 34.7 32.5–32.6
stream, k = 200 32.7–34.4 34.3–35.5 32.2–32.3
create 1M KLLs, k = 469, 10 updates each 13,890–14,546 ns/sketch 923–1,123 898–924

Remaining gap: within 4% of 60101c6 at k = 469 under that load (main: 10–11%), within noise at k = 200; a review run on a quieter host measured parity. The CHANGELOG uses the same figure. The lazy buffer's 15× creation speedup is kept.

Checks: cargo fmt --all -- --check; clippy -D warnings with --all-features, default features, octo-runtime and experimental; cargo test --all-features --locked (2,233 passed) and cargo test --locked (1,977 passed); RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --locked.

With the lazily grown item buffer (0.4.0), compress_while_updating had a
single call site left in push_value, so it was inlined there; push_value
grew to about 2.3 KB and stopped inlining into update's callers, and a
50M-update stream ran about 10% slower (34.7 vs 31.1 ns/update at
k = 469). compress_while_updating is #[inline(never)] again; push_value
inlines and the stream runs at 32.5 ns/update.

- kll/wire.rs: the decoder comment claimed a decoded sketch's level array
  is identical to the source's; only the boundaries relative to the first
  retained item match.
- UnivMon::check_shape: more than MATRIX_MAX_ROWS rows build in memory
  but to_asapv1 refuses them; a caller that needs an encodable state must
  check the row count too.
@zzylol zzylol added bug Something isn't working documentation Improvements or additions to documentation labels Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant