Remove redundant CanonicalizeHeaderKey from addApplicationHeaders - #2444
Open
kshitijsuri90 wants to merge 2 commits into
Open
Remove redundant CanonicalizeHeaderKey from addApplicationHeaders#2444kshitijsuri90 wants to merge 2 commits into
kshitijsuri90 wants to merge 2 commits into
Conversation
kshitijsuri90
force-pushed
the
kshitij/remove-canonicalize-addheaders
branch
from
April 8, 2026 12:31
7a039d5 to
8c472a6
Compare
| func addApplicationHeaders(md metadata.MD, headers transport.Headers) error { | ||
| for header, value := range headers.Items() { | ||
| header = transport.CanonicalizeHeaderKey(header) | ||
| // Items() keys are already canonical (lowercased on insertion via With). |
There was a problem hiding this comment.
nit: comment is a bit confusing for first time readers.
MahammadAgayev
approved these changes
Jun 19, 2026
kshitijsuri90
force-pushed
the
kshitij/remove-canonicalize-addheaders
branch
from
July 27, 2026 08:32
8c472a6 to
e07f876
Compare
rabbbit
approved these changes
Jul 29, 2026
Problem:
addApplicationHeaders calls transport.CanonicalizeHeaderKey
(strings.ToLower) on every key from headers.Items() while building
outbound gRPC metadata. These keys are already lowercase, making the
call redundant CPU work on every outbound RPC.
Fix:
Remove the CanonicalizeHeaderKey call from addApplicationHeaders.
Safety — Items() keys are guaranteed lowercase by the Headers API:
- Items() returns the unexported items map whose keys are stored
via With(), which always lowercases on insertion:
canonicalizedKey := CanonicalizeHeaderKey(k)
h.items[canonicalizedKey] = v
https://github.com/yarpc/yarpc-go/blob/main/api/transport/header.go#L84-L85
- The items field is unexported, so no external code can bypass
With() to insert non-canonical keys.
- The Items() doc explicitly states:
"Keys in the map are normalized using CanonicalizeHeaderKey."
Call sites:
- transportRequestToMetadata at headers.go:131 (client outbound path)
- response_writer.go:70 (server response path)
Impact:
Eliminates one strings.ToLower scan per application header on the
client outbound and server response paths.
Benchmark (count=6, AMD EPYC 9B45):
AddApplicationHeaders: 454.8 ns/op → 441.3 ns/op (-2.96%, p=0.002)
Rebase note (2026-07-26): rebased onto current main; resolved a
positional conflict in transport/grpc/headers_test.go where main's
newly-added BenchmarkIsReserved and this PR's BenchmarkAddApplicationHeaders
were both appended at the same spot — kept both functions.
Made-with: Cursor
kshitijsuri90
force-pushed
the
kshitij/remove-canonicalize-addheaders
branch
from
August 18, 2026 09:34
e07f876 to
15d7b57
Compare
bananacocodrilo
approved these changes
Aug 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
transport.CanonicalizeHeaderKeycall fromaddApplicationHeadersintransport/grpc/headers.go. The keys fromHeaders.Items()are already guaranteed lowercase by the Headers API contract.Safety
Items()returns keys stored viaWith(), which always lowercases on insertion (CanonicalizeHeaderKey(k))itemsfield is unexported — no external code can bypassWith()Items()doc: "Keys in the map are normalized using CanonicalizeHeaderKey"Benchmark
count=6, AMD EPYC 9B45:Test plan
BenchmarkAddApplicationHeadersadded and run with count=6RELEASE NOTES: N/a
Update (2026-07-26): rebased onto current
main. One positional conflict intransport/grpc/headers_test.go: main's newly-addedBenchmarkIsReservedand this PR'sBenchmarkAddApplicationHeaderswere both appended at the same location — resolved by keeping both functions.