code review 136980044: runtime: convert chanrecv to Go

runtime: convert chanrecv to Go

[khr, 2014-08-29 18:47:03]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-08-29 18:56:31]

https://codereview.appspot.com/136980044/diff/60001/src/pkg/runtime/chan.go
File src/pkg/runtime/chan.go (right):

https://codereview.appspot.com/136980044/diff/60001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:326: func chanrecv(t *chantype, c *hchan, ep
unsafe.Pointer, block bool) (selected bool, received bool) {
(selected, received bool)

https://codereview.appspot.com/136980044/diff/60001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:338: return // not reached
panic("unreachable") ?

[rsc, 2014-08-30 04:11:32]

LGTM

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go
File src/pkg/runtime/chan.go (right):

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:330: println("chanrecv: chan=", c)
println puts spaces around everything, which is usually not what you want in
these prints.

print("chanrecv: chan=", c, "\n")

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:354: c.dataqsiz > 0 &&
atomicloadp(unsafe.Pointer(&c.qcount)) == nil) &&
might as well use atomicloaduintptr. at least that's not a real pointer.
but maybe we should have atomicloaduint too. sigh.

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:355: atomicload((*uint32)(unsafe.Pointer(&c.closed))) ==
0 {
atomicload(&c.closed) == 0 should work

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:367: if ep != nil {
If you're not going to use the goto then I think this should be pulled into a
separate function.

func recvclosed(c *hchan, ep unsafe.Pointer) (selected, received bool) {
    if ep != nil {
        memclr(ep, uintptr(c.elemsize))
    }
    if raceenabled {
        raceacquire(unsafe.Pointer(c))
    }
    unlock(&c.lock)
    return true, false
}

and then return recvclosed(c, ep) here and in the other places.

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:427: // a sender sent us some data. It already
unsplit short line

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:438: if ep != nil {
return recvclosed

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:454: if ep != nil {
if t1 > 0 {
...
}
return recvclosed

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.goc
File src/pkg/runtime/chan.goc (right):

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.goc#...
src/pkg/runtime/chan.goc:409: func reflect·chansend(t *ChanType, c *Hchan, elem
*byte, nb bool) (selected bool) {
can you move this and reflect.chanrecv over so that you can delete chansend and
chanrecv from this file?

[dvyukov, 2014-08-30 05:45:40]

LGTM

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go
File src/pkg/runtime/chan.go (right):

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:39: // and SudoG's are referenced from G so can't be
collected.
please update this comment

I see that you don't set gp.waiting in async chansend and it works, so I guess
SudoG's are actually preserved by stack references

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:166: mysg := acquireSudog()
add:

mysg.releasetime = 0

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:206: mysg := acquireSudog()
add:

mysg.releasetime = 0

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:406: mysg := acquireSudog()
add:

mysg.releasetime = 0

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:454: if ep != nil {
On 2014/08/30 04:11:31, rsc wrote:
> if t1 > 0 {
> ...
> }
> return recvclosed


blockevent is outside of critical section on purpose:

selected, received = recvclosed(c, ep)
if t1 > 0 {
    blockevent(t1-t0, 2)
}
return

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:475: mysg := acquireSudog()
add:

mysg.releasetime = 0

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:481: gp.waiting = mysg
you don't set gp.waiting=mysg in async chansend
remove it here for consistency

https://codereview.appspot.com/136980044/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:489: if mysg.releasetime != 0 {
please preserve the original check:

if mysg.releasetime > 0 {

[khr, 2014-08-30 18:03:32]

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

runtime: convert chanrecv to Go

LGTM=rsc, dvyukov
R=golang-codereviews, bradfitz, rsc, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/136980044