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

Issue 9432046: code review 9432046: net/http: fewer allocations in the server path (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by bradfitz
Modified:
11 years, 7 months ago
Reviewers:
nigeltao
CC:
nigeltao, golang-dev
Visibility:
Public.

Description

net/http: fewer allocations in the server path Don't allocate for the Date or Content-Length headers. A custom Date header formatter replaces use of time.Format. benchmark old ns/op new ns/op delta BenchmarkClientServer 67791 64424 -4.97% BenchmarkClientServerParallel4 62956 58533 -7.03% BenchmarkClientServerParallel64 62043 54789 -11.69% BenchmarkServer 254609 229060 -10.03% BenchmarkServerFakeConnNoKeepAlive 17038 16316 -4.24% BenchmarkServerFakeConnWithKeepAlive 14184 13226 -6.75% BenchmarkServerFakeConnWithKeepAliveLite 8591 7532 -12.33% BenchmarkServerHandlerTypeLen 10750 9961 -7.34% BenchmarkServerHandlerNoLen 9535 8935 -6.29% BenchmarkServerHandlerNoType 9858 9362 -5.03% BenchmarkServerHandlerNoHeader 7754 6856 -11.58% benchmark old allocs new allocs delta BenchmarkClientServer 68 66 -2.94% BenchmarkClientServerParallel4 68 66 -2.94% BenchmarkClientServerParallel64 68 66 -2.94% BenchmarkServer 21 19 -9.52% BenchmarkServerFakeConnNoKeepAlive 32 30 -6.25% BenchmarkServerFakeConnWithKeepAlive 27 25 -7.41% BenchmarkServerFakeConnWithKeepAliveLite 12 10 -16.67% BenchmarkServerHandlerTypeLen 19 18 -5.26% BenchmarkServerHandlerNoLen 17 15 -11.76% BenchmarkServerHandlerNoType 17 16 -5.88% BenchmarkServerHandlerNoHeader 12 10 -16.67% Update Issue 5195

Patch Set 1 #

Patch Set 2 : diff -r 0d28fd55e721 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 0d28fd55e721 https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 731724b03c62 https://go.googlecode.com/hg/ #

Patch Set 5 : diff -r 731724b03c62 https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r 731724b03c62 https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 7 : diff -r 53b012eb5e17 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -13 lines) Patch
M src/pkg/net/http/serve_test.go View 1 2 3 4 5 3 chunks +3 lines, -0 lines 0 comments Download
M src/pkg/net/http/server.go View 1 2 3 4 5 6 6 chunks +56 lines, -13 lines 0 comments Download

Messages

Total messages: 5
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
11 years, 7 months ago (2013-05-16 01:40:10 UTC) #1
bradfitz
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 7 months ago (2013-05-18 20:26:05 UTC) #2
bradfitz
Now without time.FormatAppend (or without time.Format). Also, without the gross workaround for Issue 5492 that ...
11 years, 7 months ago (2013-05-18 20:27:30 UTC) #3
nigeltao
LGTM. https://codereview.appspot.com/9432046/diff/16001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/9432046/diff/16001/src/pkg/net/http/server.go#newcode662 src/pkg/net/http/server.go:662: headerDate = []byte("Date: ") Swap the order so ...
11 years, 7 months ago (2013-05-20 00:52:13 UTC) #4
bradfitz
11 years, 7 months ago (2013-05-20 03:15:47 UTC) #5
*** Submitted as https://code.google.com/p/go/source/detail?r=647f336edfe8 ***

net/http: fewer allocations in the server path

Don't allocate for the Date or Content-Length headers.
A custom Date header formatter replaces use of time.Format.

benchmark                                   old ns/op    new ns/op    delta
BenchmarkClientServer                           67791        64424   -4.97%
BenchmarkClientServerParallel4                  62956        58533   -7.03%
BenchmarkClientServerParallel64                 62043        54789  -11.69%
BenchmarkServer                                254609       229060  -10.03%
BenchmarkServerFakeConnNoKeepAlive              17038        16316   -4.24%
BenchmarkServerFakeConnWithKeepAlive            14184        13226   -6.75%
BenchmarkServerFakeConnWithKeepAliveLite         8591         7532  -12.33%
BenchmarkServerHandlerTypeLen                   10750         9961   -7.34%
BenchmarkServerHandlerNoLen                      9535         8935   -6.29%
BenchmarkServerHandlerNoType                     9858         9362   -5.03%
BenchmarkServerHandlerNoHeader                   7754         6856  -11.58%

benchmark                                  old allocs   new allocs    delta
BenchmarkClientServer                              68           66   -2.94%
BenchmarkClientServerParallel4                     68           66   -2.94%
BenchmarkClientServerParallel64                    68           66   -2.94%
BenchmarkServer                                    21           19   -9.52%
BenchmarkServerFakeConnNoKeepAlive                 32           30   -6.25%
BenchmarkServerFakeConnWithKeepAlive               27           25   -7.41%
BenchmarkServerFakeConnWithKeepAliveLite           12           10  -16.67%
BenchmarkServerHandlerTypeLen                      19           18   -5.26%
BenchmarkServerHandlerNoLen                        17           15  -11.76%
BenchmarkServerHandlerNoType                       17           16   -5.88%
BenchmarkServerHandlerNoHeader                     12           10  -16.67%

Update Issue 5195

R=nigeltao
CC=golang-dev
https://codereview.appspot.com/9432046

https://codereview.appspot.com/9432046/diff/16001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/9432046/diff/16001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:662: headerDate          = []byte("Date: ")
On 2013/05/20 00:52:13, nigeltao wrote:
> Swap the order so that they're sorted alphabetically? Similarly above and
below.

Done.

https://codereview.appspot.com/9432046/diff/16001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:666: // The value receiver, despite copying 5 strings
to the stack,
On 2013/05/20 00:52:13, nigeltao wrote:
> This comment could do with some wordsmithing.

Done.
Sign in to reply to this message.

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