Store gateway mmap removal: use SkipUvarintBytes() and simplify logic in PostingsOffset()
#3787
Conversation
This file contains 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
The performance impact is negligible, but this better conveys our intent.
The performance impact is negligible, but this better conveys our intent.
At present, we only ever look up a single value at once, and we gain a small performance boost by simplifying the logic: name old time/op new time/op delta PostingsOffset/20Names100Values-10 11.7µs ± 1% 11.6µs ± 1% ~ (p=0.151 n=5+5) PostingsOffset/20Names500Values-10 11.7µs ± 0% 11.5µs ± 0% -1.44% (p=0.008 n=5+5) PostingsOffset/20Names1000Values-10 11.7µs ± 0% 11.6µs ± 1% -1.22% (p=0.008 n=5+5) name old alloc/op new alloc/op delta PostingsOffset/20Names100Values-10 385B ± 0% 337B ± 0% -12.47% (p=0.000 n=5+4) PostingsOffset/20Names500Values-10 385B ± 0% 337B ± 0% -12.47% (p=0.000 n=5+4) PostingsOffset/20Names1000Values-10 385B ± 0% 337B ± 0% -12.47% (p=0.008 n=5+5) name old allocs/op new allocs/op delta PostingsOffset/20Names100Values-10 10.0 ± 0% 7.0 ± 0% -30.00% (p=0.008 n=5+5) PostingsOffset/20Names500Values-10 10.0 ± 0% 7.0 ± 0% -30.00% (p=0.008 n=5+5) PostingsOffset/20Names1000Values-10 10.0 ± 0% 7.0 ± 0% -30.00% (p=0.008 n=5+5)
pracucci
reviewed
Dec 21, 2022
There was a problem hiding this comment.
From a quick look to the logic, it LGTM. I defer @56quarters for a deeper review.
|
@charleskorn Could you fix the conflict and then ping a maintainer to merge? 🙇 |
…e-skip # Conflicts: # CHANGELOG.md # pkg/storegateway/indexheader/index/postings.go
pracucci
reviewed
Dec 23, 2022
| } | ||
| rngs = append(rngs, newSameRngs...) | ||
| break | ||
| rng.End = e.lastValOffset |
There was a problem hiding this comment.
[unrelated from this PR, non blocking] Given lastValOffset is used as the end range in case this is the last value for the given label name, I'm wondering if we could find a better name for lastValOffset, cause it sounds like "offset of the start of the last value" and not "offset of the end of the last value". Makes sense or am I missing something?
pracucci
deleted the
charleskorn/mmap-less-index-header-reader-use-skip
branch
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.
What this PR does
This PR contains two changes:
UnsafeUvarintBytes()withSkipUvarintBytes(), which has a negligible performance impact but clarifies intentPostingsOffset()to only support retrieving the byte range for a single name/value pair, as this is all we do at presentThis second change, in addition to making the code easier to read, also gives us a modest performance improvement:
If we do ever want to take advantage of the fact we can retrieve multiple ranges at once, we can always revert this change.
Which issue(s) this PR fixes or relates to
Relates to #3465, builds upon #3742.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]