code review 130340043: runtime: convert synchronous semaphores to Go

runtime: convert synchronous semaphores to Go

[dvyukov, 2014-08-20 16:41:16]

Hello golang-codereviews@googlegroups.com (cc: khr@golang.org, rlh@golang.org,
rsc@golang.org),

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

[khr, 2014-08-20 18:12:44]

You're right about the sudoG-in-G problem.  I might need to make the stack
copier aware of the elem field.  Wait on this change until we get that sorted.

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.go
File src/pkg/runtime/sema.go (right):

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.go#n...
src/pkg/runtime/sema.go:99: if sz != unsafe.Sizeof(*(*syncSema)(nil)) {
unsafe.Sizeof(syncSema{})

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.goc
File src/pkg/runtime/sema.goc (right):

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.goc#...
src/pkg/runtime/sema.goc:212: func sync·runtime_Syncsemacquire(s *syncSema) {
These stubs are bad because they prohibit stack copying (if the call to
syncsemacquire overflows the stack).  Use assembly jump stubs like in the
reflect package to break through the package boundary.

[dvyukov, 2014-08-21 12:19:02]

This change in turn blocked on https://codereview.appspot.com/123640043/

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.goc
File src/pkg/runtime/sema.goc (right):

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.goc#...
src/pkg/runtime/sema.goc:212: func sync·runtime_Syncsemacquire(s *syncSema) {
On 2014/08/20 18:12:43, khr wrote:
> These stubs are bad because they prohibit stack copying (if the call to
> syncsemacquire overflows the stack).  Use assembly jump stubs like in the
> reflect package to break through the package boundary.

you're right

[dvyukov, 2014-08-22 20:56:30]

PTAL

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.go
File src/pkg/runtime/sema.go (right):

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.go#n...
src/pkg/runtime/sema.go:99: if sz != unsafe.Sizeof(*(*syncSema)(nil)) {
On 2014/08/20 18:12:43, khr wrote:
> unsafe.Sizeof(syncSema{})

Done.

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.goc
File src/pkg/runtime/sema.goc (right):

https://codereview.appspot.com/130340043/diff/40001/src/pkg/runtime/sema.goc#...
src/pkg/runtime/sema.goc:212: func sync·runtime_Syncsemacquire(s *syncSema) {
On 2014/08/21 12:19:02, dvyukov wrote:
> On 2014/08/20 18:12:43, khr wrote:
> > These stubs are bad because they prohibit stack copying (if the call to
> > syncsemacquire overflows the stack).  Use assembly jump stubs like in the
> > reflect package to break through the package boundary.
> 
> you're right

Done.

[rsc, 2014-08-22 22:55:48]

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

https://codereview.appspot.com/130340043/diff/80001/src/pkg/runtime/sema.go#n...
src/pkg/runtime/sema.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
Please run hg cp -A sema.goc sema.go and reupload, so that we can see the diffs
from what you started with.
Also since it is a copy, please copy the copyright notice from sema.goc, which
will have an earlier year.

I noticed that the blockprofile support in sema.goc is missing from this file
and then I stopped trying to compare them by hand.

[dvyukov, 2014-08-23 08:14:22]

On 2014/08/22 22:55:48, rsc wrote:
> https://codereview.appspot.com/130340043/diff/80001/src/pkg/runtime/sema.go
> File src/pkg/runtime/sema.go (right):
> 
>
https://codereview.appspot.com/130340043/diff/80001/src/pkg/runtime/sema.go#n...
> src/pkg/runtime/sema.go:1: // Copyright 2014 The Go Authors. All rights
> reserved.
> Please run hg cp -A sema.goc sema.go and reupload, so that we can see the
diffs
> from what you started with.
> Also since it is a copy, please copy the copyright notice from sema.goc, which
> will have an earlier year.
> 
> I noticed that the blockprofile support in sema.goc is missing from this file
> and then I stopped trying to compare them by hand.

Done.

Where blockprofile support is missing? sema.go calls goblockevent in acquire...

[rsc, 2014-08-23 21:21:27]

LGTM

https://codereview.appspot.com/130340043/diff/100001/src/pkg/runtime/sema.go
File src/pkg/runtime/sema.go (right):

https://codereview.appspot.com/130340043/diff/100001/src/pkg/runtime/sema.go#...
src/pkg/runtime/sema.go:86: releasetimep :=
(*int64)(unsafe.Pointer(&wake.releasetime))
// TODO: Remove use of unsafe here.

https://codereview.appspot.com/130340043/diff/100001/src/pkg/runtime/sema.go#...
src/pkg/runtime/sema.go:113: gothrow("bad syncSema size")
insert above this line:
print("runtime: bad syncSema size - sync=", sz, " runtime=",
unsafe.Sizeof(syncSema{}), "\n")

[dvyukov, 2014-08-24 08:36:26]

https://codereview.appspot.com/130340043/diff/100001/src/pkg/runtime/sema.go
File src/pkg/runtime/sema.go (right):

https://codereview.appspot.com/130340043/diff/100001/src/pkg/runtime/sema.go#...
src/pkg/runtime/sema.go:86: releasetimep :=
(*int64)(unsafe.Pointer(&wake.releasetime))
On 2014/08/23 21:21:27, rsc wrote:
> // TODO: Remove use of unsafe here.

Done.

https://codereview.appspot.com/130340043/diff/100001/src/pkg/runtime/sema.go#...
src/pkg/runtime/sema.go:113: gothrow("bad syncSema size")
On 2014/08/23 21:21:27, rsc wrote:
> insert above this line:
> print("runtime: bad syncSema size - sync=", sz, " runtime=",
> unsafe.Sizeof(syncSema{}), "\n")

Done.

[dvyukov, 2014-08-24 08:41:26]

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

runtime: convert synchronous semaphores to Go

LGTM=rsc
R=golang-codereviews, khr, rsc
CC=golang-codereviews, rlh
https://codereview.appspot.com/130340043