Skip to content

Remove one allocation by foregoing context.WithValue#555

Merged
pkieltyka merged 1 commit intogo-chi:masterfrom
bouk:remove-context-withvalue-allocation
Nov 29, 2020
Merged

Remove one allocation by foregoing context.WithValue#555
pkieltyka merged 1 commit intogo-chi:masterfrom
bouk:remove-context-withvalue-allocation

Conversation

@bouk
Copy link
Copy Markdown
Contributor

@bouk bouk commented Nov 26, 2020 •
edited
Loading

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

@bouk
Remove one allocation by foregoing context.WithValue
09a3b7b 
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
Copy Markdown
Member

pkieltyka commented Nov 29, 2020

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

@bouk
Copy link
Copy Markdown
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
Copy Markdown
Member

pkieltyka commented Nov 29, 2020

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

@bouk
Copy link
Copy Markdown
Contributor Author

bouk commented Nov 30, 2020

Cool, thanks for merging 🙂

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

pkieltyka commented Feb 10, 2021

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bouk @pkieltyka