code review 6454142: net/http: reduce mutex contention

net/http: reduce mutex contention
benchmark                           old ns/op    new ns/op    delta
BenchmarkClientServerParallel          155909       154454   -0.93%
BenchmarkClientServerParallel-2         86012        82986   -3.52%
BenchmarkClientServerParallel-4         70211        55168  -21.43%
BenchmarkClientServerParallel-8         80755        47862  -40.73%
BenchmarkClientServerParallel-12        77753        51478  -33.79%
BenchmarkClientServerParallel-16        77920        50278  -35.47%
The benchmark is http://codereview.appspot.com/6441134
The machine is 2 x 4 HT cores (16 HW threads total).
Fixes issue 3946.
Now contention moves to net.pollServer.AddFD().

[dvyukov, 2012-08-13 11:31:30]

Hello bradfitz@golang.org (cc: dave@cheney.net, golang-dev@googlegroups.com,
remyoudompheng@gmail.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dfc, 2012-08-13 11:41:53]

Looks pretty good to me. Can you post the new contention graph somewhere ?

http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:262: t.idleConn = make(map[string][]*persistConn)
Can this move back to the end of CloseIdleConnctions so the lock isn't held
during allocation?

[dvyukov, 2012-08-13 12:01:44]

On 2012/08/13 11:41:53, dfc wrote:
> Looks pretty good to me. Can you post the new contention graph somewhere ?

Attached to the bug
http://code.google.com/p/go/issues/detail?id=3946

> http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go
> File src/pkg/net/http/transport.go (right):
> 
>
http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go...
> src/pkg/net/http/transport.go:262: t.idleConn =
make(map[string][]*persistConn)
> Can this move back to the end of CloseIdleConnctions so the lock isn't held
> during allocation?

Is it expected case? I think it must be non-nil most of the time.

[remyoudompheng, 2012-08-18 09:38:06]

Looks trivial enough to me.

[bradfitz, 2012-08-20 05:59:22]

LGTM

http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:136: t.altLk.Lock()
why not RLock here?

[gobot, 2012-08-20 06:16:36]

R=gri (assigned by bradfitz)

[gobot, 2012-08-20 06:16:36]

R=dsymonds (assigned by bradfitz)

[gobot, 2012-08-20 06:16:37]

R=dsymonds (assigned by bradfitz)

[gobot, 2012-08-20 06:21:01]

R=bradfitz (assigned by bradfitz)

[dvyukov, 2012-08-20 09:24:05]

http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6454142/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:136: t.altLk.Lock()
On 2012/08/20 05:59:22, bradfitz wrote:
> why not RLock here?

Done.

[dvyukov, 2012-08-20 09:24:33]

*** Submitted as http://code.google.com/p/go/source/detail?r=43568ec5cf55 ***

net/http: reduce mutex contention
benchmark                           old ns/op    new ns/op    delta
BenchmarkClientServerParallel          155909       154454   -0.93%
BenchmarkClientServerParallel-2         86012        82986   -3.52%
BenchmarkClientServerParallel-4         70211        55168  -21.43%
BenchmarkClientServerParallel-8         80755        47862  -40.73%
BenchmarkClientServerParallel-12        77753        51478  -33.79%
BenchmarkClientServerParallel-16        77920        50278  -35.47%
The benchmark is http://codereview.appspot.com/6441134
The machine is 2 x 4 HT cores (16 HW threads total).
Fixes issue 3946.
Now contention moves to net.pollServer.AddFD().

R=bradfitz
CC=bradfitz, dave, dsymonds, gobot, golang-dev, remyoudompheng
http://codereview.appspot.com/6454142

[dfc, 2012-08-20 09:27:40]

Thank you. 

On 20/08/2012, at 19:24, dvyukov@google.com wrote:

> *** Submitted as
> http://code.google.com/p/go/source/detail?r=43568ec5cf55 ***
> 
> net/http: reduce mutex contention
> benchmark                           old ns/op    new ns/op    delta
> BenchmarkClientServerParallel          155909       154454   -0.93%
> BenchmarkClientServerParallel-2         86012        82986   -3.52%
> BenchmarkClientServerParallel-4         70211        55168  -21.43%
> BenchmarkClientServerParallel-8         80755        47862  -40.73%
> BenchmarkClientServerParallel-12        77753        51478  -33.79%
> BenchmarkClientServerParallel-16        77920        50278  -35.47%
> The benchmark is http://codereview.appspot.com/6441134
> The machine is 2 x 4 HT cores (16 HW threads total).
> Fixes issue 3946.
> Now contention moves to net.pollServer.AddFD().
> 
> R=bradfitz
> CC=bradfitz, dave, dsymonds, gobot, golang-dev, remyoudompheng
> http://codereview.appspot.com/6454142
> 
> 
> http://codereview.appspot.com/6454142/