encoding: make CallOption a concrete struct to avoid interface boxing - #2538
Open
geekswaroop wants to merge 2 commits into
Open
encoding: make CallOption a concrete struct to avoid interface boxing#2538geekswaroop wants to merge 2 commits into
geekswaroop wants to merge 2 commits into
Conversation
CallOption wrapped an unexported callOption interface, so WithHeader and the other With* constructors boxed a small concrete value into that interface on every call (runtime.convT). WithHeader alone accounts for ~733 CPU-cores across the production Go fleet. Replace the interface with a tagged value struct that carries the option data inline and dispatches in a single apply method. The public API is unchanged: the struct's fields are unexported and only the With* helpers construct it, so this is an internal representation swap. Benchmarks on go1.26 / AMD EPYC 7B13 (count=10): WithHeader 53.6 ns -> 10.2 ns (-81%, 32 B/1 alloc -> 0) WithShardKey (dynamic) 34.5 ns -> 2.2 ns (-94%, 16 B/1 alloc -> 0) NewOutboundCall (3 hdr) 523 ns -> 375 ns (-28%, 7 -> 4 allocs) Value equality via reflect.DeepEqual is preserved, so gomock argument matching on CallOptions still works.
geekswaroop
force-pushed
the
interface-boxing-with-headers
branch
from
August 26, 2026 15:02
53e78c2 to
3b2867b
Compare
rabbbit
reviewed
Aug 27, 2026
| // Encoding authors should accept yarpc.CallOptions and convert them to | ||
| // encoding.CallOptions to use with NewOutboundCall. This will keep the | ||
| // API for service authors simple. | ||
|
|
Contributor
There was a problem hiding this comment.
Hm, so the old code explicitly follows the https://github.com/uber-go/guide/blob/master/style.md#functional-options.
If we're breaking that we should have a good explanation, let's discuss.
rabbbit
reviewed
Aug 27, 2026
| func BenchmarkWithShardKey(b *testing.B) { | ||
| b.ReportAllocs() | ||
| for i := 0; i < b.N; i++ { | ||
| benchOptionSink = WithShardKey("shard-42") |
Contributor
There was a problem hiding this comment.
WithShardKey-96 0.6454n ± 6% 2.2050n ± 1% +241.65% (p=0.000 n=10)
why is this happening? :>
rabbbit
reviewed
Aug 27, 2026
| case callOptionTypeRoutingDelegate: | ||
| v := o.value | ||
| call.routingDelegate = &v | ||
| case callOptionTypeResponseHeaders: |
Contributor
There was a problem hiding this comment.
well we def need to do something here, right? (default case)
rabbbit
reviewed
Aug 27, 2026
Comment on lines
+26
to
+27
| benchOptionSink CallOption | ||
| benchCallSink *OutboundCall |
rabbbit
reviewed
Aug 27, 2026
| // BenchmarkWithHeader measures the core option-construction site. | ||
| func BenchmarkWithHeader(b *testing.B) { | ||
| b.ReportAllocs() | ||
| for i := 0; i < b.N; i++ { |
Contributor
There was a problem hiding this comment.
per https://github.com/yarpc/yarpc-go/blob/main/go.mod we're at 1.26 so we definitely can use b.Loop now.
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
2 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.
Description
encoding.CallOptionwrapped an unexportedcallOptioninterface, soWithHeaderand the otherWith*constructors boxed a small concrete value into that interface on every call (runtime.convT). Across Uber's production Go fleet,WithHeaderwas a hotspot for boxing.This replaces the interface with a tagged value struct that carries the option data inline and dispatches in a single
applymethod. The public API is unchanged and the struct's fields are unexported and only theWith*helpers construct it. So this is an internal representation swap with no caller impact.Test Plan
api/encodingunit tests pass, includingTestCallOptionsReflectEquals,which guards the
reflect.DeepEqualequality that gomock argument matching relies on.go build ./...,go vet ./..., gofmt, goimports, staticcheck, golint: all clean.go test ./...: green.call_option_bench_test.go.RELEASE NOTES:
encoding: constructing a
CallOption(WithHeader,WithShardKey,WithRoutingKey,WithRoutingDelegate) no longer allocates an interface box per option.