[aggregator] Avoid large copies in entry rollup comparisons by making them more inline-friendly #3195
Conversation
| if val.key.Equal(k) { | ||
| // keep in sync with aggregationKey.Equal() | ||
| // this is >2x slower if not inlined manually. | ||
| for i := range vals { |
There was a problem hiding this comment.
Hmm dunno if this is possible, but any way to add a fuzz test comparing behavior using equals vs using the manual inline here? Less to test the current version of the code than to ensure against future regressions haha.
There was a problem hiding this comment.
Also, maybe worth adding a pointer from Equal to here to make sure anyone changing that method also changes this one?
Makes one wish for macros...
There was a problem hiding this comment.
good point as I was just lazy, but then again I missed something in tests :D
| for i, k := range agg.byKey { | ||
| if k.key.Equal(key) { | ||
| for i := range agg.byKey { | ||
| if agg.byKey[i].key.Equal(key) { |
There was a problem hiding this comment.
Dumb question: this is faster? I'm surprised haha--more inlining issues?
There was a problem hiding this comment.
stack copies due to how range works
| if u.Type != other.Type { | ||
| return false | ||
| } | ||
| switch u.Type { | ||
| case pipeline.TransformationOpType: |
There was a problem hiding this comment.
The definition is type TransformationOp struct{Type transformation.Type}, where transformation.Type is plain int
| case pipeline.RollupOpType: | ||
| clone := OpUnion{ | ||
| Type: u.Type, | ||
| Transformation: u.Transformation, |
There was a problem hiding this comment.
does this need a clone as well? if so, two ifs is probably going to put you back in switch land branch wise
There was a problem hiding this comment.
nope, copying an int > copying the same single int conditionally
src/metrics/pipeline/applied/type.go
Outdated
| } | ||
|
|
||
| if p.operations[i].Type == pipeline.RollupOpType && | ||
| !p.operations[i].Rollup.Equal(other.operations[i].Rollup) { |
There was a problem hiding this comment.
do we need to worry about the transform case?
There was a problem hiding this comment.
yes, my bad, missed it
Codecov Report
@@ Coverage Diff @@
## master #3195 +/- ##
=========================================
- Coverage 72.3% 72.3% -0.1%
=========================================
Files 1087 1087
Lines 100729 100736 +7
=========================================
- Hits 72875 72867 -8
- Misses 22798 22807 +9
- Partials 5056 5062 +6
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
* master: (30 commits) [dbnode] Use go context to cancel index query workers after timeout (#3194) [aggregator] Fix change ActivePlacement semantics on close (#3201) [aggregator] Simplify (Active)StagedPlacement API (#3199) [aggregator] Checking if metadata is set to default should not cause copying (#3198) [dbnode] Remove readers and writer from aggregator API (#3122) [aggregator] Avoid large copies in entry rollup comparisons by making them more inline-friendly (#3195) [dbnode] Re-add aggregator doc limit update (#3137) [m3db] Do not close reader in filterFieldsIterator.Close() (#3196) Revert "Remove disk series read limit (#3174)" (#3193) [instrument] Improve sampled timer and stopwatch performance (#3191) Omit unset fields in metadata json (#3189) [dbnode] Remove left-over code in storage/bootstrap/bootstrapper (#3190) [dbnode][coordinator] Support match[] in label endpoints (#3180) Instrument the worker pool with the wait time (#3188) Instrument query path (#3182) [aggregator] Remove indirection, large copy from unaggregated protobuf decoder (#3186) [aggregator] Sample timers completely (#3184) [aggregator] Reduce error handling overhead in rawtcp server (#3183) [aggregator] Move shardID calculation out of critical section (#3179) Move instrumentation cleanup to FetchTaggedResultIterator Close() (#3173) ...
What this PR does / why we need it:
Turns out Go was unable to inline most of the comparison logic, adding function call and copy overhead for metadata:
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?: