perf(iterator): elide y.ParseKey in hasPrefix fast path#2293
Open
shaunpatterson wants to merge 2 commits into
Open
perf(iterator): elide y.ParseKey in hasPrefix fast path#2293shaunpatterson wants to merge 2 commits into
shaunpatterson wants to merge 2 commits into
Conversation
`hasPrefix` runs on every iterator.Next() call. Previously it always called `y.ParseKey(iitr.Key())` (nil check + sub + slice) before delegating to bytes.HasPrefix. When the user-supplied Prefix fits entirely within the userKey portion (len(internalKey) >= len(Prefix)+8), bytes.HasPrefix on the full internal key is equivalent to bytes.HasPrefix on ParseKey(internalKey) — the prefix bytes can only land inside the userKey region, never inside the 8-byte ts suffix. The fallback (prefix longer than userKey) still takes the ParseKey path so we don't spuriously match against ts bytes. Direct motivation: dgraph's posting/mvcc.go ReadPostingList walks all versions of a single key on every cache miss and every opRollup batch (16 keys per batch, continuous). hasPrefix is on that hot loop. Tests added: - TestRegressionHasPrefixFastPath: prefix < userKey hits the fast path - TestRegressionHasPrefixShortPrefixFallback: prefix > userKey hits the fallback and correctly returns false Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous fixture used userKey="a" with prefix="abcdefghij". Seek lands past EOF (since "a" sorts before "abcdefghij") so hasPrefix is never called and the ParseKey fallback branch stays uncovered. Switch to userKey="z" with prefix="yy" — "z" sorts after "yy" so Seek lands on it, hasPrefix() runs, and the len(key)=9 < len(prefix)+8=10 test forces the fallback path. Confirmed: line 736 now hit in coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
1 participant
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
hasPrefixruns on everyiterator.Next()call. Previously it always calledy.ParseKey(iitr.Key())(nil check + sub + slice) before delegating tobytes.HasPrefix. When the user-suppliedPrefixfits entirely within the userKey portion (len(internalKey) >= len(Prefix)+8),bytes.HasPrefixon the full internal key is equivalent tobytes.HasPrefixonParseKey(internalKey)— the prefix bytes can only land inside the userKey region, never inside the 8-byte ts suffix.The fallback (prefix longer than userKey) still takes the
ParseKeypath so we don't spuriously match against ts bytes.Motivation
dgraph's
posting/mvcc.goReadPostingListwalks all versions of a single key on every cache miss and everyopRollupbatch (16 keys per batch, continuous).hasPrefixis on that hot loop.Tests added
TestRegressionHasPrefixFastPath: prefix < userKey hits the fast pathTestRegressionHasPrefixShortPrefixFallback: prefix > userKey hits the fallback and correctly returns falseTest plan
go test ./...passes🤖 Generated with Claude Code