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

Remove one allocation by foregoing context.WithValue #555

Merged
merged 1 commit into from
Nov 29, 2020

Conversation

bouk
Copy link
Contributor

@bouk bouk commented Nov 26, 2020

Instead of we can make Context implement the context.Context interface directly through a private type, which just implements the Value method.

If you want to remove the last allocation you could also do *r = *r.WithContext((*directContext)(rctx)), although that would be modifying something you don't own.

Benchmark

Before

~/s/g/g/chi (master) go test -bench B
goos: darwin
goarch: amd64
BenchmarkMux/route:/-12                  4089687               291 ns/op             448 B/op          3 allocs/op
BenchmarkMux/route:/hi-12                3795570               306 ns/op             448 B/op          3 allocs/op
BenchmarkMux/route:/sup/123/and/this-12                  2908492               412 ns/op             448 B/op          3 allocs/op
BenchmarkMux/route:/sup/123/foo/this-12                  2328763               519 ns/op             448 B/op          3 allocs/op
BenchmarkMux/route:/sharing/z/aBc-12                     2355368               509 ns/op             448 B/op          3 allocs/op
BenchmarkMux/route:/sharing/z/aBc/twitter-12             1925193               618 ns/op             456 B/op          4 allocs/op
BenchmarkMux/route:/sharing/z/aBc/direct-12              1654678               726 ns/op             456 B/op          4 allocs/op
BenchmarkMux/route:/sharing/z/aBc/direct/download-12             1445162               827 ns/op             480 B/op          5 allocs/op
BenchmarkTreeGet-12                                              8847862               133 ns/op               0 B/op          0 allocs/op
PASS
ok      _/Users/bouke/src/github.com/go-chi/chi 16.505s

After

~/s/g/g/chi (master) go test -bench B
goos: darwin
goarch: amd64
BenchmarkMux/route:/-12                  4975675               238 ns/op             400 B/op          2 allocs/op
BenchmarkMux/route:/hi-12                4772744               255 ns/op             400 B/op          2 allocs/op
BenchmarkMux/route:/sup/123/and/this-12                  3353338               360 ns/op             400 B/op          2 allocs/op
BenchmarkMux/route:/sup/123/foo/this-12                  2503300               500 ns/op             400 B/op          2 allocs/op
BenchmarkMux/route:/sharing/z/aBc-12                     2226513               477 ns/op             400 B/op          2 allocs/op
BenchmarkMux/route:/sharing/z/aBc/twitter-12             2153860               553 ns/op             408 B/op          3 allocs/op
BenchmarkMux/route:/sharing/z/aBc/direct-12              1837567               653 ns/op             408 B/op          3 allocs/op
BenchmarkMux/route:/sharing/z/aBc/direct/download-12             1609606               748 ns/op             432 B/op          4 allocs/op
BenchmarkTreeGet-12                                              8756982               136 ns/op               0 B/op          0 allocs/op
PASS
ok      _/Users/bouke/src/github.com/go-chi/chi 16.282s

With unsafe *r = *r.WithContext() (not in this PR)

goos: darwin
goarch: amd64
BenchmarkMux/route:/-12                  9876838               113 ns/op              83 B/op          0 allocs/op
BenchmarkMux/route:/hi-12                9744604               110 ns/op              84 B/op          0 allocs/op
BenchmarkMux/route:/sup/123/and/this-12                  3793071               331 ns/op             438 B/op          0 allocs/op
BenchmarkMux/route:/sup/123/foo/this-12                  2710080               423 ns/op             568 B/op          0 allocs/op
BenchmarkMux/route:/sharing/z/aBc-12                    11624925               102 ns/op              88 B/op          0 allocs/op
BenchmarkMux/route:/sharing/z/aBc/twitter-12             3851288               306 ns/op              93 B/op          3 allocs/op
BenchmarkMux/route:/sharing/z/aBc/direct-12             11640464               112 ns/op              88 B/op          0 allocs/op
BenchmarkMux/route:/sharing/z/aBc/direct/download-12             3862566               307 ns/op              93 B/op          3 allocs/op
BenchmarkTreeGet-12                                              8824483               145 ns/op               0 B/op          0 allocs/op
PASS
ok      _/Users/bouke/src/github.com/go-chi/chi 14.098s

Instead of we can make Context implement the context.Context interface
directly through a private type, which just implements the Value method.
@pkieltyka
Copy link
Member

This is cool, thanks for the PR. It’s tempting but I want to make it a bit more elegant

@bouk
Copy link
Contributor Author

bouk commented Nov 29, 2020

@pkieltyka sure, what do you have in mind?

@pkieltyka pkieltyka merged commit 8391bdb into go-chi:master Nov 29, 2020
@pkieltyka
Copy link
Member

@bouk just minor things with comments and placement. I've pushed a follow-up commit: 364c564

@bouk
Copy link
Contributor Author

bouk commented Nov 30, 2020

Cool, thanks for merging 🙂

@pkieltyka pkieltyka mentioned this pull request Feb 10, 2021
@pkieltyka
Copy link
Member

FYI, I've had to revert this change, #581

@bouk bouk deleted the remove-context-withvalue-allocation branch February 10, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants