[aggregator] more efficiently reference previously consumed values#3841
[aggregator] more efficiently reference previously consumed values#3841rallen090 wants to merge 6 commits into
Conversation
rallen090
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3841 +/- ##
========================================
- Coverage 57.0% 56.8% -0.2%
========================================
Files 552 552
Lines 63129 63123 -6
========================================
- Hits 36033 35906 -127
- Misses 23907 24018 +111
- Partials 3189 3199 +10
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| require.Equal(t, 0, len(*forwardRes)) | ||
| require.Equal(t, 0, len(*onForwardedFlushedRes)) | ||
| require.Equal(t, 1, len(e.values)) | ||
| require.Equal(t, 2, len(e.values)) |
There was a problem hiding this comment.
Why is this changing, shouldn't we still be consuming a value?
There was a problem hiding this comment.
Ah ok so the first one should have onConsumeExpired set on it, worth checking that here?
| timeNanos, | ||
| prevTimeNanos, | ||
| e.toConsume[i].lockedAgg, | ||
| e.toConsume[i], |
There was a problem hiding this comment.
nit: fix the comment if a previous timestamps was dirty to if a previous timestamp was dirty
| @@ -118,11 +117,14 @@ type timedAggregation struct { | |||
| startAtNanos int64 // start time of an aggregation window | |||
| lockedAgg *lockedAggregation | |||
| onConsumeExpired bool | |||
There was a problem hiding this comment.
move this to the bottom of the struct to avoid 7 bytes of padding
3 participants
What this PR does / why we need it:
Improve a CPU inefficiency in the aggregator consume path.
On any aggregate
Consume(t timestamp)call, identifying the timestamp directly precedingtrequired a full scan of a map of all data points in that aggregate's buffer past (seepreviousTimestamp(...)). We already have ordered datapoints in memory, and so now use that to identify a preceding datapoint instead of this extraneous map.Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: