Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2071)

Issue 6721055: code review 6721055: net/textproto: faster header canonicalization with fewe... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by jeff.allen
Modified:
11 years, 4 months ago
Reviewers:
CC:
bradfitz, golang-dev, dfc, rsc, jra
Visibility:
Public.

Description

net/textproto: faster header canonicalization with fewer allocations By keeping a single copy of the strings that commonly show up in headers, we can avoid one string allocation per header. benchmark old ns/op new ns/op delta BenchmarkReadMIMEHeader 19590 10824 -44.75% BenchmarkUncommon 3168 1861 -41.26% benchmark old allocs new allocs delta BenchmarkReadMIMEHeader 32 25 -21.88% BenchmarkUncommon 5 5 0.00%

Patch Set 1 #

Patch Set 2 : diff -r 3d637cc9dff0 https://code.google.com/p/go #

Patch Set 3 : diff -r 3d637cc9dff0 https://code.google.com/p/go #

Total comments: 5

Patch Set 4 : diff -r 8d919bfe75d3 https://code.google.com/p/go #

Patch Set 5 : diff -r 3312fddeb739 https://code.google.com/p/go #

Total comments: 8

Patch Set 6 : diff -r 93dc7f0e302b https://code.google.com/p/go #

Patch Set 7 : diff -r 25dcee3d220c https://code.google.com/p/go #

Patch Set 8 : diff -r 25dcee3d220c https://code.google.com/p/go #

Patch Set 9 : diff -r 25dcee3d220c https://code.google.com/p/go #

Total comments: 1

Patch Set 10 : diff -r eb7dceb84a12 https://code.google.com/p/go #

Patch Set 11 : diff -r eb7dceb84a12 https://code.google.com/p/go #

Total comments: 1

Patch Set 12 : diff -r 42c8d3aadc40 https://code.google.com/p/go #

Patch Set 13 : diff -r 42c8d3aadc40 https://code.google.com/p/go #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -14 lines) Patch
M src/pkg/net/textproto/reader.go View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +75 lines, -11 lines 2 comments Download
M src/pkg/net/textproto/reader_test.go View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +83 lines, -3 lines 1 comment Download

Messages

Total messages: 25
jeff.allen
Hello bradfitz@golang.org, golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
11 years, 5 months ago (2012-10-17 21:03:27 UTC) #1
dfc
Thanks for this proposal. I think with a bit of work we could recover that ...
11 years, 5 months ago (2012-10-18 07:17:36 UTC) #2
bradfitz
https://codereview.appspot.com/6721055/diff/5001/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6721055/diff/5001/src/pkg/net/textproto/reader.go#newcode567 src/pkg/net/textproto/reader.go:567: "Content-MD5", really? common? this is some S3 thing I ...
11 years, 5 months ago (2012-10-22 22:02:46 UTC) #3
jeff.allen
I think the lost 3% simply comes from the hash lookup, which is one more ...
11 years, 5 months ago (2012-10-23 10:38:12 UTC) #4
jeff.allen
I got the trie working. With the giant list of headers from my first patch, ...
11 years, 5 months ago (2012-10-25 12:14:29 UTC) #5
rsc
What if you use binary search instead of a trie?
11 years, 5 months ago (2012-10-25 21:20:06 UTC) #6
jeff.allen
Hello bradfitz@golang.org, golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 5 months ago (2012-10-30 16:02:42 UTC) #7
bradfitz
https://codereview.appspot.com/6721055/diff/18001/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6721055/diff/18001/src/pkg/net/textproto/reader.go#newcode512 src/pkg/net/textproto/reader.go:512: if h, ok := searchHeaderTrie(a); ok { this could ...
11 years, 5 months ago (2012-10-30 16:09:27 UTC) #8
bradfitz
The latest numbers in the CL description are accurate? On Tue, Oct 30, 2012 at ...
11 years, 5 months ago (2012-10-30 16:09:40 UTC) #9
dfc
http://codereview.appspot.com/6721055/diff/18001/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): http://codereview.appspot.com/6721055/diff/18001/src/pkg/net/textproto/reader.go#newcode511 src/pkg/net/textproto/reader.go:511: if false { I think this is an accident. ...
11 years, 5 months ago (2012-10-30 16:28:14 UTC) #10
jeff.allen
I realized I could reduce the headerTrie memory consumption from 20k down to 8k. Yay! ...
11 years, 5 months ago (2012-10-31 14:27:33 UTC) #11
rsc
I am not convinced that a trie is necessary. I understand that it is enjoyable ...
11 years, 4 months ago (2012-11-01 18:24:32 UTC) #12
jeff.allen
Got it, thanks for the idea. I experimented with something like this, but I was ...
11 years, 4 months ago (2012-11-02 09:02:34 UTC) #13
jeff.allen
Please take another look. This now feels like really the right way to go; it ...
11 years, 4 months ago (2012-11-07 16:08:53 UTC) #14
dfc
On 2012/11/07 16:08:53, jeff.allen wrote: > Please take another look. This now feels like really ...
11 years, 4 months ago (2012-11-08 00:38:12 UTC) #15
jeff.allen
Hello bradfitz@golang.org, golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-08 07:06:33 UTC) #16
rsc
Please add a test that loops over commonHeaders making sure that each entry is its ...
11 years, 4 months ago (2012-11-08 14:41:30 UTC) #17
jeff.allen
Hello bradfitz@golang.org, golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-08 15:26:59 UTC) #18
bradfitz
Looking nice. https://codereview.appspot.com/6721055/diff/28003/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6721055/diff/28003/src/pkg/net/textproto/reader.go#newcode552 src/pkg/net/textproto/reader.go:552: var commonHeaders = []string{ add: "If-Modified-Since" "If-None-Match"
11 years, 4 months ago (2012-11-08 16:47:46 UTC) #19
jeff.allen
Hello bradfitz@golang.org, golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-09 08:36:04 UTC) #20
bradfitz
LGTM after the docs are simplified, unless that was requested. Russ, any comments? https://codereview.appspot.com/6721055/diff/19009/src/pkg/net/textproto/reader.go File ...
11 years, 4 months ago (2012-11-09 09:15:54 UTC) #21
jra
I added the sentence because the behavior was a surprise to me. Le 9 nov. ...
11 years, 4 months ago (2012-11-09 15:43:58 UTC) #22
rsc
looks good; leaving for brad https://codereview.appspot.com/6721055/diff/19009/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6721055/diff/19009/src/pkg/net/textproto/reader.go#newcode489 src/pkg/net/textproto/reader.go:489: // MIME headers should ...
11 years, 4 months ago (2012-11-12 20:40:15 UTC) #23
bradfitz
Per email from Jeff, submitting with the doc change. On Mon, Nov 12, 2012 at ...
11 years, 4 months ago (2012-11-12 23:30:39 UTC) #24
bradfitz
11 years, 4 months ago (2012-11-12 23:31:45 UTC) #25
*** Submitted as http://code.google.com/p/go/source/detail?r=d465db6441bc ***

net/textproto: faster header canonicalization with fewer allocations

By keeping a single copy of the strings that commonly show up
in headers, we can avoid one string allocation per header.

benchmark                  old ns/op    new ns/op    delta
BenchmarkReadMIMEHeader        19590        10824  -44.75%
BenchmarkUncommon               3168         1861  -41.26%

benchmark                 old allocs   new allocs    delta
BenchmarkReadMIMEHeader           32           25  -21.88%
BenchmarkUncommon                  5            5    0.00%

R=bradfitz, golang-dev, dave, rsc, jra
CC=golang-dev
http://codereview.appspot.com/6721055

Committer: Brad Fitzpatrick <bradfitz@golang.org>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b