code review 7569043: runtime: integrated network poller for darwin

runtime: integrated network poller for darwin
vs tip:
benchmark                           old ns/op    new ns/op    delta
BenchmarkTCP4Persistent                 67786        33175  -51.06%
BenchmarkTCP4Persistent-2               49085        31227  -36.38%
BenchmarkTCP4PersistentTimeout          69265        32565  -52.98%
BenchmarkTCP4PersistentTimeout-2        49217        32588  -33.79%

vs old scheduler:
benchmark                           old ns/op    new ns/op    delta
BenchmarkTCP4Persistent                 63517        33175  -47.77%
BenchmarkTCP4Persistent-2               54760        31227  -42.97%
BenchmarkTCP4PersistentTimeout          63234        32565  -48.50%
BenchmarkTCP4PersistentTimeout-2        56956        32588  -42.78%

[dvyukov, 2013-03-07 23:41:59]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-03-07 23:48:47]

It's faster due to several reasons:
1. Uses edge-triggered notifications, arm kevent only once per fd.
2. Uses more efficient heap-based runtime timers.
3. No global mutex, no global hashmap, uses direct context pointers.
4. Does not use 8 dedicated polling goroutines that call enter/exitsyscall,
instead worker threads poll when idle (unique to runtime).
5. Injects batches of newly runnable goroutines directly into scheduler (unique
to runtime).
6. Lighter-weight goroutine parking/unparking on binary semaphores (unique to
runtime).

[bradfitz, 2013-03-07 23:49:36]

This will be useful for everybody who runs servers on Macs.

On Thu, Mar 7, 2013 at 3:41 PM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
> Description:
> runtime: integrated network poller for darwin
> vs tip:
> benchmark                           old ns/op    new ns/op    delta
> BenchmarkTCP4Persistent                 67786        33175  -51.06%
> BenchmarkTCP4Persistent-2               49085        31227  -36.38%
> BenchmarkTCP4PersistentTimeout          69265        32565  -52.98%
> BenchmarkTCP4PersistentTimeout**-2        49217        32588  -33.79%
>
> vs old scheduler:
> benchmark                           old ns/op    new ns/op    delta
> BenchmarkTCP4Persistent                 63517        33175  -47.77%
> BenchmarkTCP4Persistent-2               54760        31227  -42.97%
> BenchmarkTCP4PersistentTimeout          63234        32565  -48.50%
> BenchmarkTCP4PersistentTimeout**-2        56956        32588  -42.78%
>
> Please review this at
https://codereview.appspot.**com/7569043/<https://codereview.appspot.com/7569...
>
> Affected files:
>   R src/pkg/net/fd_darwin.go
>   A src/pkg/net/fd_poll_runtime.go
>   M src/pkg/net/fd_poll_unix.go
>   M src/pkg/runtime/defs_darwin_**386.h
>   M src/pkg/runtime/defs_darwin_**amd64.h
>   A src/pkg/runtime/netpoll.c
>   A src/pkg/runtime/netpoll_**kqueue.c
>   M src/pkg/runtime/sys_darwin_**386.s
>   M src/pkg/runtime/sys_darwin_**amd64.s
>
>
> --
>
> ---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...
> .
>
>
>

[dvyukov, 2013-03-07 23:56:16]

On 2013/03/07 23:49:36, bradfitz wrote:
> This will be useful for everybody who runs servers on Macs.

Local godoc will be blazingly fast as well :)

Seriously, the idea was to test common code with both epoll and kqueue, see
what's common and what's different, and whether is can work with anything
different than epoll. The next change will do the same for linux/epoll. + I
suspect kqueue impl can be reused by *BSD, and in the end only 3 functions are
kqueue-specific.

[bradfitz, 2013-03-07 23:59:25]

On Thu, Mar 7, 2013 at 3:56 PM, <dvyukov@google.com> wrote:

> On 2013/03/07 23:49:36, bradfitz wrote:
>
>> This will be useful for everybody who runs servers on Macs.
>>
>
> Local godoc will be blazingly fast as well :)
>
> Seriously, the idea was to test common code with both epoll and kqueue,
> see what's common and what's different, and whether is can work with
> anything different than epoll.


Yeah, I figured.  And it really will be nice for people who run servers on
Macs, even if there are only 4 of them.


> The next change will do the same for
> linux/epoll.


yay!


> + I suspect kqueue impl can be reused by *BSD, and in the
> end only 3 functions are kqueue-specific.


nice.

[dvyukov, 2013-03-07 23:59:56]

Platform-specific part is quite small, see the similar change for linux:
https://codereview.appspot.com/7579044/diff/2001/src/pkg/runtime/netpoll_linux.c

[dvyukov, 2013-03-08 17:25:20]

Peter, can you take a look at this?

[dvyukov, 2013-03-08 17:25:56]

On 2013/03/08 17:25:20, dvyukov wrote:
> Peter, can you take a look at this?

It depends on https://codereview.appspot.com/7448048/ which plugs netpoll() into
scheduler.

[dho, 2013-03-08 17:39:25]

2013/3/7  <dvyukov@google.com>:
> On 2013/03/07 23:49:36, bradfitz wrote:
>>
>> This will be useful for everybody who runs servers on Macs.
>
> Local godoc will be blazingly fast as well :)
>
> Seriously, the idea was to test common code with both epoll and kqueue,
> see what's common and what's different, and whether is can work with
> anything different than epoll. The next change will do the same for
> linux/epoll. + I suspect kqueue impl can be reused by *BSD, and in the
> end only 3 functions are kqueue-specific.

I don't think this is going to change much for the different BSDs,
probably some differences in syscall numbers, but I think the rest
'just works.' Are you intending to submit a CL to implement this on
the BSDs?

--dho

[dvyukov, 2013-03-08 17:43:28]

On Fri, Mar 8, 2013 at 9:39 PM, Devon H. O'Dell <devon.odell@gmail.com> wrote:
> 2013/3/7  <dvyukov@google.com>:
>> On 2013/03/07 23:49:36, bradfitz wrote:
>>>
>>> This will be useful for everybody who runs servers on Macs.
>>
>> Local godoc will be blazingly fast as well :)
>>
>> Seriously, the idea was to test common code with both epoll and kqueue,
>> see what's common and what's different, and whether is can work with
>> anything different than epoll. The next change will do the same for
>> linux/epoll. + I suspect kqueue impl can be reused by *BSD, and in the
>> end only 3 functions are kqueue-specific.
>
> I don't think this is going to change much for the different BSDs,
> probably some differences in syscall numbers, but I think the rest
> 'just works.' Are you intending to submit a CL to implement this on
> the BSDs?

When at least 1 OS is in, I will ask platform maintainers to port it
to BSD/Windows/Plan9.
Yes, for BSD it can be as simple as implementing necessary syscalls and
testing.

[dho, 2013-03-08 17:46:12]

2013/3/8 Dmitry Vyukov <dvyukov@google.com>:
> On Fri, Mar 8, 2013 at 9:39 PM, Devon H. O'Dell <devon.odell@gmail.com>
wrote:
>> 2013/3/7  <dvyukov@google.com>:
>>> On 2013/03/07 23:49:36, bradfitz wrote:
>>>>
>>>> This will be useful for everybody who runs servers on Macs.
>>>
>>> Local godoc will be blazingly fast as well :)
>>>
>>> Seriously, the idea was to test common code with both epoll and kqueue,
>>> see what's common and what's different, and whether is can work with
>>> anything different than epoll. The next change will do the same for
>>> linux/epoll. + I suspect kqueue impl can be reused by *BSD, and in the
>>> end only 3 functions are kqueue-specific.
>>
>> I don't think this is going to change much for the different BSDs,
>> probably some differences in syscall numbers, but I think the rest
>> 'just works.' Are you intending to submit a CL to implement this on
>> the BSDs?
>
> When at least 1 OS is in, I will ask platform maintainers to port it
> to BSD/Windows/Plan9.
> Yes, for BSD it can be as simple as implementing necessary syscalls and
testing.

Cool! This looks really great, looking forward to getting the rest working :D

--dho

[mikio, 2013-03-09 04:44:48]

go1.0.3 vs. tip w/ CL 7569043+7448048 on darwin/amd64:
benchmark                           old ns/op    new ns/op    delta
BenchmarkTCP4OneShot-2                7759750      7637668   -1.57%
BenchmarkTCP4OneShotTimeout-2         7713735      7588605   -1.62%
BenchmarkTCP4Persistent-2               51776        27071  -47.72%
BenchmarkTCP4PersistentTimeout-2        51556        27615  -46.44%

https://codereview.appspot.com/7569043/diff/22001/src/pkg/net/fd_poll_runtime.go
File src/pkg/net/fd_poll_runtime.go (right):

https://codereview.appspot.com/7569043/diff/22001/src/pkg/net/fd_poll_runtime...
src/pkg/net/fd_poll_runtime.go:15: func runtime_pollServerInit()
net.runtime_PollServerInit
  vs.
net.runtimePollServerInit
  vs.
net.runtime_pollserverinit

hm, the second one might be better?

https://codereview.appspot.com/7569043/diff/22001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/22001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:15: runtime·netpollinit(void)
can we have netpoll.h that contains those prototypes?

[dvyukov, 2013-03-09 11:34:13]

On Sat, Mar 9, 2013 at 8:44 AM,  <mikioh.mikioh@gmail.com> wrote:
> go1.0.3 vs. tip w/ CL 7569043+7448048 on darwin/amd64:
>
> benchmark                           old ns/op    new ns/op    delta
> BenchmarkTCP4OneShot-2                7759750      7637668   -1.57%
> BenchmarkTCP4OneShotTimeout-2         7713735      7588605   -1.62%
> BenchmarkTCP4Persistent-2               51776        27071  -47.72%
> BenchmarkTCP4PersistentTimeout-2        51556        27615  -46.44%

TCP connection creation seems to be slow on darwin for some OS internal
reasons.
On Linux I see significant speedup on OneShot benchmarks as well.
I mean that the problem is in the OS, not in our tcp stack.

[dvyukov, 2013-03-09 11:40:08]

https://codereview.appspot.com/7569043/diff/22001/src/pkg/net/fd_poll_runtime.go
File src/pkg/net/fd_poll_runtime.go (right):

https://codereview.appspot.com/7569043/diff/22001/src/pkg/net/fd_poll_runtime...
src/pkg/net/fd_poll_runtime.go:15: func runtime_pollServerInit()
On 2013/03/09 04:44:49, mikio wrote:
> net.runtime_PollServerInit
>   vs.
> net.runtimePollServerInit
>   vs.
> net.runtime_pollserverinit
> 
> hm, the second one might be better?

runtime_ is the convention used in other places.
In package net it's called as runtime_fooBar(), and so it's clear that it is
different and comes from runtime package.
See:
zsema_darwin_386.c:sync·runtime_Semacquire(uint32* addr)
runtime.c:runtime∕pprof·runtime_cyclesPerSecond(int64 res)

https://codereview.appspot.com/7569043/diff/22001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/22001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:15: runtime·netpollinit(void)
On 2013/03/09 04:44:49, mikio wrote:
> can we have netpoll.h that contains those prototypes?


At some point I've put them into netpoll.h, but there was only 3 or 4 functions,
so I just moved it into runtime.h.
How strong do you feel about it?

[mikio, 2013-03-09 12:23:40]

https://codereview.appspot.com/7569043/diff/22001/src/pkg/net/fd_poll_runtime.go
File src/pkg/net/fd_poll_runtime.go (right):

https://codereview.appspot.com/7569043/diff/22001/src/pkg/net/fd_poll_runtime...
src/pkg/net/fd_poll_runtime.go:15: func runtime_pollServerInit()
ah, understood. thx.

https://codereview.appspot.com/7569043/diff/22001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/22001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:15: runtime·netpollinit(void)
fine with runtime.h.

[dvyukov, 2013-03-12 08:13:42]

ping
this holds another big piece of changes for soaking

[iant, 2013-03-12 13:55:24]

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c
File src/pkg/runtime/netpoll.c (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:14: // and associate fd with PollFD.
What is fd in this comment?

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:15: // An implementation must call the following
function to denote that an fd become ready.
s/become/is/

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:26: PollFD* link;	// in s.cache
Protected by s.Lock, I assume.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:62: net·runtime_pollServerInit(void)
Why not make this netpoll.goc and put it in package net?

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:68: net·runtime_pollOpen(intgo intfd, PollFD *fd,
intgo errno)
This is confusing.  In the net package, this function returns a pollServer. 
Here it returns a PollFD.  We should use the same name in both places.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:70: fd = allocFD();
I find this naming a bit confusing, as in this directory fd normally denotes a
file descriptor.  Perhaps pfd?

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:227: G **gp;
In this directory gp is normally G*, not G**.  Perhaps gpp?

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:262: void
I'm finding the gp argument to this function rather cryptic. Suggest a comment
here or above.

Also gp is normally a G*, but here it is a G**.  Suggest a different name, e.g.,
gpp.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:48: return 14;  // EFAULT just to mark out from
other errors
Can we make this actually EFAULT rather than 14?

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:59: runtime·netpoll(bool block)
What calls this?

[dvyukov, 2013-03-12 16:32:18]

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c
File src/pkg/runtime/netpoll.c (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:62: net·runtime_pollServerInit(void)
On 2013/03/12 13:55:24, iant wrote:
> Why not make this netpoll.goc and put it in package net?

Do you mean that it will just #include "../runtime/runtime.h"?
I did not think about such possibility. Will it work? I will try.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:68: net·runtime_pollOpen(intgo intfd, PollFD *fd,
intgo errno)
On 2013/03/12 13:55:24, iant wrote:
> This is confusing.  In the net package, this function returns a pollServer. 
> Here it returns a PollFD.  We should use the same name in both places.

Yes, it is confusing.
I would like to pass opaque 'uintptr ctx' (the memory is not garbage-collected,
so it's fine for Go to hold uintptr). But I reuse netFD.pollServer var for it,
so the types become inconsistent. I don't want to name fd descriptor pollServer
here as well.

I can add additional var to netFD, then pollServer will be used in
fd_poll_unix.go and this new var 'uintptr pfd' will be used in
fd_poll_runtime.go.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:70: fd = allocFD();
On 2013/03/12 13:55:24, iant wrote:
> I find this naming a bit confusing, as in this directory fd normally denotes a
> file descriptor.  Perhaps pfd?

Will fix

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:59: runtime·netpoll(bool block)
On 2013/03/12 13:55:24, iant wrote:
> What calls this?

It's called from not yet submitted https://codereview.appspot.com/7448048/
(I am forced to use double buffering)
There it's called by worked threads looking for work

[rsc, 2013-03-12 16:36:25]

On Tue, Mar 12, 2013 at 12:32 PM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.**com/7569043/diff/40001/src/**
>
pkg/runtime/netpoll.c<https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c>
> File src/pkg/runtime/netpoll.c (right):
>
> https://codereview.appspot.**com/7569043/diff/40001/src/**
>
pkg/runtime/netpoll.c#**newcode62<https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#newcode62>
> src/pkg/runtime/netpoll.c:62: net·runtime_pollServerInit(**void)
> On 2013/03/12 13:55:24, iant wrote:
>
>> Why not make this netpoll.goc and put it in package net?
>>
>
> Do you mean that it will just #include "../runtime/runtime.h"?
> I did not think about such possibility. Will it work? I will try.


By "put it in package net" I believe Ian meant writing 'package net' in the
.goc file. You cannot put a .goc file in the net directory. See time.goc
for an example.

Russ

[iant, 2013-03-12 16:37:55]

On 2013/03/12 16:36:25, rsc wrote:

> By "put it in package net" I believe Ian meant writing 'package net' in the
> .goc file. You cannot put a .goc file in the net directory. See time.goc
> for an example.

Yes.

Ian

[iant, 2013-03-12 16:40:12]

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c
File src/pkg/runtime/netpoll.c (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:68: net·runtime_pollOpen(intgo intfd, PollFD *fd,
intgo errno)
On 2013/03/12 16:32:18, dvyukov wrote:
> On 2013/03/12 13:55:24, iant wrote:
> > This is confusing.  In the net package, this function returns a pollServer. 
> > Here it returns a PollFD.  We should use the same name in both places.
> 
> Yes, it is confusing.
> I would like to pass opaque 'uintptr ctx' (the memory is not
garbage-collected,
> so it's fine for Go to hold uintptr). But I reuse netFD.pollServer var for it,
> so the types become inconsistent. I don't want to name fd descriptor
pollServer
> here as well.

Well, why not?  It's a pollserver or it isn't.  Either you are obscuring matters
in the net package or you are obscuring them here.

Perhaps we should pick a new name and use it in the net package and here.

[dvyukov, 2013-03-12 16:47:41]

On Tue, Mar 12, 2013 at 8:40 PM,  <iant@golang.org> wrote:
>
> https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c
> File src/pkg/runtime/netpoll.c (right):
>
>
https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
> src/pkg/runtime/netpoll.c:68: net·runtime_pollOpen(intgo intfd, PollFD
> *fd, intgo errno)
> On 2013/03/12 16:32:18, dvyukov wrote:
>>
>> On 2013/03/12 13:55:24, iant wrote:
>> > This is confusing.  In the net package, this function returns a
>
> pollServer.
>>
>> > Here it returns a PollFD.  We should use the same name in both
>
> places.
>
>> Yes, it is confusing.
>> I would like to pass opaque 'uintptr ctx' (the memory is not
>
> garbage-collected,
>>
>> so it's fine for Go to hold uintptr). But I reuse netFD.pollServer var
>
> for it,
>>
>> so the types become inconsistent. I don't want to name fd descriptor
>
> pollServer
>>
>> here as well.
>
>
> Well, why not?  It's a pollserver or it isn't.  Either you are obscuring
> matters in the net package or you are obscuring them here.
>
> Perhaps we should pick a new name and use it in the net package and
> here.


I will look into this.

netFD contains the following fileds that are used only in
fd_poll_unix.go but not used in fd_poll_runtine.go:
        // read and write deadlines
        rdeadline, wdeadline deadline
        // owned by fd wait server
        ncr, ncw int

So my plain is to make:
type netFD struct {
  ...
  pd pollDesc
}

and then define pollDesc in fd_poll_unix.go and in fd_poll_runtine.go
differently.

[rsc, 2013-03-12 16:50:51]

The overall structure seems fine. I agree with Ian's comments about names.
Please update defs_darwin.go and regenerate the header files using the recipe.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/defs_darwin...
File src/pkg/runtime/defs_darwin_386.h (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/defs_darwin...
src/pkg/runtime/defs_darwin_386.h:1: // Created by cgo -cdefs - DO NOT EDIT
I don't see any corresponding changes to defs_darwin.go in this CL.

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c
File src/pkg/runtime/netpoll.c (right):

https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:47: } s;
How about calling this pollcache?
lock(&pollcache) looks better than lock(&s);

The field can be named 'first' or something like that.

[dvyukov, 2013-03-12 18:32:22]

On Tue, Mar 12, 2013 at 8:47 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Mar 12, 2013 at 8:40 PM,  <iant@golang.org> wrote:
>>
>> https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c
>> File src/pkg/runtime/netpoll.c (right):
>>
>>
https://codereview.appspot.com/7569043/diff/40001/src/pkg/runtime/netpoll.c#n...
>> src/pkg/runtime/netpoll.c:68: net·runtime_pollOpen(intgo intfd, PollFD
>> *fd, intgo errno)
>> On 2013/03/12 16:32:18, dvyukov wrote:
>>>
>>> On 2013/03/12 13:55:24, iant wrote:
>>> > This is confusing.  In the net package, this function returns a
>>
>> pollServer.
>>>
>>> > Here it returns a PollFD.  We should use the same name in both
>>
>> places.
>>
>>> Yes, it is confusing.
>>> I would like to pass opaque 'uintptr ctx' (the memory is not
>>
>> garbage-collected,
>>>
>>> so it's fine for Go to hold uintptr). But I reuse netFD.pollServer var
>>
>> for it,
>>>
>>> so the types become inconsistent. I don't want to name fd descriptor
>>
>> pollServer
>>>
>>> here as well.
>>
>>
>> Well, why not?  It's a pollserver or it isn't.  Either you are obscuring
>> matters in the net package or you are obscuring them here.
>>
>> Perhaps we should pick a new name and use it in the net package and
>> here.
>
>
> I will look into this.
>
> netFD contains the following fileds that are used only in
> fd_poll_unix.go but not used in fd_poll_runtine.go:
>         // read and write deadlines
>         rdeadline, wdeadline deadline
>         // owned by fd wait server
>         ncr, ncw int
>
> So my plain is to make:
> type netFD struct {
>   ...
>   pd pollDesc
> }
>
> and then define pollDesc in fd_poll_unix.go and in fd_poll_runtine.go
> differently.


Sent https://codereview.appspot.com/7762044 separately with this refactoring.
After that CL, this CL will contain:

type pollDesc struct {
    runtimeCtx uintptr
}

[dvyukov, 2013-03-12 19:54:32]

On 2013/03/12 16:50:51, rsc wrote:
> The overall structure seems fine. I agree with Ian's comments about names.
> Please update defs_darwin.go and regenerate the header files using the recipe.


When I do:

--- a/src/pkg/runtime/defs1_linux.go	Tue Mar 12 21:14:26 2013 +0400
+++ b/src/pkg/runtime/defs1_linux.go	Tue Mar 12 23:53:12 2013 +0400
@@ -15,12 +15,29 @@
 /*
 #include <ucontext.h>
 #include <fcntl.h>
+#include <errno.h>
+#include <sys/epoll.h>
 */
 import "C"
 
 const (
+	EINTR		= C.EINTR
+	EAGAIN		= C.EAGAIN
+	ENOMEM		= C.ENOMEM
+
 	O_RDONLY  = C.O_RDONLY
 	O_CLOEXEC = C.O_CLOEXEC
+
+	EPOLLIN		= C.EPOLLIN
+	EPOLLOUT	= C.EPOLLOUT
+	EPOLLERR	= C.EPOLLERR
+	EPOLLHUP	= C.EPOLLHUP
+	EPOLLRDHUP	= C.EPOLLRDHUP
+	EPOLLET		= C.EPOLLET
+	EPOLL_CLOEXEC	= C.EPOLL_CLOEXEC
+	EPOLL_CTL_ADD	= C.EPOLL_CTL_ADD
+	EPOLL_CTL_DEL	= C.EPOLL_CTL_DEL
+	EPOLL_CTL_MOD	= C.EPOLL_CTL_MOD
 )
 
 type Usigset C.__sigset_t
@@ -35,3 +52,4 @@
 type Mcontext C.mcontext_t
 type Ucontext C.ucontext_t
 type Sigcontext C.struct_sigcontext
+type EpollEvent C.epoll_event


It says:
$ GOARCH=amd64 go tool cgo -cdefs defs1_linux.go
1: error: 'epoll_event' undeclared (first use in this function)
1: note: each undeclared identifier is reported only once for each function it
appears in

while the struct is there:

typedef union epoll_data
{
  void *ptr;
  int fd;
  uint32_t u32;
  uint64_t u64;
} epoll_data_t;

struct epoll_event
{
  uint32_t events;      /* Epoll events */
  epoll_data_t data;    /* User data variable */
} __attribute__ ((__packed__));

[rsc, 2013-03-12 19:58:02]

Try C.struct_epoll_event.

[dvyukov, 2013-03-12 20:17:48]

Addressed all comments, except autogenerated defs (need darwin machine for
that).
Please take a look at all other files except defs.

[dvyukov, 2013-03-12 20:20:02]

On 2013/03/12 20:17:48, dvyukov wrote:
> Addressed all comments, except autogenerated defs (need darwin machine for
> that).
> Please take a look at all other files except defs.

Also I will need to carefully check build tags because I am testing on several
OSes and there are cross-CL files.

[iant, 2013-03-12 20:52:06]

https://codereview.appspot.com/7569043/diff/59001/src/pkg/net/fd_poll_unix.go
File src/pkg/net/fd_poll_unix.go (right):

https://codereview.appspot.com/7569043/diff/59001/src/pkg/net/fd_poll_unix.go...
src/pkg/net/fd_poll_unix.go:5: // +build darwin freebsd netbsd openbsd
Is this +build line right?

https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll.c
File src/pkg/runtime/netpoll.c (right):

https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll.c#n...
src/pkg/runtime/netpoll.c:57: void
This is still not netpoll.goc.

https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:68: tp = nil;
If block is true, we will block forever.  But what if there is some goroutine
waiting on a timer?  What is going to get that goroutine to run if there is no
network I/O, when GOMAXPROCS == 1?

[dvyukov, 2013-03-13 12:04:24]

https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll_kqu...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll_kqu...
src/pkg/runtime/netpoll_kqueue.c:68: tp = nil;
On 2013/03/12 20:52:06, iant wrote:
> If block is true, we will block forever.  But what if there is some goroutine
> waiting on a timer?  What is going to get that goroutine to run if there is no
> network I/O, when GOMAXPROCS == 1?

netpoll is running outside of G and w/o P, it is called by otherwise idle worker
threads or by sysmon thread.
so it timer G exits from the sleep, it can acquire a spare P and continue
running Go code

[dvyukov, 2013-03-13 13:39:40]

On 2013/03/12 20:52:06, iant wrote:
>
https://codereview.appspot.com/7569043/diff/59001/src/pkg/net/fd_poll_unix.go...
> src/pkg/net/fd_poll_unix.go:5: // +build darwin freebsd netbsd openbsd
> Is this +build line right?


No, it is not


> https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll.c
> File src/pkg/runtime/netpoll.c (right):
> 
>
https://codereview.appspot.com/7569043/diff/59001/src/pkg/runtime/netpoll.c#n...
> src/pkg/runtime/netpoll.c:57: void
> This is still not netpoll.goc.


Yeah, sorry

[dvyukov, 2013-03-13 13:41:18]

PTAL

I've fixed all the comments
netpoll now goc
build tags are fixed
defs are generated

[iant, 2013-03-13 13:54:07]

LGTM

but see if rsc has any comments.

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

https://codereview.appspot.com/7569043/diff/84001/src/pkg/runtime/netpoll.goc...
src/pkg/runtime/netpoll.goc:147: pd->rt.arg.type = (Type*)pd->seq;
This needs a comment.

https://codereview.appspot.com/7569043/diff/84001/src/pkg/runtime/netpoll.goc...
src/pkg/runtime/netpoll.goc:274: seq = (uint32)(uintptr)arg.type;
This needs a comment.
Why cast to uint32?  The type of pd->seq is uintptr.

[dvyukov, 2013-03-13 14:37:15]

On 2013/03/13 13:54:07, iant wrote:
> LGTM
> 
> but see if rsc has any comments.
> 
> https://codereview.appspot.com/7569043/diff/84001/src/pkg/runtime/netpoll.goc
> File src/pkg/runtime/netpoll.goc (right):
> 
>
https://codereview.appspot.com/7569043/diff/84001/src/pkg/runtime/netpoll.goc...
> src/pkg/runtime/netpoll.goc:147: pd->rt.arg.type = (Type*)pd->seq;
> This needs a comment.
> 
>
https://codereview.appspot.com/7569043/diff/84001/src/pkg/runtime/netpoll.goc...
> src/pkg/runtime/netpoll.goc:274: seq = (uint32)(uintptr)arg.type;
> This needs a comment.
> Why cast to uint32?  The type of pd->seq is uintptr.

Done
Thanks!

[rsc, 2013-03-13 16:52:56]

LGTM after fixes below

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

https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc...
src/pkg/runtime/netpoll.goc:99: return;
Because of the way that goc2c works, you cannot return in the middle of a
function that has return values. The flushing of the return values only happens
at the closing } of the function. So this needs to be goto ret;, with a ret:
label at the bottom of the function. Or better you could goto unlock and put the
label before the unlock call.

https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc...
src/pkg/runtime/netpoll.goc:113: return;
This too.

https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc...
src/pkg/runtime/netpoll.goc:124: return;
This is fine because there are no return values.

[dvyukov, 2013-03-14 06:38:11]

On 2013/03/13 16:52:56, rsc wrote:
> LGTM after fixes below
> 
> https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc
> File src/pkg/runtime/netpoll.goc (right):
> 
>
https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc...
> src/pkg/runtime/netpoll.goc:99: return;
> Because of the way that goc2c works, you cannot return in the middle of a
> function that has return values. The flushing of the return values only
happens
> at the closing } of the function. So this needs to be goto ret;, with a ret:
> label at the bottom of the function. Or better you could goto unlock and put
the
> label before the unlock call.
> 
>
https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc...
> src/pkg/runtime/netpoll.goc:113: return;
> This too.
> 
>
https://codereview.appspot.com/7569043/diff/93013/src/pkg/runtime/netpoll.goc...
> src/pkg/runtime/netpoll.goc:124: return;
> This is fine because there are no return values.

Done.
I've fixed the third case as well for consistency.

[dvyukov, 2013-03-14 06:48:14]

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

runtime: integrated network poller for darwin
vs tip:
benchmark                           old ns/op    new ns/op    delta
BenchmarkTCP4Persistent                 67786        33175  -51.06%
BenchmarkTCP4Persistent-2               49085        31227  -36.38%
BenchmarkTCP4PersistentTimeout          69265        32565  -52.98%
BenchmarkTCP4PersistentTimeout-2        49217        32588  -33.79%

vs old scheduler:
benchmark                           old ns/op    new ns/op    delta
BenchmarkTCP4Persistent                 63517        33175  -47.77%
BenchmarkTCP4Persistent-2               54760        31227  -42.97%
BenchmarkTCP4PersistentTimeout          63234        32565  -48.50%
BenchmarkTCP4PersistentTimeout-2        56956        32588  -42.78%

R=golang-dev, bradfitz, devon.odell, mikioh.mikioh, iant, rsc
CC=golang-dev, pabuhr
https://codereview.appspot.com/7569043

[minux1, 2013-03-17 21:24:28]

https://codereview.appspot.com/7569043/diff/104001/src/pkg/runtime/netpoll_kq...
File src/pkg/runtime/netpoll_kqueue.c (right):

https://codereview.appspot.com/7569043/diff/104001/src/pkg/runtime/netpoll_kq...
src/pkg/runtime/netpoll_kqueue.c:40: ev[0].flags = EV_ADD|EV_RECEIPT|EV_CLEAR;
Of all the *BSDs out there, only FreeBSD and Darwin support EV_RECEIPT,
and on platform without such support, if we want to split registering
events and waiting for them into two kevent calls, we can't check for
errors (as if we provide non-nil eventlist to kevent, it will wait for
those events).

PS: 
I looked at several cross platform event handling libraries with kqueue
support, and all of them are managing array of (networking) kevent in
user space and register them at the same time waiting for them.

perhaps we should adopt a similar way for portability between *BSDs.

[pabuhr, 2013-03-18 03:20:42]

    Peter, can you take a look at this?

Hey Dmitry, I did see this message go by about a week ago, but I though you
were
referring to the other "Peter" on the Go team. So I ignored it. ;-)

I'm still learning a LOT about stuff here at Google and with the Go team. I
know it's late,
but I'll try to look through the code this week, and see if I can generate
any useful comments.

Sorry about the misunderstanding, and thank you can asking me to look at
the code.


On Fri, Mar 8, 2013 at 9:25 AM, <dvyukov@google.com> wrote:

> Peter, can you take a look at this?
>
>
https://codereview.appspot.**com/7569043/<https://codereview.appspot.com/7569...
>

[dvyukov, 2013-03-18 04:10:37]

On Mon, Mar 18, 2013 at 7:20 AM, Peter Buhr <pabuhr@google.com> wrote:
>     Peter, can you take a look at this?
>
> Hey Dmitry, I did see this message go by about a week ago, but I though you
> were
> referring to the other "Peter" on the Go team. So I ignored it. ;-)
>
> I'm still learning a LOT about stuff here at Google and with the Go team. I
> know it's late,
> but I'll try to look through the code this week, and see if I can generate
> any useful comments.
>
> Sorry about the misunderstanding, and thank you can asking me to look at the
> code.


Useful comments are never late :)

There are still some open questions:
- whether kqueue needs explicit fd unregistration (as epoll)?
- how to port kqueue to darwin/freebsd/netbsd/openbsd?
- is edge-triggered IO a good idea here?
- why my rblock idea
(https://codereview.appspot.com/7326051/diff/10002/src/pkg/net/fd_linux_amd64.go
line 260) does not work on linux in some conditions?