transport: presize header slice for outgoing metadata#9243
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9243 +/- ##
==========================================
+ Coverage 83.27% 83.33% +0.06%
==========================================
Files 421 421
Lines 34084 34092 +8
==========================================
+ Hits 28382 28411 +29
+ Misses 4273 4254 -19
+ Partials 1429 1427 -2
🚀 New features to boost your workflow:
|
|
@geekswaroop |
easwars
added
Type: Performance
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the createHeaderFields method in http2_client.go by pre-calculating the required slice capacity for metadata headers to minimize allocations. It also introduces a new benchmark file, http2_client_header_bench_test.go, to evaluate the performance impact. The reviewer suggested adding a benchmark case that utilizes AppendToOutgoingContext to ensure full coverage of the optimization logic, providing a code suggestion to facilitate this improvement.
createHeaderFields pre-sizes the headerFields slice, but the estimate omits the outgoing metadata (md and added from the outgoing context, plus t.md). For RPCs that propagate context headers (tracing, tenancy, routing, etc.), the slice starts at capacity ~8 and grows once or twice via append on nearly every call. Fetch the raw outgoing metadata once, add its field count to hfLen, and reuse the fetched maps in the append loop below. Reserved headers are dropped when writing, so this may slightly over-count, which is preferable to growing the slice. This completes the TODO(mmukhi) noted above the size estimate, which grpc#8547 addressed for scalar fields (deadline, compressors, retry attempts) but left open for metadata. The standard unary benchmark used in grpc#8547 sends no application metadata, so the metadata realloc path was not visible in the profiles that motivated that PR. Benchmark (internal/transport, BenchmarkCreateHeaderFields, table-driven by outgoing metadata field count, benchstat over 10 runs): mdCount allocs/op B/op sec/op 0 5 -> 5 416 -> 416 ~ (indistinguishable) 4 6 -> 5 1120 -> 576 -49% B, -17% allocs, -24% time 12 7 -> 5 2528 -> 992 -61% B, -29% allocs, -38% time Macro benchmark (benchmark/benchmain, 5 alternated 60s unary runs, benchstat): every metric indistinguishable (all p>0.2). Expected -- the standard workload sends no outgoing metadata, so the counting loops iterate zero times and there is nothing to save. Confirms no regression on the workload grpc#8547 measured. RELEASE NOTES: * transport: Include outgoing metadata in the header slice size estimate to eliminate reallocations for RPCs that propagate context headers. Signed-off-by: Krishna Swaroop <krishna.swaroop@uber.com>
4b4a1ac to
6e72630
Compare
|
Assigning to @mbissa for second review |
|
Storing benchmark results from original PR description here for posterity: Completes the Testing
Micro-benchmark
Macro benchmark
|
3 participants
createHeaderFieldspre-sizes theheaderFieldsslice, but the estimate omits the outgoing metadata (mdandaddedfrom the outgoing context, plust.md). For RPCs that propagate context headers (tracing, tenancy, routing, etc.), the slice starts at capacity ~8 and grows once or twice viaappendon nearly every call.This change fetches the raw outgoing metadata once, adds its field count to
hfLen, and reuses the fetched maps in the append loop below. Reserved headers are dropped when writing, so this may slightly over-count, which is preferable to growing the slice.RELEASE NOTES: none