Skip to content

Topk pr117 rebuild - #124

Open
zackcam wants to merge 12 commits into
valkey-io:topkfrom
zackcam:topk-pr117-rebuild
Open

zackcam wants to merge 12 commits into
valkey-io:topkfrom
zackcam:topk-pr117-rebuild

Conversation

@zackcam

@zackcam zackcam commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

This is a rebuild of: #117 with different changes. Thank you detemmienation for setting this up!

What this does

This change vendors the heavykeeper crate into the repo as a local path dependency and tunes it for Valkey. Before this, the module pulled heavykeeper from an external git branch. Vendoring it lets us make TopK-specific memory and correctness changes without waiting on the upstream crate.

Main changes

Vendoring and build:

  • Add heavykeeper under heavykeeper/ and switch Cargo.toml to a local path dependency.
  • Strip upstream dev-only files from the vendored copy.
  • Track our changes to the crate in MODIFICATIONS.md.

Memory reductions (the TopK sketch uses CuckooTopK):

  • Use u32 fingerprint and counter cells instead of u64, halving per-cell memory. A u32 counter cannot overflow because counts are saturated on the way in.
  • Store item keys up to 15 bytes inline (SmallKey) instead of always heap-allocating. Small keys (IPs, ids, short strings) now cost zero extra bytes.
  • Move TopKQueue to single-copy storage and merge the sketch cell arrays, removing duplicated per-key data.
  • Remove the decay-threshold lookup table.

Correctness and upkeep:

  • Rebuild the priority-queue lookup table when hashbrown tombstones grow it, so a key's memory footprint stays fixed under churn.
  • Add generic Fingerprint/Counter types so the cell width is a type parameter.
  • Add defrag support for the crate's large heap allocations.
  • Rebase on merged upstream commits and update the memory accounting to charge only spilled (heap) key bytes; inline keys are already counted in the queue's structural size.
  • The module code shrinks too: TopKObject no longer stores k/width/depth/decay separately, since those are read from the sketch.

Tests

  • All heavykeeper crate tests pass (161).
  • Module TopK integration tests pass (correctness, DUMP/RESTORE round-trip, memory-gauge stress, count saturation).
  • Full module builds clean.

Performance and memory

K sketch Redis + items VB bytes VB vs Redis
10 8x7 2960 1216 -59%
50 8x7 4560 2896 -36%
K sketch Redis + items VB bytes VB vs Redis
100 16x7 7008 5528 -21%
100 64x7 9696 8600 -11%
100 256x7 20448 20888 +2%

two example tables, at large large k and sketches, we cap at 15% for more memory (Note this is using the items added for redis memory which is untracked by memory usage in redis but is a part of the topk object)

Notes

The vendored crate keeps upstream's other implementations (TopK, BucketedTopK) even though the module only uses CuckooTopK, to stay a faithful mirror of the upstream crate.

For the future we will try and get these changes accepted upstream but if unable we will continue to use the vendored crate

zackcam and others added 12 commits September 16, 2026 18:50
Vendors upstream heavykeeper-rs at tag v0.7.1 (commit a5191aa, tip merge
d410785) as a local path dependency so it can be modified in-tree.

This baseline already includes upstream's native byte serialization for
CuckooTopK and BucketedTopK, the shared serialization module with magic
bytes, a variant tag, an on-load hasher probe, RDB-format hardening, and
the priority-queue redundant-lookup perf fix.

Provenance: https://github.com/pmcgleenon/heavykeeper-rs at v0.7.1

Signed-off-by: Cameron
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Remove files not needed by the vendored crate build: the data/ test
corpus (war_and_peace.txt plus its generator scripts and README), the
examples/ programs (basic, ip_files, word_count) which exist only to
demo the standalone crate and read the removed data corpus, and the
upstream .github/ CI config (dependabot, release-plz, rust workflows).
These are development and CI artifacts of the standalone heavykeeper-rs
repo and play no part in building the module. The Rust source, benches,
LICENSE, LICENSE-APACHE, README, CHANGELOG, and MODIFICATIONS.md are kept
so the vendored copy stays a recognizable upstream snapshot.

Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Introduce Fingerprint and Counter traits with impls for u64/u32/u16, and
parameterize CuckooTopK/CuckooBuilder over the cell storage widths. Both
default to u64, so existing CuckooTopK<T> usage is unchanged.

The decay threshold lookup table is retained.

Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Tracy <yuningt@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
…ions

Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Signed-off-by: Cameron Zack <zackcam@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants