MQE: don't reuse FloatHistogram instances for successive points in instant vector selectors
#9588
Conversation
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
…amples in a series
…tHistograms that have been used multiple times
jhesketh
approved these changes
Oct 14, 2024
There was a problem hiding this comment.
I agree with the rationale of this. I do wonder though if we can measure the real-world impact. Should we have a metric on number of copied samples? We can see if that rises too high if we should reconsider reimplementing such an optimisation down the road (although it may be a bigger risk to do later)
We should be able to see this on profiles, including the continuous profiling we run on Mimir instances in Grafana Cloud. |
jhesketh
approved these changes
Oct 14, 2024
I've added a test to ensure |
This was referenced Oct 14, 2024
charleskorn
added a commit
that referenced
this pull request
Oct 14, 2024
charleskorn
added a commit
that referenced
this pull request
Oct 14, 2024
charleskorn
added a commit
that referenced
this pull request
Oct 14, 2024
charleskorn
added a commit
that referenced
this pull request
Oct 14, 2024
charleskorn
added a commit
that referenced
this pull request
Oct 14, 2024
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 is an iteration on #8677. It changes the behaviour of instant vector selectors to not return the same
FloatHistograminstance for successive points if they all select the same histogram.This preserves the main optimisation of #8677 (avoiding creating copies of histograms in
sum) without the complexity and associated bugs of needing to ensure that sharedFloatHistograminstances are modified safely.For example, I just found yet another issue related to this that was introduced in #9507, where shared
FloatHistograminstances could be left in the unused portion of the histogram slice here and then corrupted if theHPointslice was later used for a range vector selector.I believe similar corruption could occur if a
HPointslice was used for an instant vector selector that returned the sameFloatHistogramfor successive points, and then that slice was reused for a range vector selector: the range vector selector would reuse theFloatHistograminstances when filling the buffer here and so all points would have the same value as the last histogram, rather than their expected values.The performance impact of this change is minimal in our benchmarks: less than 1% degradation or less than 5% reduction in latency in all cases. In some cases things get slightly slower due to the extra copies created, and in some cases things get slightly faster due to the simpler logic. I think the simpler logic and far reduced likelihood of bugs make this change worthwhile.
In the real world, where lookback is more common, we'll only need to make copies (and therefore pay the price of those copies) if the same point is used for successive output samples, which would usually only happen if the query step is smaller than the scrape interval. Given most users run with 15s or 60s scrape intervals, and most queries tend to be either instant queries, I don't think this is likely to become a huge issue.
Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.