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

Improve logql parser allocations. #2927

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Conversation

cyriltovena
Copy link
Contributor

Also moving labels parsing into logql package, and I'm not shy to say that promql is faster for this.
So we're using promql for parser, it's faster because their lexer is more sophisticated than ours.

Result are impressive.

❯ benchcmp before.txt after.txt
benchmark                    old ns/op     new ns/op     delta
Benchmark_ParseLabels-16     24394         8745          -64.15%

benchmark                    old allocs     new allocs     delta
Benchmark_ParseLabels-16     156            20             -87.18%

benchmark                    old bytes     new bytes     delta
Benchmark_ParseLabels-16     24976         2099          -91.60%

Combined with #2926, I think we should be able to get rid of util.ToClientLabels. But that's for another PR.

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

Also moving labels parsing into logql package, and I'm not shy to say that promql is faster for this.
So we're using promql for parser, it's faster because their lexer is more sophisticated than ours.

Result are impressive.

```
❯ benchcmp before.txt after.txt
benchmark                    old ns/op     new ns/op     delta
Benchmark_ParseLabels-16     24394         8745          -64.15%

benchmark                    old allocs     new allocs     delta
Benchmark_ParseLabels-16     156            20             -87.18%

benchmark                    old bytes     new bytes     delta
Benchmark_ParseLabels-16     24976         2099          -91.60%
```

Combined with grafana#2926, I think we should be able to get rid of `util.ToClientLabels`. But that's for another PR.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena added this to the 2.1 milestone Nov 13, 2020
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #2927 (7cead0e) into master (f0d4adc) will increase coverage by 0.01%.
The diff coverage is 77.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2927      +/-   ##
==========================================
+ Coverage   61.72%   61.74%   +0.01%     
==========================================
  Files         181      181              
  Lines       14724    14721       -3     
==========================================
+ Hits         9089     9090       +1     
+ Misses       4801     4799       -2     
+ Partials      834      832       -2     
Impacted Files Coverage Δ
pkg/logql/engine.go 87.50% <0.00%> (ø)
pkg/util/conv.go 50.00% <0.00%> (+26.47%) ⬆️
pkg/logql/parser.go 74.07% <80.00%> (-3.43%) ⬇️
pkg/logql/lex.go 90.90% <88.88%> (+0.16%) ⬆️
pkg/ingester/tailer.go 40.94% <100.00%> (ø)
pkg/logql/range_vector.go 97.40% <100.00%> (ø)
pkg/logql/test_utils.go 81.75% <100.00%> (ø)
pkg/promtail/targets/lokipush/pushtarget.go 54.05% <100.00%> (-0.62%) ⬇️
pkg/promtail/positions/positions.go 46.80% <0.00%> (-11.71%) ⬇️
pkg/querier/queryrange/downstreamer.go 97.64% <0.00%> (+2.35%) ⬆️

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

I am not very much aware of lexer and parser code but I tried to connect the dots and it LGTM.
I am okay to let this merge because you know this code very well or you can let someone more familiar than me with lexer and parser do a review of it.

Nice job!

@cyriltovena cyriltovena merged commit 049c7ac into grafana:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants