code review 61040044: bench: add http benchmark

bench: add http benchmark

[dvyukov, 2014-02-10 16:24:30]

Hello adg@golang.org, bradfitz@golang.org (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.benchmarks

[adg, 2014-02-11 05:14:00]

old chunk mismatch
please re-upload

[dvyukov, 2014-02-11 09:08:15]

On 2014/02/11 05:14:00, adg wrote:
> old chunk mismatch
> please re-upload

Done
f****** codereview

[adg, 2014-02-11 10:08:16]

brad should certainly comment on the http stuff, which looks ok to me

https://codereview.appspot.com/61040044/diff/60001/http/http.go
File http/http.go (right):

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode44
http/http.go:44: all, err := ioutil.ReadAll(res.Body)
s/all/b/

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode50
http/http.go:50: if body != "Hello world.\n" {
if s := string(b); s != "Hello world.\n" {

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode51
http/http.go:51: log.Fatalf("Got body: " + body)
fatalf here, but printf earlier?
comment as to why

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode57
http/http.go:57: var server = func() *http.Server {
var server *http.Server

func inti() {
  ...
  server = &http.Server{
  ...
}

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode74
http/http.go:74: time.Sleep(100 * time.Millisecond)
why not make a first Get request to the server, to wait for it to start?

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode75
http/http.go:75: return s
delete

[dvyukov, 2014-02-11 11:52:01]

PTAL

>brad should certainly comment on the http stuff, which looks ok to me

Brad?

https://codereview.appspot.com/61040044/diff/60001/http/http.go
File http/http.go (right):

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode44
http/http.go:44: all, err := ioutil.ReadAll(res.Body)
On 2014/02/11 10:08:16, adg wrote:
> s/all/b/

Done.

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode50
http/http.go:50: if body != "Hello world.\n" {
On 2014/02/11 10:08:16, adg wrote:
> if s := string(b); s != "Hello world.\n" {

Done.

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode51
http/http.go:51: log.Fatalf("Got body: " + body)
On 2014/02/11 10:08:16, adg wrote:
> fatalf here, but printf earlier?
> comment as to why

Done.

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode57
http/http.go:57: var server = func() *http.Server {
On 2014/02/11 10:08:16, adg wrote:
> var server *http.Server
> 
> func inti() {
>   ...
>   server = &http.Server{
>   ...
> }

Done.

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode74
http/http.go:74: time.Sleep(100 * time.Millisecond)
On 2014/02/11 10:08:16, adg wrote:
> why not make a first Get request to the server, to wait for it to start?

Done
this required more changes, because I wanted to reuse the code to make a single
request between the benchmark and here

https://codereview.appspot.com/61040044/diff/60001/http/http.go#newcode75
http/http.go:75: return s
On 2014/02/11 10:08:16, adg wrote:
> delete

Done.

[adg, 2014-02-11 12:24:37]

LGTM

Much nicer actually!

[bradfitz, 2014-02-11 21:54:28]

LGTM but see comments. feel free to ignore if you disagree.

https://codereview.appspot.com/61040044/diff/80001/http/http.go
File http/http.go (right):

https://codereview.appspot.com/61040044/diff/80001/http/http.go#newcode29
http/http.go:29: func benchmarkHttpImpl(N uint64) {
HTTPImpl capital

https://codereview.appspot.com/61040044/diff/80001/http/http.go#newcode68
http/http.go:68: l, err := net.Listen("tcp", "127.0.0.1:0")
I would just use httptest.NewUnstartedServer, tweak its params, then run
httptest.Server.Start.

It will do this IPv4 vs IPv6 stuff.

https://codereview.appspot.com/61040044/diff/80001/http/http.go#newcode88
http/http.go:88: Proxy: http.ProxyFromEnvironment,
I see why you'd want to use the same as the default, but I think you also want
to eliminate the use of a proxy which could interfere with benchmarks.  I would
just set this to nil.

[dvyukov, 2014-02-12 14:18:06]

https://codereview.appspot.com/61040044/diff/80001/http/http.go
File http/http.go (right):

https://codereview.appspot.com/61040044/diff/80001/http/http.go#newcode29
http/http.go:29: func benchmarkHttpImpl(N uint64) {
On 2014/02/11 21:54:29, bradfitz wrote:
> HTTPImpl capital

Done.

https://codereview.appspot.com/61040044/diff/80001/http/http.go#newcode68
http/http.go:68: l, err := net.Listen("tcp", "127.0.0.1:0")
On 2014/02/11 21:54:29, bradfitz wrote:
> I would just use httptest.NewUnstartedServer, tweak its params, then run
> httptest.Server.Start.
> 
> It will do this IPv4 vs IPv6 stuff.

I don't trust it.
It must work reliably back to Go1.
There is some historyListener and waitGroupHandler, I don't want them to affect
the results.
What if we decide to add anotherFancyHandlerWrapper that is really useful for
testing...

https://codereview.appspot.com/61040044/diff/80001/http/http.go#newcode88
http/http.go:88: Proxy: http.ProxyFromEnvironment,
On 2014/02/11 21:54:29, bradfitz wrote:
> I see why you'd want to use the same as the default, but I think you also want
> to eliminate the use of a proxy which could interfere with benchmarks.  I
would
> just set this to nil.


ProxyFromEnvironment is part of default setup which everybody uses, I would
prefer to test it as well.
But it's a good point about interference. I've added the following in the
beginning of init:

	// These environment variables affect net/http behavior,
	// ensure that we get predictable results regardless of environment on the
machine.
	os.Setenv("HTTP_PROXY", "")
	os.Setenv("http_proxy", "")
	os.Setenv("NO_PROXY", "")
	os.Setenv("no_proxy", "")

[dvyukov, 2014-02-12 14:22:45]

*** Submitted as
https://code.google.com/p/go/source/detail?r=66046080a303&repo=benchmarks ***

bench: add http benchmark

LGTM=adg, bradfitz
R=adg, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/61040044