Skip to content

perf(db): make key flattening 2x faster with 1/3 allocs#3783

Merged
rodrodros merged 1 commit into
mainfrom
rdr/faster-key
Jul 2, 2026
Merged

perf(db): make key flattening 2x faster with 1/3 allocs#3783
rodrodros merged 1 commit into
mainfrom
rdr/faster-key

Conversation

@rodrodros

@rodrodros rodrodros commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Reduce unnecessary allocations, I suspect we can go even more to 0 allocations with fixed functions.

Summary: : 3→1 allocs, ~40–50% fewer bytes, and ~55% less time (geomean), strongest on the dominant single-felt call site.

Case Time (old → new) Allocs (old → new) Bytes (old → new)
NoKey (prefix only) 10.1 → 8.0 ns (−21%) 1 → 1 1 → 1
SingleFelt 47.5 → 15.9 ns (−67%) 3 → 1 (−67%) 81 → 48 (−41%)
PrefixPlusFelt 51.6 → 20.7 ns (−60%) 3 → 1 (−67%) 97 → 48 (−51%)
TwoFelts 54.7 → 21.2 ns (−61%) 3 → 1 (−67%) 145 → 80 (−45%)
FourFelts 68.5 → 31.2 ns (−54%) 3 → 1 (−67%) 273 → 144 (−47%)
ManyFelts (1000) 7.21 → 5.04 µs (−30%) 3 → 1 (−67%) 64 → 32 KiB (−50%)
geomean −55% −58% −39%

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rodrodros's task in 42s —— View job


PR Review: perf(db) key flattening

  • Read the diff and changed files
  • Verify correctness of the new Key implementation
  • Check call sites for capacity/aliasing assumptions
  • Review the benchmark

Verdict: clean — LGTM. The rewrite of Bucket.Key is correct and a solid, idiomatic single-allocation improvement.

The logic is equivalent to the old append([]byte{byte(b)}, slices.Concat(key...)...): precompute total size, allocate once, write the prefix byte, then copy each segment. Edge cases all hold:

  • NoKeysize == 1, returns [byte(b)].
  • empty sub-slices → len(k) == 0, copy is a no-op, i unchanged.
  • Every call returns a fresh, non-aliased slice, so the only difference from append (exact capacity vs. possible spare) is irrelevant — any caller that later appends to the result stays safe.

No blockers or important issues. Two nits (non-blocking, skip if you prefer):

  • nit db/buckets_bench_test.go:9-10 — typos in the comment: "optimazations" → "optimizations", and "compiler optimazations will give us better results" reads slightly off. Minor.
  • nit The benchmark's keySink guard against dead-code elimination is a nice touch; consider also asserting the result once in a plain unit test (db/buckets_test.go already exists) so the fast path stays behavior-covered, not just perf-covered — though existing Key usage across the suite already exercises it.

Nice speedup on the dominant SingleFelt path (−67% allocs, −67% time). 👍
· rdr/faster-key

@rodrodros rodrodros requested review from brbrr and ongyimeng July 2, 2026 11:02
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.41%. Comparing base (2095502) to head (86df61a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3783      +/-   ##
==========================================
+ Coverage   75.39%   75.41%   +0.02%     
==========================================
  Files         438      438              
  Lines       39463    39471       +8     
==========================================
+ Hits        29753    29768      +15     
+ Misses       7637     7634       -3     
+ Partials     2073     2069       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread db/buckets_bench_test.go Outdated

@brbrr brbrr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Noticed just a small typo in the comment.

@RafaelGranza RafaelGranza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread db/buckets.go
Comment thread db/buckets_bench_test.go
@rodrodros rodrodros enabled auto-merge (squash) July 2, 2026 15:16
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rodrodros's task in 33s —— View job


PR Review: perf(db) key flattening

  • Read the diff and changed files
  • Verify correctness of the new Bucket.Key implementation
  • Check call-site capacity/aliasing assumptions
  • Review the benchmark

Verdict: clean — LGTM. No blockers, important issues, or nits.

The rewrite of Bucket.Key (db/buckets.go:132) is a correct, idiomatic single-allocation replacement for append([]byte{byte(b)}, slices.Concat(key...)...): it precomputes the exact size (1 + Σ len(k)), allocates once, writes the prefix byte, and copys each segment. Behavior is fully equivalent:

  • NoKeysize == 1, returns [byte(b)].
  • empty sub-slices → copy is a no-op, i stays put.
  • every call returns a fresh, exact-capacity slice; any caller that later appends stays safe since the result is never aliased.

The benchmark (db/buckets_bench_test.go) is well-constructed — keySink prevents dead-code elimination, b.Loop() is the modern form, and the cases mirror real call sites plus the new ManyFelts (1000) stress case. The earlier comment typo has been fixed. Nice speedup on the dominant SingleFelt path. 👍
· rdr/faster-key

@rodrodros rodrodros merged commit 71319f9 into main Jul 2, 2026
28 checks passed
@rodrodros rodrodros deleted the rdr/faster-key branch July 2, 2026 15:42
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.

4 participants