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

Issue 6843056: code review 6843056: net/http, net/textproto: cache common header values (Closed)

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

Description

net/http, net/textproto: cache common header values We use a small, automatically updated cache of recently seen values in order to avoid allocations for values like "keep-alive" and "close". benchmark old ns/op new ns/op delta BenchmarkHeaderWriteSubset 2215 2284 +3.12% BenchmarkClientServer 360168 382181 +6.11% BenchmarkClientServerParallel4 382254 371981 -2.69% BenchmarkClientServerParallel64 319201 304283 -4.67% BenchmarkServer 336708 327858 -2.63% benchmark old allocs new allocs delta BenchmarkHeaderWriteSubset 2 2 0.00% BenchmarkClientServer 114 105 -7.89% BenchmarkClientServerParallel4 114 105 -7.89% BenchmarkClientServerParallel64 115 106 -7.83% BenchmarkServer 36 32 -11.11% Benchmarks in net/textproto: benchmark old ns/op new ns/op delta BenchmarkReadMIMEHeader 11302 10785 -4.57% BenchmarkUncommon 1851 1777 -4.00% benchmark old allocs new allocs delta BenchmarkReadMIMEHeader 25 19 -24.00% BenchmarkUncommon 5 4 -20.00%

Patch Set 1 #

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

Total comments: 16

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

Total comments: 3

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

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

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

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -7 lines) Patch
M src/pkg/net/http/request.go View 1 2 3 2 chunks +4 lines, -2 lines 1 comment Download
M src/pkg/net/http/response.go View 1 2 3 2 chunks +4 lines, -3 lines 1 comment Download
M src/pkg/net/http/server.go View 1 2 3 2 chunks +38 lines, -0 lines 0 comments Download
M src/pkg/net/textproto/reader.go View 1 2 3 4 3 chunks +61 lines, -2 lines 0 comments Download
M src/pkg/net/textproto/reader_test.go View 1 2 3 1 chunk +34 lines, -0 lines 0 comments Download

Messages

Total messages: 12
bradfitz
https://codereview.appspot.com/6843056/diff/1002/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6843056/diff/1002/src/pkg/net/textproto/reader.go#newcode595 src/pkg/net/textproto/reader.go:595: str string s is probably enough https://codereview.appspot.com/6843056/diff/1002/src/pkg/net/textproto/reader.go#newcode598 src/pkg/net/textproto/reader.go:598: type ...
9 years ago (2012-11-14 17:13:34 UTC) #1
jeff.allen
https://codereview.appspot.com/6843056/diff/1002/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6843056/diff/1002/src/pkg/net/textproto/reader.go#newcode595 src/pkg/net/textproto/reader.go:595: str string On 2012/11/14 17:13:34, bradfitz wrote: > s ...
9 years ago (2012-11-15 10:50:01 UTC) #2
dvyukov
https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go#newcode642 src/pkg/net/textproto/reader.go:642: sc[len(x)].s = s That will crash and corrupt memory. ...
9 years ago (2012-11-15 11:33:15 UTC) #3
dvyukov
https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go#newcode642 src/pkg/net/textproto/reader.go:642: sc[len(x)].s = s On 2012/11/15 11:33:15, dvyukov wrote: > ...
9 years ago (2012-11-15 11:44:16 UTC) #4
dvyukov
On 2012/11/15 11:44:16, dvyukov wrote: > https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go > File src/pkg/net/textproto/reader.go (right): > > https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go#newcode642 > ...
9 years ago (2012-11-15 11:50:02 UTC) #5
dvyukov
https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go File src/pkg/net/textproto/reader.go (right): https://codereview.appspot.com/6843056/diff/7001/src/pkg/net/textproto/reader.go#newcode650 src/pkg/net/textproto/reader.go:650: sc[len(x)].miss = 0 You better not update it if ...
9 years ago (2012-11-15 11:53:05 UTC) #6
jeff.allen
Cool, my mental model was that a string assign is atomic. I'm going to play ...
9 years ago (2012-11-15 15:54:54 UTC) #7
dvyukov
On Thu, Nov 15, 2012 at 7:54 PM, <jeff.allen@gmail.com> wrote: > Cool, my mental model ...
9 years ago (2012-11-15 16:13:57 UTC) #8
jeff.allen
Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
9 years ago (2012-11-16 15:21:41 UTC) #9
dfc
Hi Jeff, Thanks for this proposal. I don't think the performance gain here is worth ...
9 years ago (2012-11-20 09:18:35 UTC) #10
bradfitz
It's easy to write a benchmark that makes a particular optimization look good. I agree ...
9 years ago (2012-11-20 18:01:26 UTC) #11
jeff.allen
9 years ago (2012-11-21 09:42:19 UTC) #12
I agree that it feels like this change is messy.

The performance improvement from reducing garbage comes long after the function
in question is complete. It is hard to measure it, and it is only a win in bulk.
So as I work on reducing garbage, I make it a goal that the function should get
faster as well. That way, the wins are guaranteed: the function is faster
(immediate payoff) AND there's less trash (long-term payoff).

I think I need to build a bit of infrastructure to quantify the GC cost
reduction so that I can make better decisions about the justifiable costs
increases.

Stay tuned. :)
Sign in to reply to this message.

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