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 PromQL parser performance by making it non-concurrent #6356

Merged

Conversation

@slrtbtfs
Copy link
Contributor

slrtbtfs commented Nov 21, 2019

Before this commit, the PromQL parser ran in two goroutines:

  • The lexer goroutine that splits the input into tokens and sent them over a channel to
  • the parser goroutine which produces the abstract syntax tree

The Problem with this approach is that the parser spends more time on goroutine creation
and synchronisation than on actual parsing.

This commit removes that concurrency and replaces the channel by a slice based buffer.

Benchmarks show that this makes the parser up to 7 times faster than before.

Signed-off-by: Tobias Guggenmos tguggenm@redhat.com

Benchmark results (based on #6355): https://gist.github.com/slrtbtfs/24a4d92de43ad4ae871714840dab2d76

Before this commit, the PromQL parser ran in two goroutines:
* The lexer goroutine that splits the input into tokens and sent them over a channel to
* the parser goroutine which produces the abstract syntax tree

The Problem with this approach is that the parser spends more time on goroutine creation
and syncronisation than on actual parsing.

This commit removes that concurrency and replaces the channel by a slice based buffer.

Benchmarks show that this makes the up to 7 times faster than before.

Signed-off-by: Tobias Guggenmos <tguggenm@redhat.com>
@brian-brazil brian-brazil merged commit ac3932e into prometheus:master Nov 21, 2019
5 of 6 checks passed
5 of 6 checks passed
ci/circleci: build CircleCI is running your tests
Details
DCO DCO
Details
ci/circleci: fuzzit_regression Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/prometheus-react/deploy-preview Deploy preview ready!
Details
@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Nov 21, 2019

Thanks!

I didn't even know we were doing this, it doesn't make sense to me at all.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Nov 21, 2019

Yeah, I think it was based on Rob Pike's goroutine+channel-based lexer+parser design (https://www.youtube.com/watch?v=HxaD_trXwRE), which didn't really care much about performance. Not that it matters too much here either way given how little time is spent in this code path in general.

@slrtbtfs

This comment has been minimized.

Copy link
Contributor Author

slrtbtfs commented Nov 21, 2019

Thanks for the quick merge and the pointer to the talk.

Not that it matters too much here either way given how little time is spent in this code path in general.

For Prometheus itself it doesn't. When just using the normal PromQL engine benchmarks this gets indeed lost in the noise.

However, for the language server, when working with large YAML files with thousands of PromQL expressions, the parser performance starts to matter.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented Nov 21, 2019

That's a good point. And generally totally for getting rid of this weird artefact :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.