Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TraceQL: Improve regexp perf #3139

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Conversation

joe-elliott
Copy link
Member

What this PR does:
Fixes a nasty performance issue when regexp matches get to the engine layer. Currently we are calling regexp.MatchString() which recompiles the regex on every span.

Uses benchmarks from this PR against main plus one to highlight the improvement:

{resource.namespace!=""} && {resource.service.name="loki-distributor"} && {duration>2s} && {resource.cluster=~"prod.*"}

name                                             old time/op    new time/op    delta
BackendBlockTraceQL/spanAttValNoMatch-8            6.71ms ± 0%    6.73ms ± 0%     ~     (p=0.151 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-8      5.80ms ± 0%    5.91ms ± 2%     ~     (p=0.056 n=5+5)
BackendBlockTraceQL/resourceAttValNoMatch-8        12.1ms ± 1%    12.1ms ± 1%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/resourceAttIntrinsicMatch-8    24.4ms ± 2%    24.0ms ± 1%   -1.64%  (p=0.032 n=5+5)
BackendBlockTraceQL/mixedValNoMatch-8               327ms ± 2%     317ms ± 2%   -3.04%  (p=0.008 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchAnd-8        5.76ms ± 1%    5.76ms ± 1%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchOr-8          326ms ± 3%     328ms ± 4%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/count-8                         577ms ± 3%     578ms ± 2%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/struct-8                        1.24s ± 4%     1.19s ± 5%     ~     (p=0.095 n=5+5)
BackendBlockTraceQL/||-8                            443ms ± 2%     446ms ± 2%     ~     (p=0.548 n=5+5)
BackendBlockTraceQL/mixed-8                        37.4ms ± 2%    37.6ms ± 3%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/spansets-8                      3.05s ± 1%     1.53s ± 3%  -49.90%  (p=0.008 n=5+5)

name                                             old speed      new speed      delta
BackendBlockTraceQL/spanAttValNoMatch-8           409MB/s ± 0%   408MB/s ± 0%     ~     (p=0.151 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-8     292MB/s ± 0%   286MB/s ± 2%     ~     (p=0.056 n=5+5)
BackendBlockTraceQL/resourceAttValNoMatch-8       183MB/s ± 1%   183MB/s ± 1%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/resourceAttIntrinsicMatch-8   679MB/s ± 2%   691MB/s ± 1%   +1.66%  (p=0.032 n=5+5)
BackendBlockTraceQL/mixedValNoMatch-8            14.7MB/s ± 2%  15.2MB/s ± 2%   +3.15%  (p=0.008 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchAnd-8       201MB/s ± 1%   201MB/s ± 1%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchOr-8       59.1MB/s ± 3%  58.8MB/s ± 4%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/count-8                      29.8MB/s ± 3%  29.8MB/s ± 2%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/struct-8                     1.49MB/s ± 4%  1.56MB/s ± 5%     ~     (p=0.087 n=5+5)
BackendBlockTraceQL/||-8                         39.9MB/s ± 2%  39.7MB/s ± 2%     ~     (p=0.548 n=5+5)
BackendBlockTraceQL/mixed-8                       116MB/s ± 2%   116MB/s ± 3%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/spansets-8                   1.54MB/s ± 1%  3.07MB/s ± 3%  +99.74%  (p=0.008 n=5+5)

name                                             old MB_io/op   new MB_io/op   delta
BackendBlockTraceQL/spanAttValNoMatch-8              2.74 ± 0%      2.74 ± 0%     ~     (all equal)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-8        1.69 ± 0%      1.69 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttValNoMatch-8          2.21 ± 0%      2.21 ± 0%     ~     (all equal)
BackendBlockTraceQL/resourceAttIntrinsicMatch-8      16.6 ± 0%      16.6 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValNoMatch-8                4.80 ± 0%      4.80 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValMixedMatchAnd-8          1.16 ± 0%      1.16 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixedValMixedMatchOr-8           19.2 ± 0%      19.2 ± 0%     ~     (all equal)
BackendBlockTraceQL/count-8                          17.2 ± 0%      17.2 ± 0%     ~     (all equal)
BackendBlockTraceQL/struct-8                         1.85 ± 0%      1.85 ± 0%     ~     (all equal)
BackendBlockTraceQL/||-8                             17.7 ± 0%      17.7 ± 0%     ~     (all equal)
BackendBlockTraceQL/mixed-8                          4.36 ± 0%      4.36 ± 0%     ~     (all equal)
BackendBlockTraceQL/spansets-8                       4.69 ± 0%      4.69 ± 0%     ~     (all equal)

name                                             old alloc/op   new alloc/op   delta
BackendBlockTraceQL/spanAttValNoMatch-8            3.99MB ± 1%    4.00MB ± 2%     ~     (p=0.690 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-8      2.97MB ± 2%    2.96MB ± 3%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/resourceAttValNoMatch-8        5.47MB ± 2%    5.43MB ± 2%     ~     (p=0.690 n=5+5)
BackendBlockTraceQL/resourceAttIntrinsicMatch-8    3.07MB ± 5%    3.18MB ± 6%     ~     (p=0.151 n=5+5)
BackendBlockTraceQL/mixedValNoMatch-8              6.70MB ± 6%    6.73MB ± 9%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchAnd-8        2.93MB ± 2%    2.94MB ± 2%     ~     (p=0.690 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchOr-8         6.12MB ±12%    5.84MB ±13%     ~     (p=0.310 n=5+5)
BackendBlockTraceQL/count-8                         275MB ± 2%     275MB ± 2%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/struct-8                       21.3MB ± 0%    22.4MB ±15%     ~     (p=0.857 n=4+4)
BackendBlockTraceQL/||-8                            453MB ± 0%     452MB ± 0%     ~     (p=0.056 n=5+5)
BackendBlockTraceQL/mixed-8                        3.09MB ± 5%    3.05MB ± 8%     ~     (p=0.421 n=5+5)
BackendBlockTraceQL/spansets-8                     1.59GB ± 5%   0.04GB ±105%  -97.31%  (p=0.008 n=5+5)

name                                             old allocs/op  new allocs/op  delta
BackendBlockTraceQL/spanAttValNoMatch-8             44.1k ± 0%     44.1k ± 0%     ~     (p=0.643 n=5+5)
BackendBlockTraceQL/spanAttIntrinsicNoMatch-8       44.0k ± 0%     44.0k ± 0%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/resourceAttValNoMatch-8         44.2k ± 0%     44.2k ± 0%     ~     (p=0.905 n=5+5)
BackendBlockTraceQL/resourceAttIntrinsicMatch-8     45.6k ± 0%     45.6k ± 0%     ~     (p=0.206 n=5+5)
BackendBlockTraceQL/mixedValNoMatch-8               47.2k ± 0%     47.2k ± 0%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchAnd-8         44.1k ± 0%     44.1k ± 0%     ~     (p=0.683 n=5+5)
BackendBlockTraceQL/mixedValMixedMatchOr-8          46.0k ± 0%     46.0k ± 0%     ~     (p=0.841 n=5+5)
BackendBlockTraceQL/count-8                         2.71M ± 0%     2.71M ± 0%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/struct-8                         250k ± 0%      250k ± 0%     ~     (p=0.857 n=4+4)
BackendBlockTraceQL/||-8                            1.12M ± 0%     1.12M ± 0%     ~     (p=1.000 n=5+5)
BackendBlockTraceQL/mixed-8                         44.9k ± 0%     44.9k ± 0%     ~     (p=0.635 n=5+5)
BackendBlockTraceQL/spansets-8                      18.1M ± 1%      0.5M ±21%  -97.43%  (p=0.008 n=5+5)

Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. In the upcoming metrics work I ran into a similar problem of needing to do initialization between parsing and evaluation. Ideally we could compile and validate the regexp before the first evaluation because otherwise it will look like a querier or ingester error (since it isn't caught in the query-frontend). We can't do it during parsing because it's hard (impossible?) to return errors through the YACC snippets. Validate is ok but not quite right. So I added an init() method to some AST elements. Not asking for it here, but aligning some ideas.

Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

We can't do it during parsing because it's hard (impossible?) to return errors through the YACC snippets. Validate is ok but not quite right. So I added an init() method to some AST elements. Not asking for it here, but aligning some ideas.

Yeah, I ran up against these same decisions while making the change. I considered some kind of init() but it seemed like a big change for such a focused improvement. I do like the idea though if we have other places where it's useful.

Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott joe-elliott merged commit 348508b into grafana:main Nov 14, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants