Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go/vt/callerid: speedup callerid context access #16352

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 14 additions & 20 deletions go/vt/callerid/callerid.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,15 @@ import (
)

// The datatype for CallerID Context Keys
type callerIDKey int

var (
// internal Context key for immediate CallerID
immediateCallerIDKey callerIDKey
// internal Context key for effective CallerID
effectiveCallerIDKey callerIDKey = 1
)
type callerIDContextKey struct{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the keys were integers. Now we have only one key and it is an empty struct. Is it actually more efficient to use an empty struct as the key versus an integer value of 0?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An int is 4 or 8 bytes of storage whereas an empty struct is 0 bytes AFAIUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a zero struct occupies no extra space, it's relatively insignificant here since we're talking about a single integer, but a zero sized struct has a constant pointer in the Go runtime and is effectively a pointer to one address no matter the type. Specifically this runtime.zerobase.

The comparison has to be done with an interface because of the any in Context, so an interface just holds a reference to what type + the value, in which case, it's effectively only a ref to the type without a value, since the value is nothing, and the comparison is just on the type, rather than an interface of {type: int, val: 1} or whatever.

The docs also suggest using empty structs for keys: https://pkg.go.dev/context#WithValue

To avoid allocating when assigning to an interface{}, context keys often have concrete type struct{}. Alternatively, exported context key variables' static type should be a pointer or interface.

Examples of this can be seen through the stdlib codebase.

https://github.com/golang/go/blob/a71bb570d064f6d0c1cf59cef4b7a044be7a39d3/src/runtime/trace/annotation.go#L14
https://github.com/golang/go/blob/a71bb570d064f6d0c1cf59cef4b7a044be7a39d3/src/net/http/transport.go#L2809
https://github.com/golang/go/blob/a71bb570d064f6d0c1cf59cef4b7a044be7a39d3/src/runtime/pprof/label.go#L25

etc

I doubt in practice this makes a tangible difference, but it is the established pattern in stdlib for context keys. If they were to make compiler optimizations for this, I imagine they'd target this pattern specifically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. TiL


// callerIDContext is a tuple containing the entirety
// of the "callerid" such that both pointers can be
// stored together.
type callerIDContext struct {
ef *vtrpcpb.CallerID
im *querypb.VTGateCallerID
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call these effective and immediate for (a) readability and (b) so that people can correlate them to concepts they are already familiar with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can do that. I yanked ef and im since they were used throughout this file already for function args, etc.

}

// NewImmediateCallerID creates a querypb.VTGateCallerID initialized with username
func NewImmediateCallerID(username string) *querypb.VTGateCallerID {
Expand Down Expand Up @@ -90,30 +91,23 @@ func GetSubcomponent(ef *vtrpcpb.CallerID) string {
// NewContext adds the provided EffectiveCallerID(vtrpcpb.CallerID) and ImmediateCallerID(querypb.VTGateCallerID)
// into the Context
func NewContext(ctx context.Context, ef *vtrpcpb.CallerID, im *querypb.VTGateCallerID) context.Context {
ctx = context.WithValue(
context.WithValue(ctx, effectiveCallerIDKey, ef),
immediateCallerIDKey,
im,
)
return ctx
return context.WithValue(ctx, callerIDContextKey{}, callerIDContext{ef, im})
}

// EffectiveCallerIDFromContext returns the EffectiveCallerID(vtrpcpb.CallerID)
// stored in the Context, if any
func EffectiveCallerIDFromContext(ctx context.Context) *vtrpcpb.CallerID {
ef, ok := ctx.Value(effectiveCallerIDKey).(*vtrpcpb.CallerID)
if ok && ef != nil {
return ef
if cid, ok := ctx.Value(callerIDContextKey{}).(callerIDContext); ok {
return cid.ef
}
return nil
}

// ImmediateCallerIDFromContext returns the ImmediateCallerID(querypb.VTGateCallerID)
// stored in the Context, if any
func ImmediateCallerIDFromContext(ctx context.Context) *querypb.VTGateCallerID {
im, ok := ctx.Value(immediateCallerIDKey).(*querypb.VTGateCallerID)
if ok && im != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also removed the extraneous != nil check, which wasn't actually free. But is entirely unnecessary to check. If it's nil, can simply return nil.

return im
if cid, ok := ctx.Value(callerIDContextKey{}).(callerIDContext); ok {
return cid.im
}
return nil
}
67 changes: 67 additions & 0 deletions go/vt/callerid/callerid_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
Copyright 2019 The Vitess Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package callerid_test

import (
"context"
"testing"

"vitess.io/vitess/go/vt/callerid"

"github.com/stretchr/testify/assert"
)

func TestCallerIDContext(t *testing.T) {
ef := callerid.NewEffectiveCallerID("principal", "component", "subComponent")
im := callerid.NewImmediateCallerID("username")
ctx := callerid.NewContext(context.Background(), ef, im)

assert.Equal(t, ef, callerid.EffectiveCallerIDFromContext(ctx))
assert.Equal(t, im, callerid.ImmediateCallerIDFromContext(ctx))

assert.Nil(t, callerid.EffectiveCallerIDFromContext(context.Background()))
assert.Nil(t, callerid.ImmediateCallerIDFromContext(context.Background()))
}

func BenchmarkCallerIDContext(b *testing.B) {
ef := callerid.NewEffectiveCallerID("principal", "component", "subComponent")
im := callerid.NewImmediateCallerID("username")

b.Run("New", func(b *testing.B) {
b.ReportAllocs()
baseCtx := context.Background()
for range b.N {
_ = callerid.NewContext(baseCtx, ef, im)
}
})

b.Run("EffectiveCallerIDFromContext", func(b *testing.B) {
b.ReportAllocs()
ctx := callerid.NewContext(context.Background(), ef, im)
for range b.N {
_ = callerid.EffectiveCallerIDFromContext(ctx)
}
})

b.Run("ImmediateCallerIDFromContext", func(b *testing.B) {
b.ReportAllocs()
ctx := callerid.NewContext(context.Background(), ef, im)
for range b.N {
_ = callerid.EffectiveCallerIDFromContext(ctx)
}
})
}
Loading