Issue 7313048: code review 7313048: net/http: add BenchmarkReadRequest (Closed)
|
Created:
12 years, 4 months ago by dave Modified:
12 years, 4 months ago Reviewers:
CC:
golang-dev, bradfitz, minux1, felixge Visibility:
Public. |
Descriptionnet/http: add BenchmarkReadRequest
Add benchmark for request parsing. Fixture data is taken from https://github.com/felixge/node-http-perf
% go version
go version devel +28966b7b2f0c Thu Feb 07 20:26:12 2013 -0800 linux/amd64
% go test -run=nil -bench=ReadRequest -benchtime=10s
PASS
BenchmarkReadRequest 2000000 9900 ns/op 61.71 MB/s 3148 B/op 32 allocs/op
ok net/http 12.180s
Patch Set 1 #Patch Set 2 : diff -r ec4982dfc0fc https://code.google.com/p/go #Patch Set 3 : diff -r ec4982dfc0fc https://code.google.com/p/go #Patch Set 4 : diff -r 28966b7b2f0c https://code.google.com/p/go #
Total comments: 4
Patch Set 5 : diff -r 28966b7b2f0c https://code.google.com/p/go #
Total comments: 5
Patch Set 6 : diff -r 28966b7b2f0c https://code.google.com/p/go #Patch Set 7 : diff -r 9d68dabd0026 https://code.google.com/p/go #MessagesTotal messages: 17
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
ping
Sign in to reply to this message.
It'd be nice if you could make an io.Reader which repeats a string n times, and then only make one bufio.Reader before the loop. That would reflect reality more too, where TCP connections are re-used with many requests (with one bufio per connection). On Wed, Feb 6, 2013 at 12:25 AM, <dave@cheney.net> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > net/http: add BenchmarkReadRequest > > Add benchmark for request parsing. Fixture data is taken from > https://github.com/felixge/**node-http-perf<https://github.com/felixge/node-h... > > % go version > go version devel +fceaf0ddea29 Tue Feb 05 15:39:55 2013 +1100 > linux/amd64 > > % go test -run=nil -bench=ReadRequest -benchmem -benchtime=10s > PASS > BenchmarkReadRequest 1000000 12041 ns/op 50.74 > MB/s 7369 B/op 34 allocs/op > ok net/http 12.180s > > Please review this at https://codereview.appspot.**com/7313048/<https://codereview.appspot.com/7313... > > Affected files: > M src/pkg/net/http/request_test.**go > > > Index: src/pkg/net/http/request_test.**go > ==============================**==============================**======= > --- a/src/pkg/net/http/request_**test.go > +++ b/src/pkg/net/http/request_**test.go > @@ -401,3 +401,29 @@ > ` + textbValue + ` > --MyBoundary-- > ` > + > +func benchmarkReadRequest(b *testing.B, buf []byte) { > + b.SetBytes(int64(len(buf))) > + r := bytes.NewReader(buf) > + for i := 0; i < b.N; i++ { > + r.Seek(0, 0) // reset > + _, err := ReadRequest(bufio.NewReader(r)**) > + if err != nil { > + b.Fatalf("failed to read request: %v", err) > + } > + } > +} > + > +func BenchmarkReadRequest(b *testing.B) { > + // taken from https://github.com/felixge/** > node-http-perf/blob/master/**fixtures/get.http<https://github.com/felixge/node-http-perf/blob/master/fixtures/get.http> > + const fixture = "GET / HTTP/1.1\r\n" + > + "Host: localhost:8080\r\n" + > + "Connection: keep-alive\r\n" + > + "Accept: text/html,application/xhtml+** > xml,application/xml;q=0.9,*/*;**q=0.8\r\n" + > + "User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X > 10_8_2) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 > Safari/537.17\r\n" + > + "Accept-Encoding: gzip,deflate,sdch\r\n" + > + "Accept-Language: en-US,en;q=0.8\r\n" + > + "Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.**3\r\n" + > + "Cookie: __utma=1.1978842379.**1323102373.1323102373.**1323102373.1; > EPi:NumberOfVisits=1,2012-02-**28T13:42:18; CrmSession=** > 5b707226b9563e1bc69084d07a107c**98; plushContainerWidth=100%25; > plushNoTopMenu=0; hudson_auto_refresh=false\r\n\**r\n" > + benchmarkReadRequest(b, []byte(fixture)) > +} > > > -- > > ---You received this message because you are subscribed to the Google > Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > >
Sign in to reply to this message.
Sure thing, will do. I was sad to see that ReadRequest takes a *bufio.Reader, not an io.Reader, but not much can be done about that now. On Fri, Feb 8, 2013 at 11:27 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > It'd be nice if you could make an io.Reader which repeats a string n times, > and then only make one bufio.Reader before the loop. > > That would reflect reality more too, where TCP connections are re-used with > many requests (with one bufio per connection). > > > On Wed, Feb 6, 2013 at 12:25 AM, <dave@cheney.net> wrote: >> >> Reviewers: golang-dev_googlegroups.com, >> >> Message: >> Hello golang-dev@googlegroups.com, >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> Description: >> net/http: add BenchmarkReadRequest >> >> Add benchmark for request parsing. Fixture data is taken from >> https://github.com/felixge/node-http-perf >> >> % go version >> go version devel +fceaf0ddea29 Tue Feb 05 15:39:55 2013 +1100 >> linux/amd64 >> >> % go test -run=nil -bench=ReadRequest -benchmem -benchtime=10s >> PASS >> BenchmarkReadRequest 1000000 12041 ns/op 50.74 >> MB/s 7369 B/op 34 allocs/op >> ok net/http 12.180s >> >> Please review this at https://codereview.appspot.com/7313048/ >> >> Affected files: >> M src/pkg/net/http/request_test.go >> >> >> Index: src/pkg/net/http/request_test.go >> =================================================================== >> --- a/src/pkg/net/http/request_test.go >> +++ b/src/pkg/net/http/request_test.go >> @@ -401,3 +401,29 @@ >> ` + textbValue + ` >> --MyBoundary-- >> ` >> + >> +func benchmarkReadRequest(b *testing.B, buf []byte) { >> + b.SetBytes(int64(len(buf))) >> + r := bytes.NewReader(buf) >> + for i := 0; i < b.N; i++ { >> + r.Seek(0, 0) // reset >> + _, err := ReadRequest(bufio.NewReader(r)) >> + if err != nil { >> + b.Fatalf("failed to read request: %v", err) >> + } >> + } >> +} >> + >> +func BenchmarkReadRequest(b *testing.B) { >> + // taken from >> https://github.com/felixge/node-http-perf/blob/master/fixtures/get.http >> + const fixture = "GET / HTTP/1.1\r\n" + >> + "Host: localhost:8080\r\n" + >> + "Connection: keep-alive\r\n" + >> + "Accept: >> text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\n" + >> + "User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X >> 10_8_2) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 >> Safari/537.17\r\n" + >> + "Accept-Encoding: gzip,deflate,sdch\r\n" + >> + "Accept-Language: en-US,en;q=0.8\r\n" + >> + "Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.3\r\n" + >> + "Cookie: >> __utma=1.1978842379.1323102373.1323102373.1323102373.1; >> EPi:NumberOfVisits=1,2012-02-28T13:42:18; >> CrmSession=5b707226b9563e1bc69084d07a107c98; plushContainerWidth=100%25; >> plushNoTopMenu=0; hudson_auto_refresh=false\r\n\r\n" >> + benchmarkReadRequest(b, []byte(fixture)) >> +} >> >> >> >> -- >> >> ---You received this message because you are subscribed to the Google >> Groups "golang-dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to golang-dev+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/groups/opt_out. >> >> >
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
https://codereview.appspot.com/7313048/diff/9001/src/pkg/net/http/request_tes... File src/pkg/net/http/request_test.go (right): https://codereview.appspot.com/7313048/diff/9001/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:406: b.SetBytes(int64(len(request))) b.ReportAllocs() https://codereview.appspot.com/7313048/diff/9001/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:407: r := bufio.NewReader(&infiniteReader{buf: []byte(request)}) b.ResetTimer()
Sign in to reply to this message.
Thank you for your feedback. PTAL. https://codereview.appspot.com/7313048/diff/9001/src/pkg/net/http/request_tes... File src/pkg/net/http/request_test.go (right): https://codereview.appspot.com/7313048/diff/9001/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:406: b.SetBytes(int64(len(request))) On 2013/02/08 06:47:37, minux wrote: > b.ReportAllocs() Done. https://codereview.appspot.com/7313048/diff/9001/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:407: r := bufio.NewReader(&infiniteReader{buf: []byte(request)}) On 2013/02/08 06:47:37, minux wrote: > b.ResetTimer() Done.
Sign in to reply to this message.
https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... File src/pkg/net/http/request_test.go (right): https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:427: r.offset += copy(b, r.buf[r.offset:n]) this (r.buf[r.offset:n]) seems wrong to me. i suggest this: n := copy(b, r.buf[r.offset:]) r.offset = (r.offset + n) % len(r.buf) return n, nil https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:441: const request = "GET / HTTP/1.1\r\n" + this string concatenation looks ugly to me. how about using raw string + strings.Replace(s, "\n", "\r\n", -1)? does this have license problems? i can't find any license info on the node-http-perf page.
Sign in to reply to this message.
https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... File src/pkg/net/http/request_test.go (right): https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:441: const request = "GET / HTTP/1.1\r\n" + > does this have license problems? i can't find any license info > on the node-http-perf page. It's a capture of the default GET request issued by Chrome, so I'm not sure if I could possibly claim copyright on that, but just in case: https://github.com/felixge/node-http-perf/commit/eface1ab82ce1fc7ef0dad60600b...
Sign in to reply to this message.
On Friday, February 8, 2013, haimuiba wrote: > > https://codereview.appspot.**com/7313048/diff/6002/src/pkg/** > net/http/request_test.go#**newcode441<https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_test.go#newcode441> > src/pkg/net/http/request_test.**go:441: const request = "GET / > HTTP/1.1\r\n" + > >> does this have license problems? i can't find any license info >> on the node-http-perf page. >> > > It's a capture of the default GET request issued by Chrome, so I'm not > sure if I could possibly claim copyright on that, but just in case: > > https://github.com/felixge/**node-http-perf/commit/** > eface1ab82ce1fc7ef0dad60600b65**0b66397e00<https://github.com/felixge/node-http-perf/commit/eface1ab82ce1fc7ef0dad60600b650b66397e00> > great. thanks for the prompt action. using the same fixture surely helps in comparing performance.
Sign in to reply to this message.
Thanks. Ill fix my reader code and repurpose after dinner On 08/02/2013, at 19:31, minux <minux.ma@gmail.com> wrote: > > On Friday, February 8, 2013, haimuiba wrote: >> >> https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... >> src/pkg/net/http/request_test.go:441: const request = "GET / >> HTTP/1.1\r\n" + >>> does this have license problems? i can't find any license info >>> on the node-http-perf page. >> >> It's a capture of the default GET request issued by Chrome, so I'm not >> sure if I could possibly claim copyright on that, but just in case: >> >> https://github.com/felixge/node-http-perf/commit/eface1ab82ce1fc7ef0dad60600b... > great. thanks for the prompt action. > using the same fixture surely helps in comparing performance.
Sign in to reply to this message.
> > using the same fixture surely helps in comparing performance. One thing about the fixture: It may not be the ideal fixture, as I suspect most benchmarking tools will include significantly less headers in their requests. I just picked it because I'd rather focus on reality, as far as this is possible with these vanity benchmarks : ). --fg On Friday, 8 February 2013 09:31:57 UTC+1, minux wrote: > > > On Friday, February 8, 2013, haimuiba wrote: >> >> https://codereview.appspot.**com/7313048/diff/6002/src/pkg/** >> net/http/request_test.go#**newcode441<https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_test.go#newcode441> >> src/pkg/net/http/request_test.**go:441: const request = "GET / >> HTTP/1.1\r\n" + >> >>> does this have license problems? i can't find any license info >>> on the node-http-perf page. >>> >> >> It's a capture of the default GET request issued by Chrome, so I'm not >> sure if I could possibly claim copyright on that, but just in case: >> >> https://github.com/felixge/**node-http-perf/commit/** >> eface1ab82ce1fc7ef0dad60600b65**0b66397e00<https://github.com/felixge/node-http-perf/commit/eface1ab82ce1fc7ef0dad60600b650b66397e00> >> > great. thanks for the prompt action. > using the same fixture surely helps in comparing performance. >
Sign in to reply to this message.
On Fri, Feb 8, 2013 at 5:09 PM, Felix Geisendoerfer <haimuiba@gmail.com>wrote: > using the same fixture surely helps in comparing performance. > > > One thing about the fixture: It may not be the ideal fixture, as I suspect > most benchmarking tools will include significantly less headers in their > requests. > but this request is typical for a web server to expect so i think it's better. > > I just picked it because I'd rather focus on reality, as far as this is > possible with these vanity benchmarks : ). > seconded. i agree with using real-world requests for benchmarks. optimizing for trivial cases might not actually speed up the average case (as one can always special case those). one of the reason i gave for not comparing Go by benchmarking hello world web service is that: if it's that important, the Go team could implement a pattern matching in the gc compiler and rewrite that code into much simpler one to just give better benchmark results.
Sign in to reply to this message.
We can add more, and in fact we should. That is why the current benchmark is over engineered. I have small patch to textproto that improves ReadRequest for that fixture by 10% but will probably be less effective with a wider sample size. On 08/02/2013, at 20:09, Felix Geisendoerfer <haimuiba@gmail.com> wrote: >> using the same fixture surely helps in comparing performance. > > One thing about the fixture: It may not be the ideal fixture, as I suspect most benchmarking tools will include significantly less headers in their requests. > > I just picked it because I'd rather focus on reality, as far as this is possible with these vanity benchmarks : ). > > --fg > > On Friday, 8 February 2013 09:31:57 UTC+1, minux wrote: >> >> >> On Friday, February 8, 2013, haimuiba wrote: >>> >>> https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... >>> src/pkg/net/http/request_test.go:441: const request = "GET / >>> HTTP/1.1\r\n" + >>>> does this have license problems? i can't find any license info >>>> on the node-http-perf page. >>> >>> It's a capture of the default GET request issued by Chrome, so I'm not >>> sure if I could possibly claim copyright on that, but just in case: >>> >>> https://github.com/felixge/node-http-perf/commit/eface1ab82ce1fc7ef0dad60600b... >> great. thanks for the prompt action. >> using the same fixture surely helps in comparing performance.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bradfitz@golang.org, minux.ma@gmail.com, haimuiba@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=7dc9a98ddfcc *** net/http: add BenchmarkReadRequest Add benchmark for request parsing. Fixture data is taken from https://github.com/felixge/node-http-perf % go version go version devel +28966b7b2f0c Thu Feb 07 20:26:12 2013 -0800 linux/amd64 % go test -run=nil -bench=ReadRequest -benchtime=10s PASS BenchmarkReadRequest 2000000 9900 ns/op 61.71 MB/s 3148 B/op 32 allocs/op ok net/http 12.180s R=golang-dev, bradfitz, minux.ma, haimuiba CC=golang-dev https://codereview.appspot.com/7313048 https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... File src/pkg/net/http/request_test.go (right): https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:427: r.offset += copy(b, r.buf[r.offset:n]) On 2013/02/08 07:16:51, minux wrote: > this (r.buf[r.offset:n]) seems wrong to me. > > i suggest this: > n := copy(b, r.buf[r.offset:]) > r.offset = (r.offset + n) % len(r.buf) > return n, nil Done, thank you. https://codereview.appspot.com/7313048/diff/6002/src/pkg/net/http/request_tes... src/pkg/net/http/request_test.go:441: const request = "GET / HTTP/1.1\r\n" + On 2013/02/08 07:16:51, minux wrote: > this string concatenation looks ugly to me. > how about using raw string + strings.Replace(s, "\n", "\r\n", -1)? > > does this have license problems? i can't find any license info > on the node-http-perf page. Done.
Sign in to reply to this message.
|
