Repository navigation
perf: kll_sketch grows its items buffer by 8x up to k - #569
Draft
stojkomilos wants to merge 6 commits into
Draft
stojkomilos wants to merge 6 commits into
stojkomilos wants to merge 6 commits into
Conversation
The constructor allocated storage for k items up front, so every empty sketch held k * sizeof(T) bytes (1.6 KB for the default k with doubles). Code that creates many sketches that stay empty or small (one per group in a grouped aggregation) paid for all of it. Allocate the items on the first update or non-empty merge instead. The copy and converting constructors only allocate if the source has items; the destructor and the iterator already handle an unallocated buffer. Results and the serialized format are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Check for an empty sketch in get_sorted_view before sorting level zero or forming item ranges. This shared entry point is the only caller of sort_level_zero, so no per-call-site null checks or eager allocation are needed. Exercise empty views after construction, copies, moves, assignments and deserialization, then verify that first updates still allocate and sort correctly. The new custom-type regression reproduces the null-pointer offset under UBSan before the fix.
Review feedback: rather than each caller guarding against items_ being
nullptr, keep that handling inside the helpers that own item storage:
- allocate_items(): the only place items_ is allocated (update, merge)
- copy_items(): the only place items are copied (copy and converting
constructors), replacing two duplicated null checks
- item_ptr(): the only place an item index becomes a pointer for readers;
without item storage every range is empty, so sort_level_zero(),
check_sorting() and get_sorted_view() need no guards of their own
get_sorted_view() is back to its original form, without the is_empty()
early return. UBSan flags the unguarded pointer arithmetic on an empty
sketch ("applying non-zero offset to null pointer"), and passes with
item_ptr().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Builds on apache#558, which allocates the items buffer on the first update or merge. That allocation now has room for min(k, 200) items instead of k, and a full buffer smaller than k grows by RESIZE_FACTOR (X8) up to k. Compaction still starts only when the buffer holds k items, and while there is one level the serialized levels are offsets into a buffer of k items, so results and serialized bytes do not change. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
This branch has not been deployed
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
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.
This PR builds on #558 and is meant to be merged after #558 is merged. Its first three commits are #558. This PR adds the two commits after them, and a merge of master that resolves a conflict in
kll_sketch_test.cpp.NOTE - this PR is AI written, and shouldn't be reviewed yet, as I will polish and rewrite it better if @rustyconover decides he won't add these changes in his #558 PR
What this adds on top of #558
#558 allocates the items buffer on the first update or merge, but then allocates all k items at once, so a sketch with one item still holds 512 KiB at k = 65535 with doubles. This PR makes that first allocation room for min(k, 200) items, and grows a full buffer by 8x up to k: 200, 1600, 12800, then k.
kll_constants::RESIZE_FACTOR = resize_factor::X8, the same as the theta default. Areserve()as in the dev@ proposal would allocate k items, and then the buffer never grows.Bytes held by a
kll_sketch<double>with k = 65535 (items buffer and levels):At the default k = 200 nothing changes, because the first allocation is already k items.
The only output that changes is
to_string(): its "Capacity items" line shows the current buffer size of a sketch that is still growing. I can print k there instead if you prefer.Tests
New test case
kll sketch: lazy growth of the items buffer:test_allocatorbytes follow 200, 1600, 12800, 65535 items, and the first merge into an empty sketch allocates 200 items.random_utils::random_bitstate, through compactions (k = 201, 5000, 20000, up to 10k items), merges in both orders, copies, moves, the type-converting constructor and strings. Items, weights, min and max, quantiles and ranks at 101 ranks, PMF, CDF and both serializations are identical.override_seed()seeds onlyrand, and KLL draws fromrandom_bit, so the test copies that state.Forcing the serialized level offset to 0 makes this test case fail.
All 17 CTest suites pass with GCC 10.5 and C++11, and
kll_testalso passes with ASan and UBSan. A separate harness (not in this PR) wrote bitwise-identical serializations with master, #558 and this PR. It covers double, float, int64 and string items, 15 values of k, n around every buffer size, 4 input orders, 2 seeds, merges by copy and by move, and the type-converting constructor.Benchmarks
benchmarks/benchmark_kll_sketch.cpp(new) onkll_sketch<double>, with items from a fixed-seed 64-bit LCG (integers spread uniformly over [0, 2^53)). I built the same benchmark against master, #558 and this PR, with GCC 10.5 in Release, on an x86_64 VM. The three binaries ran in 5 rounds in random order, on one pinned core, with the median of 5 repetitions per round. The times are the median over rounds, and the range is the ratio per round.Filling one sketch up to k copies its items once per growth step (200, 1600 and 12800 items for k = 65535), which costs 6% to 12% until the first compaction. A sketch with 1 or 10 items takes 12 to 14 ns longer at k >= 5000. Its time then matches a sketch at the default k = 200 (49 ns on master), which makes the same 1.6 KB first allocation.
Java
KllHeapDoublesSketchalso allocates its k items up front. I can make the same change there once the dev@ convention is in.