MQE: initial implementation of common subexpression elimination #11189
Conversation
charleskorn
force-pushed
the
charleskorn/mqe-common-subexpression-elimination
branch
6 times, most recently
from
18942bc to
3fda6ae
Compare
charleskorn
force-pushed
the
charleskorn/mqe-common-subexpression-elimination
branch
from
3fda6ae to
1a81f9e
Compare
| } | ||
|
|
||
| func (d *Duplicate) Describe() string { | ||
| return "" |
There was a problem hiding this comment.
No - there's nothing to describe about this node (compare this with a node representing a function call where we'd include the function name, for example).
pkg/streamingpromql/optimize/plan/commonsubexpressionelimination/operator.go
Outdated
Show resolved
Hide resolved
| return metadata, nil | ||
| } | ||
|
|
||
| // Return a copy of the original series metadata. |
There was a problem hiding this comment.
Will it be possible in the future to determine if a downstream operator will mutate the metadata or not. And if it will not we can avoid copying it?
If so, perhaps a comment here for now about a future optimisation?
There was a problem hiding this comment.
Anything's possible 🙂 But I don't think avoiding the copy here will have a huge impact compared to the cost of evaluating the rest of the query, so I'm not sure it's worth the added complexity.
pkg/streamingpromql/optimize/plan/commonsubexpressionelimination/operator.go
Outdated
Show resolved
Hide resolved
|
|
||
| func NewOptimizationPass(reg prometheus.Registerer) *OptimizationPass { | ||
| return &OptimizationPass{ | ||
| duplicationNodesIntroduced: promauto.With(reg).NewCounter(prometheus.CounterOpts{ |
There was a problem hiding this comment.
what if other optimisations want to use the duplication node? Wouldn't the metric be better off held on the duplication node and then a label used to determine the source?
There was a problem hiding this comment.
For me, this metric is about evaluating the impact of this optimization pass, rather than the node. So if we end up using the node for other optimization passes, I'd expect that optimization pass to have its own metrics.
pkg/streamingpromql/optimize/plan/commonsubexpressionelimination/series_data_ring_buffer.go
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| func (b *SeriesDataRingBuffer) Remove(seriesIndex int) types.InstantVectorSeriesData { | ||
| if seriesIndex != b.firstSeriesIndex { |
There was a problem hiding this comment.
(nit) why not just always remove b.firstSeriesIndex (ie why have this and not just RemoveFirst)? Or is this a check to make sure the caller knows the index it wants deleted is correct?
There was a problem hiding this comment.
Or is this a check to make sure the caller knows the index it wants deleted is correct?
This - if we had a bug where the positions of each consumer were wrong, I'd want this to fail rather than quietly return incorrect results.
… query planning is enabled
…thout query planner enabled
…pression can be reached from different leaf nodes
…rrectly deduplicated
…hen no longer needed
…orBenchmarkQueries`
charleskorn
force-pushed
the
charleskorn/mqe-common-subexpression-elimination
branch
from
052def7 to
8e35d43
Compare
What this PR does
This PR implements common subexpression elimination in MQE.
Prior to this PR, when given an expression like
sum(foo) / (sum(foo) + sum(bar)), MQE would evaluatesum(foo)twice. This is not necessary: we can instead computesum(foo)once and use the result in both places, and that's what this PR implements.This improves overall query performance, and also saves CPU time in queriers, ingesters and store-gateways. However, the drawback is that queriers must buffer unused parts of common results until they are consumed by all places that need them. For queries where the size of the common result is small, this is not an issue, but in other cases this can increase the peak memory consumption of queries significantly.
This could be addressed somewhat by consuming from both sides of binary operations concurrently, but this is out of scope for this PR - for now, given the substantial performance improvements and relative cost of CPU and memory, we can provision more memory for queriers if needed. In any case, query memory consumption will be no worse than Prometheus' engine.
Benchmark results
Overall, latency improves roughly in line with the ratio of eliminated selectors to original selectors. (For example, if there were two selectors originally, and they're both the same, then latency improves around 50%, or if there were three selectors and two are the same, then latency improves around 33%.)
Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.