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

context: Remove allocation discussion from WithValue documentation #33742

Open
evanj opened this issue Aug 20, 2019 · 3 comments
Open

context: Remove allocation discussion from WithValue documentation #33742

evanj opened this issue Aug 20, 2019 · 3 comments

Comments

@evanj
Copy link
Contributor

@evanj evanj commented Aug 20, 2019

This is a proposed package documentation change. I'm happy to submit a code change with this update, if it makes sense. I took the liberty of abbreviating the questions in the template.

What version of Go are you using (go version)?

Documentation in the latest source of context.go (commit d6ffc1d8394d6f6420bb92d79d320da88720fbe0)

What does the current documentation say

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."

Current documentation: https://tip.golang.org/pkg/context/#WithValue
Code:

go/src/context/context.go

Lines 476 to 479 in d6ffc1d

// types for keys. 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.

What should it say

Those two sentences should be removed. With Go >= 1.9, it no longer matters. To verify, I ran the following test with different versions of Go on a VM with a command like:

docker run --workdir=/wtf/test -v $HOME:/wtf -ti --rm golang:1.12 go test -bench=. .

I tested each release from 1.12 through to 1.8. With version 1.8, this mattered a lot. It no longer does. From the output below, you can see that using an int key is slower, but does not allocate. The other choices (interface{}, pointer, custom string), all appear to be equivalent.

I think it would simplify the package documentation to omit this.

This was previously changed after the discussion in #17826 . My test is based on that one.

Go 1.12

goos: linux
goarch: amd64
BenchmarkInterfaceKey-2      	1000000000	         2.64 ns/op	       0 B/op	       0 allocs/op
BenchmarkIntKey-2            	300000000	         4.56 ns/op	       0 B/op	       0 allocs/op
BenchmarkStringKey-2         	1000000000	         2.64 ns/op	       0 B/op	       0 allocs/op
BenchmarkCustomStringKey-2   	1000000000	         2.64 ns/op	       0 B/op	       0 allocs/op
BenchmarkEmptyStructKey-2    	1000000000	         2.63 ns/op	       0 B/op	       0 allocs/op
BenchmarkPtrKey-2            	1000000000	         2.63 ns/op	       0 B/op	       0 allocs/op

Go 1.9

goos: linux
goarch: amd64
BenchmarkInterfaceKey-2      	1000000000	         2.66 ns/op	       0 B/op	       0 allocs/op
BenchmarkIntKey-2            	300000000	         5.28 ns/op	       0 B/op	       0 allocs/op
BenchmarkStringKey-2         	1000000000	         2.64 ns/op	       0 B/op	       0 allocs/op
BenchmarkCustomStringKey-2   	1000000000	         2.63 ns/op	       0 B/op	       0 allocs/op
BenchmarkEmptyStructKey-2    	1000000000	         2.66 ns/op	       0 B/op	       0 allocs/op
BenchmarkPtrKey-2            	1000000000	         2.67 ns/op	       0 B/op	       0 allocs/op

Go 1.8

BenchmarkInterfaceKey-2      	300000000	         4.37 ns/op	       0 B/op	       0 allocs/op
BenchmarkIntKey-2            	50000000	        30.6 ns/op	       8 B/op	       1 allocs/op
BenchmarkStringKey-2         	30000000	        47.6 ns/op	      16 B/op	       1 allocs/op
BenchmarkCustomStringKey-2   	30000000	        47.6 ns/op	      16 B/op	       1 allocs/op
BenchmarkEmptyStructKey-2    	100000000	        15.1 ns/op	       0 B/op	       0 allocs/op
BenchmarkPtrKey-2            	300000000	         4.57 ns/op	       0 B/op	       0 allocs/op

Test code

package test

import (
	"context"
	"testing"
)

type key interface{}

var keyInterface key = 0

type keyIntType int

var keyInt keyIntType = 0

type List struct{}

type emptyStruct struct{}

var emptyStructKey = emptyStruct{}

const stringKey = "somestring"

type customStringKeyT string

const customStringKey = customStringKeyT("customstring")

var someString = "hello"
var ptrKey *string = &someString

func BenchmarkInterfaceKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(keyInterface)
		}
	})
}

func BenchmarkIntKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(keyInt)
		}
	})
}

func BenchmarkStringKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(stringKey)
		}
	})
}

func BenchmarkCustomStringKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(customStringKey)
		}
	})
}

func BenchmarkEmptyStructKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(emptyStructKey)
		}
	})
}

func BenchmarkPtrKey(b *testing.B) {
	b.ReportAllocs()
	ctx := context.Background()
	b.RunParallel(func(pb *testing.PB) {
		for pb.Next() {
			ctx.Value(ptrKey)
		}
	})
}
@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Aug 20, 2019

If you change

var keyInt keyIntType = 0

to

var keyInt keyIntType = 1

then it allocates. If you change

const stringKey = "somestring"

to

var stringKey = "somestring"

then it allocates. (Tested with Go 1.12.5.)

There are some nice optimizations to prevent certain interface allocations, but they shouldn't be documented here. Best to stick with simple rules of thumb.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 20, 2019

CC @Sajmani for context.

@bcmills bcmills added this to the Unplanned milestone Aug 20, 2019
@evanj

This comment has been minimized.

Copy link
Contributor Author

@evanj evanj commented Aug 20, 2019

Wow thanks @cespare! Since I was wrong and this does matter, should we change the example in the package to follow its own advice and not use type key int?: https://github.com/golang/go/blob/master/src/context/context.go#L133

There is also duplicated description about context key types: There is a description on Context.Value: "A key can be any type that supports equality [...]", as well as on WithValue: "The provided key must be comparable and [...]" Possibly the WithValue documentation should refer to Context.Value, or vice-versa. Alternatively, both places could have the full description of "good" key types?

Here are the updated metrics with the var string and key types for Go 1.12. As you can see: var/const makes a huge difference.

Go 1.12

BenchmarkVarInterfaceKey-2           	1000000000	         2.67 ns/op	       0 B/op	       0 allocs/op
BenchmarkVarIntKeyZero-2          	300000000	         4.34 ns/op	       0 B/op	       0 allocs/op
BenchmarkVarIntKeyOne-2           	100000000	        20.6 ns/op	       8 B/op	       1 allocs/op
BenchmarkConstIntKeyOne-2         	1000000000	         2.64 ns/op	       0 B/op	       0 allocs/op
BenchmarkConstStringKey-2         	1000000000	         2.65 ns/op	       0 B/op	       0 allocs/op
BenchmarkVarStringKey-2           	50000000	        36.5 ns/op	      16 B/op	       1 allocs/op
BenchmarkConstCustomStringKey-2   	1000000000	         2.64 ns/op	       0 B/op	       0 allocs/op
BenchmarkVarCustomStringKey-2     	50000000	        36.2 ns/op	      16 B/op	       1 allocs/op
BenchmarkVarEmptyStructKey-2      	1000000000	         2.63 ns/op	       0 B/op	       0 allocs/op
BenchmarkVarPtrKey-2              	1000000000	         2.63 ns/op	       0 B/op	       0 allocs/op
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.