Hello rsc@golang.org, jsing@google.com, hectorchu@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
13 years, 4 months ago
(2011-10-26 16:47:55 UTC)
#1
The futex implementation is basically old Linux mutex implementation. The sema implementation is based on ...
13 years, 4 months ago
(2011-10-26 16:52:28 UTC)
#2
The futex implementation is basically old Linux mutex implementation.
The sema implementation is based on old Windows implementation, however it is
extended with spinning.
I've tried sema implementation on Linux, it sucks.
I've tested on Linux and Darwin. For others I merely run GOARCH=xxx ./make.bash
on Linux to where it runs.
On 2011/10/26 16:52:28, dvyukov wrote: > The futex implementation is basically old Linux mutex implementation. ...
13 years, 4 months ago
(2011-10-26 18:18:26 UTC)
#3
On 2011/10/26 16:52:28, dvyukov wrote:
> The futex implementation is basically old Linux mutex implementation.
> The sema implementation is based on old Windows implementation, however it is
> extended with spinning.
> I've tried sema implementation on Linux, it sucks.
> I've tested on Linux and Darwin. For others I merely run GOARCH=xxx
./make.bash
> on Linux to where it runs.
hector, jsing, I will appreciate if you try it on OpenBSD/Windows. Ideally
something like
src/pkg/runtime$ gotest -v -bench=.* -benchtime=.1 -cpu=1,2,3,4,5,6,7,8
On 2011/10/26 19:46:41, hector wrote: Thanks, Hector! It's a good sign that it does not ...
13 years, 4 months ago
(2011-10-27 07:56:50 UTC)
#7
On 2011/10/26 19:46:41, hector wrote:
Thanks, Hector! It's a good sign that it does not crash :)
Do you run all the benchmarks with -benchtime=.1? It would explain bad results
at least for GC/Finalizer related benchmarks, they require something like
-benchtine=3 to get more consistent results.
There is also some degradation on ChanSync, however I think it is OK because the
benchmark implies constant OS thread parking and unparking (it is a way too
synthetic). ChanProdCons (producer-consumer) is a more realistic benchmark, and
it shows good speedups.
It's interesting that CallClosure shows some speedups, does it involve locking?
humm...
AppendSpecialCase shows some degradation, I am not sure how it relates to
locking... it must be something else.
On 27 October 2011 08:56, <dvyukov@google.com> wrote: > Do you run all the benchmarks with ...
13 years, 4 months ago
(2011-10-27 08:00:26 UTC)
#8
On 27 October 2011 08:56, <dvyukov@google.com> wrote:
> Do you run all the benchmarks with -benchtime=.1?
Yes I ran the benchmark with the command line you described in a previous email.
http://codereview.appspot.com/5140043/diff/36001/src/pkg/runtime/lock_sema.c File src/pkg/runtime/lock_sema.c (right): http://codereview.appspot.com/5140043/diff/36001/src/pkg/runtime/lock_sema.c#newcode66 src/pkg/runtime/lock_sema.c:66: if((v&LOCKED) == 0) In this case can you avoid ...
13 years, 4 months ago
(2011-10-27 08:14:55 UTC)
#9
13 years, 4 months ago
(2011-10-27 08:42:21 UTC)
#11
On 2011/10/27 08:14:55, hector wrote:
> http://codereview.appspot.com/5140043/diff/36001/src/pkg/runtime/lock_sema.c
> File src/pkg/runtime/lock_sema.c (right):
>
>
http://codereview.appspot.com/5140043/diff/36001/src/pkg/runtime/lock_sema.c#...
> src/pkg/runtime/lock_sema.c:66: if((v&LOCKED) == 0)
> In this case can you avoid a double atomicloadp as it returns to the top of
the
> for loop? Most easiest way is to goto line 48.
Done.
>
http://codereview.appspot.com/5140043/diff/36001/src/pkg/runtime/lock_sema.c#...
> src/pkg/runtime/lock_sema.c:125: if(runtime·casp(&n->waitm, nil, m))
> Are notes only waited on by a single m at a time?
Yes, it was always that way. There was some confusion because note comments said
that several threads can wait, but some OSes always implement it in such a way
that at most 1 thread can wait. Now note comments reflect that:
/*
* sleep and wakeup on one-time events.
* before any calls to notesleep or notewakeup,
* must call noteclear to initialize the Note.
* then, exactly one thread can call notesleep
* and exactly one thread can call notewakeup (once).
* once notewakeup has been called, the notesleep
* will return. future notesleep will return immediately.
* subsequent noteclear must be called only after
* previous notesleep has returned, e.g. it's disallowed
* to call noteclear straight after notewakeup.
*/
void runtime·noteclear(Note*);
void runtime·notesleep(Note*);
void runtime·notewakeup(Note*);
LGTM, though I wish there weren't so many waitsema* vars in M. http://codereview.appspot.com/5140043/diff/28014/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h ...
13 years, 4 months ago
(2011-10-27 10:31:25 UTC)
#12
On 2011/10/27 10:31:25, hector wrote: > LGTM, though I wish there weren't so many waitsema* ...
13 years, 4 months ago
(2011-10-27 10:40:14 UTC)
#14
On 2011/10/27 10:31:25, hector wrote:
> LGTM, though I wish there weren't so many waitsema* vars in M.
Any suggestions?
semacreate() could allocate them dynamically, but it can't.
OpenBSD thread parking/unparking API is weird, it combines cons of both futexes
and semaphores and requires 2 vars.
> http://codereview.appspot.com/5140043/diff/28014/src/pkg/runtime/runtime.h
> File src/pkg/runtime/runtime.h (right):
>
>
http://codereview.appspot.com/5140043/diff/28014/src/pkg/runtime/runtime.h#ne...
> src/pkg/runtime/runtime.h:126: uint32 state; // futex-based imple
> impl. Also perhaps rename state to key in keeping with Lock?
Done.
Sorry Dmitry just one more thing I noticed, please see comments. I guess in future ...
13 years, 4 months ago
(2011-10-27 13:26:08 UTC)
#15
Sorry Dmitry just one more thing I noticed, please see comments. I guess in
future I should batch up my comments to save context switching time. :)
P.S. While looking at this code I couldn't find any documentation that stated
that futex_wake before futex_wait wakes up the waiter. I'm assuming that it
does, and that is probably why wakeups can be spurious.
http://codereview.appspot.com/5140043/diff/37012/src/pkg/runtime/lock_futex.c
File src/pkg/runtime/lock_futex.c (right):
http://codereview.appspot.com/5140043/diff/37012/src/pkg/runtime/lock_futex.c...
src/pkg/runtime/lock_futex.c:110: runtime·futexwakeup(&n->key, 1<<30);
Since no more than one thread sleeps on the futex, it should be ok to set cnt to
1 now.
On 2011/10/27 13:26:08, hector wrote: > Sorry Dmitry just one more thing I noticed, please ...
13 years, 4 months ago
(2011-10-27 14:26:42 UTC)
#17
On 2011/10/27 13:26:08, hector wrote:
> Sorry Dmitry just one more thing I noticed, please see comments. I guess in
> future I should batch up my comments to save context switching time. :)
np
> P.S. While looking at this code I couldn't find any documentation that stated
> that futex_wake before futex_wait wakes up the waiter. I'm assuming that it
> does, and that is probably why wakeups can be spurious.
Futex_wake has no effect if there is no waiters, that is, subsequent futex_wait
will *not* return immediately. But futex_wait rechecks the variable value after
adding the thread to waitlist.
> http://codereview.appspot.com/5140043/diff/37012/src/pkg/runtime/lock_futex.c
> File src/pkg/runtime/lock_futex.c (right):
>
>
http://codereview.appspot.com/5140043/diff/37012/src/pkg/runtime/lock_futex.c...
> src/pkg/runtime/lock_futex.c:110: runtime·futexwakeup(&n->key, 1<<30);
> Since no more than one thread sleeps on the futex, it should be ok to set cnt
to
> 1 now.
Done.
On Wed, Nov 2, 2011 at 5:21 PM, Joel Sing <jsing@google.com> wrote: > I think ...
13 years, 4 months ago
(2011-11-02 13:38:20 UTC)
#20
On Wed, Nov 2, 2011 at 5:21 PM, Joel Sing <jsing@google.com> wrote:
> I think I've just tracked it down - the results below look
> considerably better and are certainly more consistent. For the
> spinlock on m->waitsemalock you are using runtime·procyield(10)
> whereas I was previously using runtime·osyield() - changing this back
> gives the results below. I'm guessing that we are spending too much
> time spinning.
Done.
Issue 5140043: code review 5140043: runtime: unify mutex code across OSes
(Closed)
Created 13 years, 5 months ago by dvyukov
Modified 13 years, 4 months ago
Reviewers:
Base URL:
Comments: 4
