MQE: skip decoding histogram buckets where possible #11587
Conversation
|
💻 Deploy preview deleted. |
|
This PR is pending prometheus/prometheus#16686 being merged and vendored into Mimir. |
0369ebb to
983a970
Compare
The Prometheus PR has been merged into Prometheus and vendored into Mimir, so this PR is ready for review. |
| return | ||
| } | ||
|
|
||
| if dup, ok := node.(*commonsubexpressionelimination.Duplicate); ok { |
There was a problem hiding this comment.
Does this imply that future optimization passes will also need to be done after CSE and will need to be aware of previously applied optimizations?
There was a problem hiding this comment.
I think it's going to depend on the optimisation. I'm really hoping that we can avoid every optimisation knowing about every other optimisation before it. If CSE becomes a common source of problems like here, then I'm thinking we could reverse the order, and move all the special logic into the CSE optimisation pass - this would increase the complexity of the CSE pass, but make all the others simpler, and keep everything CSE-related in one place.
pkg/streamingpromql/config.go
Outdated
| } | ||
|
|
||
| func (o *EngineOpts) RegisterFlags(f *flag.FlagSet) { | ||
| f.BoolVar(&o.EnableCommonSubexpressionElimination, "querier.mimir-query-engine.enable-common-subexpression-elimination", true, "Enable common subexpression elimination when evaluating queries.") | ||
| f.BoolVar(&o.EnableSkippingHistogramDecoding, "querier.mimir-query-engine.enable-skipping-histogram-decoding", true, "Enable skipping histogram decoding when evaluating queries. Only applies if query planner is enabled.") |
There was a problem hiding this comment.
Can this help text be expanded to qualify when and why skipping of decoding is done? Without looking at the code I wouldn't know that we skip decoding histograms as part of a query because it's not needed in some cases.
# Conflicts: # pkg/streamingpromql/optimize/plan/commonsubexpressionelimination/optimization_pass_test.go
|
Given the approval, I'm going to merge this, but if you have any further feedback, please let me know. |
#### What this PR does This PR fixes a broken test introduced by #11587 that did not take into account the changes from #11900. #### Which issue(s) this PR fixes or relates to #11587 #### Checklist - [x] Tests updated. - [n/a] Documentation added. - [n/a] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`. If changelog entry is not needed, please add the `changelog-not-needed` label to the PR. - [n/a] [`about-versioning.md`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features.
What this PR does
This PR implements an optimisation inspired by prometheus/prometheus#14097.
I've implemented this as a MQE optimisation pass (rather than reusing the logic from that Prometheus PR) for two reasons:
SkipHistogramBucketshint.Peak memory consumption is reduced by ~10% in our benchmarks. Latency improves by up to 25% in some cases, but regresses by a small amount (in both absolute and relative terms) for some instant queries:
Which issue(s) this PR fixes or relates to
#10067
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.