Optimize long alternate lists #463
Merged
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
|
|
charleskorn
reviewed
Mar 30, 2023
|
Further testing: I downloaded a dataset of 713287 real queries from the cell where this issue was discovered (Cell A), and 504496 queries from a shared cell where this issue is not noticeable (Cell B). I wrote a small program to parse the queries and throw away the result. Much of this time is spent reading the queries from disk or running GC, but nonetheless the results are promising:
I'm cautiously optimistic that the small increases in cost don't manifest in real queries. Furthermore, I believe there is still significant improvements to be made by supporting case insensitive matches via the prefix trie. Cell A has 79% of queries making use of |
I would defer it to a follow up PR. |
|
I just pushed a commit to fix fuzzy test with the fuzzy regexp: 0590a86 |
|
I was wondering if we really need a trie, or a map is just good enough. So I picked this PR and implemented your proposed optimization just using a map (specifically using This is the Maybe it's something you want to explore more. Also be aware of |
pracucci
reviewed
Mar 30, 2023
pracucci
reviewed
Mar 30, 2023
About this ☝️ : in the meanwhile I was exploring another optimization, but didn't get much benefit so I'm not going to open a PR. However, you may want to look at the changes I did to |
|
Based on @pracucci's feedback I profiled an implementation with a map and also with a slice / map depending on input size. The results favor using a slice / map, so I've replaced the trie. Full Benchmark Results |
Logiraptor
changed the title
pracucci
reviewed
Mar 31, 2023
…e fuzzy regex is invalid Signed-off-by: Marco Pracucci <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>
Logiraptor
force-pushed
the
logiraptor/optimize-alternating-literals
branch
from
828342e to
297c90f
Compare
|
Now with this PR and #465 together: I downloaded a dataset of 713287 real queries from the cell where this issue was discovered (Cell A), and 504496 queries from a shared cell where this issue is not noticeable (Cell B). I wrote a small program to parse the queries and throw away the result. Much of this time is spent reading the queries from disk
|
pracucci
approved these changes
Apr 1, 2023
3 tasks
Logiraptor
added a commit
that referenced
this pull request
Jan 2, 2024
I was surprised to find out that posting lookups for `foo=~"(bar|bar)"` are faster than `foo=~"bar"`. It turns out we introduced a performance regression in #463. When we added the `optimizeAlternatingLiterals` function, we subtly broke one edge case. A regexp matcher which matches a single literal, like `foo=~"bar"` used to return `bar` from `SetMatches()`, but currently does not. The implication is that the tsdb will first do a LabelValues call to get all values for `foo`, then match them against the regexp `bar`. This PR restores the previous behavior which is able to directly lookup postings for `foo="bar"` instead.
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.
We've seen that very long regex matchers of the form
abc|def|...consume a large amount of CPU both inNewFastRegexMatcherand while matching label values. This PR uses a slice or map to optimize both operations as a special case when the regex is one long alternate of literal values. In that special case, the regex parsing is skipped entirely. As an example, this can occur when using Grafana dashboard template variables with theAlloption selected.Unfortunately, this introduces a constant performance cost in
NewFastRegexMatcherfor other regex queries, but It's not clear that this is actually significant in practice. For example, cpu for a simple case:This is a 10% increase, but in concrete terms less than 0.3µs on my laptop.
Here are the results for the specific edge case this PR addresses:
Full Benchmark Results