code review 45700043: runtime: remove locks from netpoll hotpaths

runtime: remove locks from netpoll hotpaths
Introduces two-phase goroutine parking mechanism -- prepare to park, commit park.
This mechanism does not require backing mutex to protect wait predicate.
Use it in netpoll. See comment in netpoll.goc for details.
This slightly reduces contention between reader, writer and read/write io notifications;
and just eliminates a bunch of mutex operations from hotpaths, thus making then faster.

benchmark                             old ns/op    new ns/op    delta
BenchmarkTCP4ConcurrentReadWrite           2109         1945   -7.78%
BenchmarkTCP4ConcurrentReadWrite-2         1162         1113   -4.22%
BenchmarkTCP4ConcurrentReadWrite-4          798          755   -5.39%
BenchmarkTCP4ConcurrentReadWrite-8          803          748   -6.85%
BenchmarkTCP4Persistent                    9411         9240   -1.82%
BenchmarkTCP4Persistent-2                  5888         5813   -1.27%
BenchmarkTCP4Persistent-4                  4016         3968   -1.20%
BenchmarkTCP4Persistent-8                  3943         3857   -2.18%

[dvyukov, 2013-12-31 10:55:43]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[mikio, 2014-01-01 09:04:45]

I'haven't tested yet but looks good. Please pick any one hotpath or fastpath in
the CL description.

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:27: //        the goroutine commits to park by
chaning the state to G pointer,
s/chaning/changing/

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:281: netpollblock(PollDesc *pd, int32 mode, bool
checkerrors)
I'd rather call this argument semwait or just wait instead of checkerrors.

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:307: return false;
drop unnecessary trailing tabs or spaces

[dvyukov, 2014-01-01 13:25:20]

PTAL

I've also fixed a bug on windows, where I could lose IO notification and
deadlock
now I use atomic exchange (instead of plain store) to reset the semaphore to
nil, so the notification is not lost

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:281: netpollblock(PollDesc *pd, int32 mode, bool
checkerrors)
On 2014/01/01 09:04:45, mikio wrote:
> I'd rather call this argument semwait or just wait instead of checkerrors.

renamed to waitio
this function waits in either case, but when this flag is set it waits only for
IO

https://codereview.appspot.com/45700043/diff/120001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:307: return false;
On 2014/01/01 09:04:45, mikio wrote:
> drop unnecessary trailing tabs or spaces

Done.

[mikio, 2014-01-02 10:03:28]

LGTM

[dvyukov, 2014-01-04 13:42:58]

https://codereview.appspot.com/45700043/diff/140001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/45700043/diff/140001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:1345: static bool parkunlock(G *gp, void *lock)
newline after bool, please

[dvyukov, 2014-01-18 16:55:09]

https://codereview.appspot.com/45700043/diff/140001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/45700043/diff/140001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:1345: static bool parkunlock(G *gp, void *lock)
On 2014/01/04 13:42:59, dvyukov wrote:
> newline after bool, please

Done.

[dvyukov, 2014-01-18 17:00:47]

anybody else?

[gobot, 2014-01-19 18:03:25]

R=iant@golang.org (assigned by bradfitz@golang.org)

[rsc, 2014-01-22 03:51:49]

Please remember that when you say "anybody else?" to golang-codereviews, no one
can hear you.

[rsc, 2014-01-22 03:55:26]

LGTM

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:348: return old > WAIT ? old : nil;
please do not use ? : operators.
they almost always make the code difficult to follow.
there is a reason they are not in go.

if(old > WAIT)
    return old;
return nil;

[iant, 2014-01-22 05:32:12]

LGTM

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (left):

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:100: runtime·lock(pd);
Not acquiring the lock here means that the closing and rd fields are accessed
without a lock, contradicting the comment in the definition of PollDesc.  That
probably deserves a comment here or somewhere.

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:27: //        the goroutine commits to park by
chaning the state to G pointer,
s/chaning/changing/

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:41: // The following fields are manipulated w/o
locks.
s/The following fields/rg and wg/

There are several following fields, it's not automatically clear that the
comment ends at the blank line.

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:127: err = checkerr(pd, mode);
Here again closing and rd may be examined without a lock.

[dvyukov, 2014-01-22 07:02:57]

On Wed, Jan 22, 2014 at 7:51 AM,  <rsc@golang.org> wrote:
> Please remember that when you say "anybody else?" to golang-codereviews,
> no one can hear you.

I've added "the anybody" into CC list :)

[dvyukov, 2014-01-22 07:28:40]

*** Submitted as https://code.google.com/p/go/source/detail?r=da649606c628 ***

runtime: remove locks from netpoll hotpaths
Introduces two-phase goroutine parking mechanism -- prepare to park, commit
park.
This mechanism does not require backing mutex to protect wait predicate.
Use it in netpoll. See comment in netpoll.goc for details.
This slightly reduces contention between reader, writer and read/write io
notifications;
and just eliminates a bunch of mutex operations from hotpaths, thus making then
faster.

benchmark                             old ns/op    new ns/op    delta
BenchmarkTCP4ConcurrentReadWrite           2109         1945   -7.78%
BenchmarkTCP4ConcurrentReadWrite-2         1162         1113   -4.22%
BenchmarkTCP4ConcurrentReadWrite-4          798          755   -5.39%
BenchmarkTCP4ConcurrentReadWrite-8          803          748   -6.85%
BenchmarkTCP4Persistent                    9411         9240   -1.82%
BenchmarkTCP4Persistent-2                  5888         5813   -1.27%
BenchmarkTCP4Persistent-4                  4016         3968   -1.20%
BenchmarkTCP4Persistent-8                  3943         3857   -2.18%

R=golang-codereviews, mikioh.mikioh, gobot, iant, rsc
CC=golang-codereviews, khr
https://codereview.appspot.com/45700043

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.goc
File src/pkg/runtime/netpoll.goc (right):

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:27: //        the goroutine commits to park by
chaning the state to G pointer,
On 2014/01/22 05:32:13, iant wrote:
> s/chaning/changing/

Done.

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:41: // The following fields are manipulated w/o
locks.
On 2014/01/22 05:32:13, iant wrote:
> s/The following fields/rg and wg/
> 
> There are several following fields, it's not automatically clear that the
> comment ends at the blank line.

Agree, this was confusing.
I've added an extended comment describing how fields are protected.

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:127: err = checkerr(pd, mode);
On 2014/01/22 05:32:13, iant wrote:
> Here again closing and rd may be examined without a lock.

Added a comment to PollDesc.

https://codereview.appspot.com/45700043/diff/200001/src/pkg/runtime/netpoll.g...
src/pkg/runtime/netpoll.goc:348: return old > WAIT ? old : nil;
On 2014/01/22 03:55:27, rsc wrote:
> please do not use ? : operators.
> they almost always make the code difficult to follow.
> there is a reason they are not in go.
> 
> if(old > WAIT)
>     return old;
> return nil;

Done.