Check bucket series time range when decoding chunks from index #3367
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
|
Running benchmarks is tricky but there are ways to make it reliable and more trustworthy.
|
|
Anyway good work I think it makes sense, wonder if we can clean decodeWithReq a bit more 🤗 Some reference for benchmarks: https://gist.github.com/bwplotka/3b853c31ed11e77c975b9df45d105d74 |
|
I finished the benchmarks and the new results between this pr and the current master are below. Seems it is not very good when there are not many samples per series.
|
|
After one more optimization, I think the performance is good now. That's the result:
|
yeya24
force-pushed
the
improve-index-valid
branch
from
c0b6a99
to
2f0ee7d
Compare
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
yeya24
force-pushed
the
improve-index-valid
branch
from
2f0ee7d
to
79f0eff
Compare
pracucci
reviewed
Nov 12, 2020
Comment on lines
+2082
to
+2083
| ref0 += d.Varint64() | ||
| t0 = maxt |
There was a problem hiding this comment.
This should be done before if maxt < req.MinTime, otherwise the next for iteration will read invalid data. This PR #3440 fixes it.
Oghenebrume50
pushed a commit
to Oghenebrume50/thanos
that referenced
this pull request
Dec 7, 2020
…s-io#3367) * check bucket series time range when decoding chunks from index Signed-off-by: Ben Ye <[email protected]> * reduce alloc Signed-off-by: Ben Ye <[email protected]> Signed-off-by: Oghenebrume50 <[email protected]>
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.
Signed-off-by: Ben Ye [email protected]
Changes
This idea behind this pr is to check the chunk time range when decoding the series from the index. This change can benefit a lot the
skipChunkqueries because it can immediately return if it finds a valid chunk instead of decoding all the chunks in this series.Verification
Uses the benchmarks from #3366. Benchmark and compare between this pr and current master.
SkipChunkquery.It is expected that this type of query should be much faster than before.
The expected performance of this should be almost the same or a little bit slower than before. However, this pr is about 10% - 20% slower when there are a lot of samples in one series. Not sure how to improve this case. I think there is no extra overhead except we are using more if-else statements than before when decoding the index.