perf(iterator): -25% on dgraph-shaped scans via lockless isBanned, hoisted fill, internal-key skip, KeyOnly opt-in, user-key lastKey#2285
Open
shaunpatterson wants to merge 1 commit into
Conversation
… etc)
Five distinct optimizations on the iterator hot path, plus the
structural cleanups flagged by the thermo-nuclear review.
db.go:
- lockedKeys: add hasAny atomic.Bool; flipped true on first add().
DB.isBanned now fast-paths when no namespaces have ever been
banned (the common production case), skipping the RLock entirely.
isBanned is called on every iterator step and every Txn.Get/modify,
so avoiding the lock here matters when NamespaceOffset is enabled
but no bans are active.
errors.go:
- ErrKeyOnlyMode: returned by Item.Value / Item.ValueCopy when the
containing iterator was created with IteratorOptions.KeyOnly=true.
y/y.go:
- y.SameUserKey: bytes.Equal wrapper for already-parsed user keys.
Centralizes the pattern so callers (parseItem) don't re-do the
8-byte-strip math themselves.
iterator.go:
- IteratorOptions.KeyOnly: skip copying value bytes (and the vptr)
into each yielded Item. Forces PrefetchValues off in NewIterator
since the prefetch goroutine would just throw the value away.
- Item.keyOnly: per-item marker so Value/ValueCopy/EstimatedSize/
ValueSize can short-circuit on KeyOnly iterators.
- IteratorOptions.precludesInternalKeys() method: replaces the
free-function canSeeInternalKeys(prefix) and the Iterator field
it set. The prefix lives on the options struct already; the
predicate is a property of those options, not the iterator.
- parseItem:
- Use opt.precludesInternalKeys() to gate the per-step
bytes.HasPrefix(key, badgerPrefix) probe.
- Defensive guard: if len(key) < 8 (corrupt block, short key
slipping past the 8-byte ts invariant) skip rather than panic
on the ukLen := len(key)-8 calculation that follows.
- lastKey now holds the user-key only (no ts suffix). Comparison
goes through y.SameUserKey, eliminating the per-step
y.SameKey/ParseKey pair.
- Inline same-key compare: bytes.Equal on the user-key slice
(with the explicit length check kept).
- fill(item, key, vs *y.ValueStruct): signature changed to take the
already-fetched key and a *ValueStruct pointer. Avoids the
per-item mi.Key() / mi.Value() call and the ~40-byte ValueStruct
copy on the hot path.
iterator_keyonly_test.go: regression coverage for KeyOnly modes,
precludesInternalKeys contract, the user-key-only dedup behavior,
and the hasAny fast-path.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
16e8b0f to
fcb6ead
Compare
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.
Summary
Five targeted optimizations to the iterator hot path, measured against a new dgraph-shaped benchmark suite. Composite improvement: −25.4% vs upstream/main on the 5-bench composite (sum of ns/op medians, Apple M4 Max,
-benchtime=5s, median-of-3).PrefixScanKeyOnlyPrefixScanKeyOnlyOpt(new, opt-in)PrefixScanAllVersionsKeyIteratorAllVersionsNamespaceOffset/offNamespaceOffset/dgraphWhat changed
Six commits behind the perf series + 2 doc/test commits:
bench— Addsiterator_dgraph_bench_test.gowith 5 benches that model dgraph's iteration patterns (NamespaceOffset=1, NumVersionsToKeep=MaxInt32, DetectConflicts=false). These are the metric for every iteration below.perf: lockless fast path for DB.isBanned when no namespaces banned— Adds monotonicatomic.Bool hasAnyonlockedKeys.isBannedreturns on a single atomic load when no namespaces are banned (the production steady state for tenants who enable NamespaceOffset for isolation but rarely ban). EliminatesRWMutex.RLockper iterator step. Sets the flag inside the same lock that guards the map, so any reader observinghasAny=trueis guaranteed to see the populated map via release-acquire on the lock.perf: hoist mi.Value() and key out of fill()—fill(item)→fill(item, key, *y.ValueStruct). The KeyOnly path'sparseItempreviously made twomi.Value()+Decode calls per kept item.vsis passed by pointer to avoid copying the ~40-byte ValueStruct on every kept item.perf: skip per-step internal-key probe when prefix excludes badgerPrefix— PrecomputecanSeeInternalKeysonce at iterator construction. When the user'sopt.Prefixexists and its first byte differs frombadgerPrefix[0], no key the iterator lands on can be a badger-internal key, so the per-stepbytes.HasPrefix(key, badgerPrefix)probe is dead code.perf: IteratorOptions.KeyOnly skip per-item value SafeCopy— New opt-inIteratorOptions.KeyOnly. When set,fillskips the per-itemSafeCopy(item.vptr, vs.Value).Item.Value/Item.ValueCopyreturn a newErrKeyOnlyMode(strict-error, not silent-empty).PrefetchValuesis forced off whenKeyOnly=true. Captures an additional ~2–4% on top of the implicit gains for callers (e.g. dgraph'shas()evaluator / index scans) that already discarditem.vptr.perf: store user-key only in lastKey, inline same-key compare—Iterator.lastKeypreviously held the full key (user-key‖ts). Stores user-key only now. Replacesy.SameKey(lastKey, key)with an inlinedlen(key)-8 == len(lastKey) && bytes.Equal(key[:ukLen], lastKey). Avoids oneParseKeyperSameKeyand an 8-byte memcpy on each lastKey update. Same-key dedup invariant preserved exactly.docs(iterator): clarify FILL key-freshness and ukLen invariants— Strengthened comments on two hot-path invariants surfaced during code review.test: cover KeyOnly iterator paths and preserved-behavior regressions— Newiterator_keyonly_test.go. Together with the benchmarks, every touched line indb.goanditerator.gois now at 100% coverage (verified viago tool cover).Correctness
Every commit preserves existing semantics:
add()stores the flag inside the same lock that guards the map → release-acquire visibility holds.keyat everyfill()call site is the currentmi.Key(); the onlygoto FILL(reverse path) refreshes it aftermi.Next().canSeeInternalKeys=false,isInternalKeystaysfalse, which is the correct value for prefix shapes that cannot overlapbadgerPrefix.Item.ValuereturnsErrKeyOnlyModerather than a silent empty slice.PrefetchValuesis forced off so the prefetch goroutine never racesfill.lastKeyuser-key-only representation is preserved across the existing "b 7 (del), b 5" hazard becauselastKeyis still updated before the deleted-or-expired check.Tests
TestPreserved*/TestRegression*) pass on upstream/main AND on this branch — they were validated on both via a worktree.TestKeyOnlyIterator_*,TestCanSeeInternalKeys) pass on this branch.go test -run='^Test' ./...): PASS in 234.7s.db.go(isBanned + lockedKeys.add) anditerator.go(Item.Value/ValueCopy/EstimatedSize/ValueSize, NewIterator, canSeeInternalKeys, fill, parseItem).Test plan
go test ./...— full suite passesgo test -bench=BenchmarkDgraph -benchtime=5s -count=3 .— composite −25.2% vs maingo tool cover -func=...— 100% on touched linesNotes for reviewers
IteratorOptions.KeyOnlyis opt-in; existing callers see no behavior change. Dgraph would need a one-line change at itshas()/ index-scan iterator construction sites to capture the additional 2–4% on those paths.PrefixScanAllVersions) dominates and hides smaller relative wins onKeyIteratorAllVersions. Per-benchmark breakdown above tells the full story.IteratorOptions.KeyOnly(bool, default false) andErrKeyOnlyMode(sentinel error).🤖 Generated with Claude Code