metadata: replace added [][]string with O(1) delta linked list - #9129
metadata: replace added [][]string with O(1) delta linked list#9129notandruu wants to merge 5 commits into
Conversation
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9129 +/- ##
==========================================
- Coverage 83.20% 83.17% -0.04%
==========================================
Files 414 419 +5
Lines 33489 33868 +379
==========================================
+ Hits 27865 28170 +305
- Misses 4214 4274 +60
- Partials 1410 1424 +14
🚀 New features to boost your workflow:
|
|
Could a maintainer add a |
|
@notandruu Could you please sign the CLA for us to proceed with the review. |
easwars
added
the
Type: Performance
|
CLA is now signed. The EasyCLA bot should show a green check. Let me know if anything else is needed. |
|
Hi @easwars — the EasyCLA check is now green. The remaining |
|
/gemini review |
|
Moving to @arjan-bal for initial review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes metadata handling by replacing the slice-of-slices implementation of appended metadata ([][]string) with a singly-linked list (deltaKV). This changes sequential AppendToOutgoingContext calls from O(N²) to O(N) total complexity. New benchmarks have also been added to measure these performance improvements. The review feedback highlights three optimization and compatibility opportunities: 1) adding an early nil check in fromOutgoingContextRaw to preserve backward compatibility and avoid empty slice allocations, 2) using a stack-allocated array to back the nodes slice in FromOutgoingContext to prevent heap allocations, and 3) pre-allocating kvCopy to its exact size in AppendToOutgoingContext to avoid slice growth overhead.
7f6f7d3 to
d83e023
Compare
|
Applied all three suggestions from the Gemini review in the latest push:
|
d83e023 to
e2c43a3
Compare
|
Re: magic number in Re: |
|
It's safe to change |
AppendToOutgoingContext was O(N) per call: it allocated a new [][]string
of length N+1 and copied all N prior slices into it on every append.
A context with N sequential appends therefore cost O(N²) in total
allocation/copy work.
Replace the flat slice with a singly-linked list of deltaKV nodes.
Each AppendToOutgoingContext call allocates exactly one node and one
flattened kv slice — O(1) regardless of chain depth.
FromOutgoingContext and fromOutgoingContextRaw collect the chain
(newest-first) into a temporary slice, then iterate in reverse to
preserve FIFO ordering, keeping the read path O(N) while maintaining
backward-compatible return types for internal callers.
Benchmark results on Apple M3 Max (go1.26.3):
BenchmarkAppendToOutgoingContext (accumulating context, num=10):
before: 3,263,813 ns/op (quadratic growth, chain length ≈ 10·N)
after: 1,646 ns/op (1982× faster)
BenchmarkFromOutgoingContext:
before: 544.3 ns/op
after: 212.4 ns/op (2.6× faster)
BenchmarkAppendToOutgoingContextN (fresh context, N appends):
N= 1: 87 ns/op, 160 B/op, 4 allocs
N= 5: 446 ns/op, 800 B/op, 20 allocs
N=10: 872 ns/op, 1600 B/op, 40 allocs
N=50: 4373 ns/op, 8000 B/op, 200 allocs
(linear: ~87·N ns/op confirmed)
Fixes grpc#8860
Signed-off-by: Andrew Liu <andrewjliu22@gmail.com>
e2c43a3 to
a9619fa
Compare
|
Done in the latest push. Changed |
|
Fixed: the CI panic was a nil pointer dereference at Fixed in the latest push by returning a package-level |
|
Fixed: renamed |
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
For a more direct and comprehensive comparison, can you run the new benchmarks on your branch and on master?
git checkout master
go test ./metadata/ -bench="BenchmarkAppendToOutgoingContextN|BenchmarkFromOutgoingContextN" -benchmem -cpu=2 -count=5 2>&1 | tee master.txt
git checkout metadata-outgoing-context-linked-list
go test ./metadata/ -bench="BenchmarkAppendToOutgoingContextN|BenchmarkFromOutgoingContextN" -benchmem -cpu=2 -count=5 2>&1 | tee new.txt
benchstat master.txt new.txtSince we're modifying fromOutgoingContextRaw, could you add a benchmark for it as well?
- Remove benchmark for old context-manipulation approach; reuse BenchmarkAppendToOutgoingContext name for N-sub-benchmark version using b.Loop() on a fresh context per iteration (fixes skewed measurements from accumulating context across b.N iterations) - Remove BenchmarkFromOutgoingContext; rename BenchmarkFromOutgoingContextN to BenchmarkFromOutgoingContext using b.Loop() - Add BenchmarkFromOutgoingContextRaw - Remove 'old vs new' language from benchmark comments - Use context.Background() directly (no timeout overhead) - FromOutgoingContext: use make(MD, len(raw.md)) to avoid over-allocating when delta pairs contain duplicate keys
|
All feedback addressed in the latest push. Here are the benchmark results on an Apple M3 Max (
O(1) per append, O(N) total — cost scales linearly as expected.
Changes summary:
|
|
Friendly ping @arjan-bal — all the review feedback from the last pass is addressed (the |
|
Sorry for the delay! I've been tied up recently, but I'll do another pass by tomorrow. |
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
I checked out the test file on the master branch to run the same benchamarks against master:
git checkout master
git checkout <feature-branch-commit-sha> metadata/metadata_test.goTo run the test:
go test ./metadata/ -bench="Benchmark" -benchmem -cpu=2 -count=10 2>&1 | tee old.txtTo compare the results of master against the feature branch:
benchstat old.txt new.txt
The results are as follows:
goos: linux
goarch: amd64
pkg: google.golang.org/grpc/metadata
cpu: Intel(R) Xeon(R) CPU @ 2.60GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
_AddingMetadata_ContextManipulationApproach-2 11.71µ ± 3% 11.56µ ± 5% ~ (p=0.481 n=10)
AppendToOutgoingContext/1-2 182.1n ± 7% 175.2n ± 4% -3.82% (p=0.001 n=10)
AppendToOutgoingContext/5-2 1093.0n ± 2% 868.4n ± 2% -20.55% (p=0.000 n=10)
AppendToOutgoingContext/10-2 2.512µ ± 2% 1.739µ ± 2% -30.76% (p=0.000 n=10)
AppendToOutgoingContext/50-2 26.493µ ± 3% 8.644µ ± 2% -67.37% (p=0.000 n=10)
FromOutgoingContext/1-2 398.9n ± 4% 369.6n ± 7% -7.36% (p=0.003 n=10)
FromOutgoingContext/5-2 645.6n ± 4% 643.5n ± 7% ~ (p=1.000 n=10)
FromOutgoingContext/10-2 1.218µ ± 3% 1.498µ ± 2% +22.95% (p=0.000 n=10)
FromOutgoingContext/50-2 4.519µ ± 4% 6.964µ ± 6% +54.09% (p=0.000 n=10)
FromOutgoingContextRaw/1-2 6.673n ± 2% 107.500n ± 3% +1511.09% (p=0.000 n=10)
FromOutgoingContextRaw/5-2 9.911n ± 2% 154.950n ± 6% +1463.49% (p=0.000 n=10)
FromOutgoingContextRaw/10-2 9.816n ± 2% 202.250n ± 2% +1960.31% (p=0.000 n=10)
FromOutgoingContextRaw/50-2 22.20n ± 2% 596.15n ± 2% +2585.97% (p=0.000 n=10)
FromIncomingContext-2 326.2n ± 3% 294.1n ± 5% -9.84% (p=0.000 n=10)
ValueFromIncomingContext/key-found-2 45.26n ± 3% 44.69n ± 3% ~ (p=0.172 n=10)
ValueFromIncomingContext/key-not-found-2 66.12n ± 1% 66.83n ± 2% ~ (p=0.065 n=10)
geomean 293.9n 567.5n +93.10%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
_AddingMetadata_ContextManipulationApproach-2 9.530Ki ± 0% 9.374Ki ± 0% -1.64% (p=0.000 n=10)
AppendToOutgoingContext/1-2 168.0 ± 0% 160.0 ± 0% -4.76% (p=0.000 n=10)
AppendToOutgoingContext/5-2 1096.0 ± 0% 800.0 ± 0% -27.01% (p=0.000 n=10)
AppendToOutgoingContext/10-2 2.727Ki ± 0% 1.562Ki ± 0% -42.69% (p=0.000 n=10)
AppendToOutgoingContext/50-2 38.789Ki ± 0% 7.812Ki ± 0% -79.86% (p=0.000 n=10)
FromOutgoingContext/1-2 432.0 ± 0% 432.0 ± 0% ~ (p=1.000 n=10) ¹
FromOutgoingContext/5-2 496.0 ± 0% 544.0 ± 0% +9.68% (p=0.000 n=10)
FromOutgoingContext/10-2 968.0 ± 0% 1400.0 ± 0% +44.63% (p=0.000 n=10)
FromOutgoingContext/50-2 3.508Ki ± 0% 6.383Ki ± 0% +81.96% (p=0.000 n=10)
FromOutgoingContextRaw/1-2 0.00 ± 0% 64.00 ± 0% ? (p=0.000 n=10)
FromOutgoingContextRaw/5-2 0.0 ± 0% 104.0 ± 0% ? (p=0.000 n=10)
FromOutgoingContextRaw/10-2 0.0 ± 0% 136.0 ± 0% ? (p=0.000 n=10)
FromOutgoingContextRaw/50-2 0.0 ± 0% 472.0 ± 0% ? (p=0.000 n=10)
FromIncomingContext-2 416.0 ± 0% 416.0 ± 0% ~ (p=1.000 n=10) ¹
ValueFromIncomingContext/key-found-2 16.00 ± 0% 16.00 ± 0% ~ (p=1.000 n=10) ¹
ValueFromIncomingContext/key-not-found-2 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
geomean ² ? ²
¹ all samples are equal
² summaries must be >0 to compute geomean
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
_AddingMetadata_ContextManipulationApproach-2 104.0 ± 0% 104.0 ± 0% ~ (p=1.000 n=10) ¹
AppendToOutgoingContext/1-2 4.000 ± 0% 4.000 ± 0% ~ (p=1.000 n=10) ¹
AppendToOutgoingContext/5-2 20.00 ± 0% 20.00 ± 0% ~ (p=1.000 n=10) ¹
AppendToOutgoingContext/10-2 40.00 ± 0% 40.00 ± 0% ~ (p=1.000 n=10) ¹
AppendToOutgoingContext/50-2 200.0 ± 0% 200.0 ± 0% ~ (p=1.000 n=10) ¹
FromOutgoingContext/1-2 4.000 ± 0% 4.000 ± 0% ~ (p=1.000 n=10) ¹
FromOutgoingContext/5-2 8.000 ± 0% 9.000 ± 0% +12.50% (p=0.000 n=10)
FromOutgoingContext/10-2 15.00 ± 0% 17.00 ± 0% +13.33% (p=0.000 n=10)
FromOutgoingContext/50-2 55.00 ± 0% 61.00 ± 0% +10.91% (p=0.000 n=10)
FromOutgoingContextRaw/1-2 0.000 ± 0% 4.000 ± 0% ? (p=0.000 n=10)
FromOutgoingContextRaw/5-2 0.000 ± 0% 4.000 ± 0% ? (p=0.000 n=10)
FromOutgoingContextRaw/10-2 0.000 ± 0% 4.000 ± 0% ? (p=0.000 n=10)
FromOutgoingContextRaw/50-2 0.000 ± 0% 4.000 ± 0% ? (p=0.000 n=10)
FromIncomingContext-2 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹
ValueFromIncomingContext/key-found-2 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹
ValueFromIncomingContext/key-not-found-2 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
geomean ² ? ²
¹ all samples are equal
² summaries must be >0 to compute geomean
It looks like we've improved the write performance (AppendXXX) while regressing the read functions (FromOutgoingContextXXX). I think we need to spent time trying to close the gap in the read functions. I'll try to spend some time analysing this in the coming days.
I wrote a benchmark to measure the combined performance of multiple appends followed by a single FromOutgoingContextRaw call:
func BenchmarkAppendAndFromOutgoingContextRaw(b *testing.B) {
for _, n := range []int{1, 5, 10, 50} {
b.Run(strconv.Itoa(n), func(b *testing.B) {
b.ReportAllocs()
for b.Loop() {
ctx := NewOutgoingContext(context.Background(), MD{"k-base": {"v-base"}})
for i := 0; i < n; i++ {
ctx = AppendToOutgoingContext(ctx, "k"+strconv.Itoa(i), "v"+strconv.Itoa(i))
}
_, vals, _ := fromOutgoingContextRaw(ctx)
for range vals {
// This empty loop is safe from DCE
// because b.Loop() protects it!
}
}
})
}
}Results indicate that with 5 values the gains in write performance outweigh the regression in reads.
goos: linux
goarch: amd64
pkg: google.golang.org/grpc/metadata
cpu: Intel(R) Xeon(R) CPU @ 2.60GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
AppendAndFromOutgoingContextRaw/1-2 516.0n ± 1% 604.1n ± 2% +17.07% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/5-2 1.557µ ± 2% 1.477µ ± 2% -5.17% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/10-2 2.903µ ± 7% 2.509µ ± 2% -13.57% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/50-2 23.06µ ± 7% 11.18µ ± 4% -51.51% (p=0.000 n=10)
geomean 2.708µ 2.237µ -17.41%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
AppendAndFromOutgoingContextRaw/1-2 636.0 ± 0% 680.0 ± 0% +6.92% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/5-2 1.418Ki ± 0% 1.219Ki ± 0% -14.05% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/10-2 2.938Ki ± 0% 1.891Ki ± 0% -35.64% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/50-2 38.000Ki ± 0% 7.469Ki ± 0% -80.35% (p=0.000 n=10)
geomean 3.149Ki 1.839Ki -41.61%
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
AppendAndFromOutgoingContextRaw/1-2 11.00 ± 0% 15.00 ± 0% +36.36% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/5-2 35.00 ± 0% 39.00 ± 0% +11.43% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/10-2 65.00 ± 0% 69.00 ± 0% +6.15% (p=0.000 n=10)
AppendAndFromOutgoingContextRaw/50-2 305.0 ± 0% 309.0 ± 0% +1.31% (p=0.000 n=10)
geomean 52.56 59.43 +13.06%
The raw path returns an iter.Seq2; calling it without consuming measured only setup. Drain it inside b.Loop() so the iteration cost is included.
|
Fixed the benchmark to drain the lazy iterator ( Here is the branch-vs-master comparison you asked for, M3 Max, go1.26.3, BenchmarkAppendToOutgoingContext (N sequential appends, the path this PR optimizes):
At N=50 that is ~2x faster and ~5x less memory; the linked-list delta avoids the O(N^2) re-copy of the BenchmarkFromOutgoingContext (public, materializing read):
I want to be upfront: the materializing read regresses at high N (~1.6x slower, ~1.8x more memory at N=50), because it now walks the linked list and reverses before building the map. BenchmarkFromOutgoingContextRaw (branch only, the lazy iterator the transport actually uses on the send path,
So the per-RPC hot read path (raw iterator) is cheap and flat at 4 allocs, and only the legacy materializing |
|
Friendly ping @arjan-bal — it's been about 9 days past when you said you'd take another look. Happy to make any further changes, just let me know. |
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
I've suggested a few changes to avoid an extra pass over the linked list and eliminate a heap allocation for most real-world use cases. This should narrow the performance gap in FromOutgoingContext compared to master.
Ultimately, I don't believe the remaining regression in FromOutgoingContext will be an issue in practice because:
- Most users interleave
FromOutgoingContextandAppendToOutgoingContext. The gains inAppendToOutgoingContextshould yield a net improvement in overall performance. - If users are performing a disproportionate number of
FromOutgoingContextcalls, we can introduce an API similar toValueFromIncomingContextto fetch a single metadata entry, which would be much more efficient.
Thanks for the great work!
| // Count nodes to pre-allocate the reversal slice. | ||
| n := 0 | ||
| for d := raw.added; d != nil; d = d.prev { | ||
| n++ | ||
| } | ||
| // Collect newest-first; the iterator yields oldest-first (FIFO order). | ||
| nodes := make([]*deltaKV, 0, n) | ||
| for d := raw.added; d != nil; d = d.prev { | ||
| nodes = append(nodes, d) | ||
| } | ||
| return raw.md, func(yield func(string, string) bool) { | ||
| for i := len(nodes) - 1; i >= 0; i-- { | ||
| kv := nodes[i].kv | ||
| for j := 0; j+1 < len(kv); j += 2 { | ||
| if !yield(kv[j], kv[j+1]) { | ||
| return | ||
| } | ||
| } | ||
| } | ||
| }, true |
There was a problem hiding this comment.
Gemini suggested an optimizations here that improve performance slightly for most real world cases where n < 16 :
- Traverse the raw.added linked list only once while appending to the slice.
- Allocate the node array inside the closure.
return raw.md, func(yield func(string, string) bool) {
// Appending to a context more than 16 times is exceedingly rare, so
// this will stay on the stack and avoid heap allocations entirely.
var buf [16]*deltaKV
nodes := buf[:0]
for d := raw.added; d != nil; d = d.prev {
nodes = append(nodes, d)
}
// Yield in reverse order (oldest-first)
for i := len(nodes) - 1; i >= 0; i-- {
kv := nodes[i].kv
for j := 0; j+1 < len(kv); j += 2 {
if !yield(kv[j], kv[j+1]) {
return
}
}
}
}, true| n := 0 | ||
| for d := raw.added; d != nil; d = d.prev { | ||
| n++ | ||
| } | ||
| nodes := make([]*deltaKV, 0, n) |
There was a problem hiding this comment.
We can use a stack allocated array here also to avoid a heap alloc and an extra pass over the linked-list:
var buf [16]*deltaKV
nodes := buf[:0]
for d := raw.added; d != nil; d = d.prev {
nodes = append(nodes, d)
}
|
@notandruu : Friendly ping on this one. |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
4 participants
Addresses #8860
Problem
AppendToOutgoingContextis O(N) per call because it allocates a new[][]stringof length N+1 and copies all N prior slices on everyinvocation. A call chain with N sequential appends costs O(N²) in total
allocation and copy work. This is visible in production as latency spikes
in metadata-heavy RPCs.
Solution
Replace
rawMD.added [][]stringwith a singly-linked list ofdeltaKVnodes. Each
AppendToOutgoingContextcall allocates exactly one node andone flattened kv slice — O(1) regardless of chain depth.
FromOutgoingContextandfromOutgoingContextRawtraverse the chain(newest-first), collect into a temporary slice, then iterate in reverse
to preserve FIFO ordering, keeping the read path O(N) and maintaining
backward-compatible return types for internal callers in transport and
stream code.
Benchmarks
Apple M3 Max, go1.26.3:
All existing metadata tests pass.
RELEASE NOTES:
AppendToOutgoingContextis now O(1) per call instead of O(N), eliminating quadratic allocation overhead in metadata-heavy RPC call chains.