[query] Increase performance of temporal functions #1917
Conversation
src/query/functions/temporal/base.go
Outdated
| rBound time.Time, | ||
| init int, | ||
| ) (int, int, bool) { | ||
| if init > len(dp) || init < 0 { |
There was a problem hiding this comment.
hm should this be init >= len(dp)?
| } | ||
|
|
||
| if r == -1 { | ||
| r = len(dp) |
There was a problem hiding this comment.
Is this not r = len(dp)-1? Are the callers going to directly index into the slice?
There was a problem hiding this comment.
Still need to update this or is this different now?
There was a problem hiding this comment.
Yeah sorry meant to respond on here; it's used as subslice indices, which should work fine. Added a comment noting this
src/query/functions/temporal/base.go
Outdated
| @@ -349,7 +394,7 @@ func (c *baseNode) processSingleRequest( | |||
|
|
|||
| aggDuration := c.op.duration | |||
| steps := int((aggDuration + bounds.Duration) / bounds.StepSize) | |||
| values := make([]ts.Datapoints, 0, steps) | |||
| values := make(ts.Datapoints, 0, steps) | |||
There was a problem hiding this comment.
Seems like steps doesn't take into account the dep iters? Perhaps we could get the full running count.
e.g. just thinking out loud:
depExpectedDatapoints := 0
for i, blk := range request.deps {
// ...
depIters[i] = iter
depExpectedDatapoints += len(iter.Datapoints())
}
// ...
values := make(ts.Datapoints, 0, depExpectedDatapoints + steps)Also at this point do we know how big DatapointsAtStep(x) will be? Do we need to multiply steps * expectedDatapointsAtStep or something like that?
There was a problem hiding this comment.
Also can values be reused between calls to node.processSingleRequest(...)? Or does processor.process(values[l:r], ...) end up taking ownership of the slice?
src/query/functions/temporal/base.go
Outdated
| steps := int((aggDuration + bounds.Duration) / bounds.StepSize) | ||
| values := make([]ts.Datapoints, 0, steps) | ||
| desiredLength := int(math.Ceil(float64(aggDuration) / float64(bounds.StepSize))) | ||
| depIters := make([]block.UnconsolidatedSeriesIter, len(request.deps)) |
There was a problem hiding this comment.
For safety, should we just use make([]foo, 0, length) and append(...) wherever it's possible to? Been running into a lot of these bugs lately (in not just M3 code base, elsewhere too).
Codecov Report
@@ Coverage Diff @@
## master #1917 +/- ##
=========================================
- Coverage 63.4% 63.4% -0.1%
=========================================
Files 1113 1112 -1
Lines 104747 104797 +50
=========================================
+ Hits 66428 66455 +27
- Misses 34077 34108 +31
+ Partials 4242 4234 -8
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1917 +/- ##
=========================================
- Coverage 63.4% 63.4% -0.1%
=========================================
Files 1113 1112 -1
Lines 104747 104797 +50
=========================================
+ Hits 66428 66455 +27
- Misses 34077 34108 +31
+ Partials 4242 4234 -8
Continue to review full report at Codecov.
|
arnikola
changed the title
What this PR does / why we need it:
FIxes very slow temporal function application when working with larger ranges
Special notes for your reviewer:
This is a quick and easy solution for the time being, just removing the allocations made in a tight loop.
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: