This repository was archived by the owner on Aug 13, 2019. It is now read-only.
Reuse Chunk Iterator - #642
Merged
Merged
Conversation
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Contributor
Author
|
Pointing out the function which causes the allocs |
brian-brazil
left a comment
brian-brazil
left a comment
Contributor
There was a problem hiding this comment.
Reset should always work.
Contributor
Author
For |
Contributor
|
Sounds like this isn't quite the right abstraction then. How about something more type specific? |
Contributor
Author
|
You mean to say have that method to the struct and deference the interface to the type in order to call that method when needed? |
Contributor
|
Yes, it really depends on what the call site is. |
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
codesome
changed the title
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
codesome
commented
Jun 28, 2019
Contributor
|
I would like to see what the benchmarks look like for this, as when I tested things like this previously it didn't help much. Does this change any of the API semantics? |
Contributor
Author
|
Tested on the same query and data as described in this comment. Also the code changes that I made in cortex querier to benchmark this is exactly similar to what I did in With repeated benchmarks, apparently, the gain in time seems to decrease. So it's mostly allocs and B/op. |
Contributor
Author
I am not actually sure what you mean here. |
codesome
marked this pull request as ready for review
June 28, 2019 12:10
Contributor
Author
I am not actually sure what you mean here. |
Contributor
|
Currently a caller of this code can keep multiple of the returned iterators around, now it's only valid until the next Seek/Next. |
Contributor
Author
I am setting the iterators while the iterator is being created https://github.com/prometheus/tsdb/blob/cef83448cdf1fd61850b870976786dddcd99dee7/querier.go#L1082 |
added 3 commits
July 3, 2019 16:53
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Contributor
Author
|
Now I am also re-using |
brian-brazil
reviewed
Jul 4, 2019
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Contributor
Author
|
I have come up with another way to re-use the iterator. |
Contributor
Author
|
Here are the benchmarks results from |
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
brian-brazil
reviewed
Jul 5, 2019
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Contributor
|
👍 |
Contributor
Author
|
(Will hold off merging until 2.11.0 is out, just in case if there is a bugfix to be included from tsdb in the release) |
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Want feedback if something like this would be acceptable.
When there are millions of chunks to be parsed for a query, re-using the chunk iterator saves a lot of allocs and in my benchmark in cortex it shed up to
5+sfor a query. (I don't have an exact number of chunks as of now).