Reduce memory allocations in functions used to propagate contextual information between gRPC calls #7529
Conversation
charleskorn
left a comment
charleskorn
left a comment
There was a problem hiding this comment.
Is there ever a case where we'd want to use metadata.FromIncomingContext()? I'm wondering if we should add a linting rule to avoid re-introducing this in the future.
|
i was going to rebase and merge this, but Charles' comment makes sense, i'll leave this open |
|
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
…ServerStreamInterceptor()
goos: darwin
goarch: amd64
pkg: github.com/grafana/mimir/pkg/querier/api
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
ReadConsistencyServerUnaryInterceptor/with_read_consistency:_true-12 935.7n ± 2% 127.6n ± 1% -86.36% (p=0.002 n=6)
ReadConsistencyServerUnaryInterceptor/with_read_consistency:_false-12 740.2n ± 2% 115.2n ± 2% -84.43% (p=0.002 n=6)
ReadConsistencyServerStreamInterceptor/with_read_consistency:_true-12 1025.5n ± 3% 205.8n ± 3% -79.94% (p=0.002 n=6)
ReadConsistencyServerStreamInterceptor/with_read_consistency:_false-12 783.1n ± 1% 159.7n ± 2% -79.61% (p=0.002 n=6)
geomean 863.6n 148.3n -82.83%
│ before.txt │ after.txt │
│ B/op │ B/op vs base │
ReadConsistencyServerUnaryInterceptor/with_read_consistency:_true-12 576.00 ± 0% 80.00 ± 0% -86.11% (p=0.002 n=6)
ReadConsistencyServerUnaryInterceptor/with_read_consistency:_false-12 496.0 ± 0% 0.0 ± 0% -100.00% (p=0.002 n=6)
ReadConsistencyServerStreamInterceptor/with_read_consistency:_true-12 640.0 ± 0% 144.0 ± 0% -77.50% (p=0.002 n=6)
ReadConsistencyServerStreamInterceptor/with_read_consistency:_false-12 528.00 ± 0% 32.00 ± 0% -93.94% (p=0.002 n=6)
geomean 557.4 ? ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean
│ before.txt │ after.txt │
│ allocs/op │ allocs/op vs base │
ReadConsistencyServerUnaryInterceptor/with_read_consistency:_true-12 11.000 ± 0% 3.000 ± 0% -72.73% (p=0.002 n=6)
ReadConsistencyServerUnaryInterceptor/with_read_consistency:_false-12 8.000 ± 0% 0.000 ± 0% -100.00% (p=0.002 n=6)
ReadConsistencyServerStreamInterceptor/with_read_consistency:_true-12 13.000 ± 0% 5.000 ± 0% -61.54% (p=0.002 n=6)
ReadConsistencyServerStreamInterceptor/with_read_consistency:_false-12 9.000 ± 0% 1.000 ± 0% -88.89% (p=0.002 n=6)
geomean 10.07 ? ¹ ²
¹ summaries must be >0 to compute geomean
² ratios must be >0 to compute geomean
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
optimize-FromIncomingContext
branch
from
03751d1 to
a856384
Compare
Signed-off-by: Marco Pracucci <[email protected]>
Good idea. Done in b6b64c6, where I've spot another place where we can use the |
Actually that function is unused and will get removed in #7530, which I will merge after this PR. |
6 participants
What this PR does
In Mimir ingesters, 10% of object (and memory) allocations come from gRPC
metadata.FromIncomingContext().metadata.FromIncomingContext()is called in few places. The problem ofmetadata.FromIncomingContext()is that it creates a copy of all metadata map, when we only want to lookup a single key.Two years ago,
metadata.ValueFromIncomingContext()was introduced in gRPC golang library exactly for this reason. The function is still marked as experimental but it was left untouched since was introduced 2 years ago. I propose to use it. If it will get removed, we can always revert back tometadata.FromIncomingContext().I've done the same in grafana/dskit#502, which I'm vendoring in this PR.
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.