code review 123700044: runtime: convert timers to Go

runtime: convert timers to Go

[dvyukov, 2014-08-22 18:07:02]

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

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

[ruiu, 2014-08-23 20:41:40]

I don't have enough knowledge on this code so take the
comments with a grain of salt.

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/mgc0.c#n...
src/pkg/runtime/mgc0.c:2117: 
remove this line

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:50: t.period = 0
you don't need this line

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:69: return deltimer(t)
can you inline deltimer here and remove deltimer?

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:167: t.when += t.period * (1 + -delta/t.period)
I don't understand this code, although this is not new. Is rhs of * supposed to
be 0 or 1? If so, can we write like this?

if delta < t.period {
  t.when += t.period
}

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:188: f(arg)
Are f and arg needed? Seems you can write t.f(t.arg) here.

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:218: t[i] = t[p]
I'd use multi-valued assignment.
t[i], t[p] = t[p], tmp

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:256: t[i] = t[c]
Multi-valued assignment

[dvyukov, 2014-08-24 16:28:18]

PTAL

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/mgc0.c#n...
src/pkg/runtime/mgc0.c:2117: 
On 2014/08/23 20:41:40, ruiu wrote:
> remove this line

it's kind of kernel C style, we always put an empty line between var decls and
code

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:50: t.period = 0
On 2014/08/23 20:41:40, ruiu wrote:
> you don't need this line

Done.

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:69: return deltimer(t)
On 2014/08/23 20:41:40, ruiu wrote:
> can you inline deltimer here and remove deltimer?

runtime.deltimer is called from netpoll.goc

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:167: t.when += t.period * (1 + -delta/t.period)
On 2014/08/23 20:41:40, ruiu wrote:
> I don't understand this code, although this is not new. Is rhs of * supposed
to
> be 0 or 1? If so, can we write like this?
> 
> if delta < t.period {
>   t.when += t.period
> }

yes, I've just copied it, and it does not look trivial to refactor it along the
way
it seems to me that rhs can well be 1, 2, 3, 4, ....

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:188: f(arg)
On 2014/08/23 20:41:40, ruiu wrote:
> Are f and arg needed? Seems you can write t.f(t.arg) here.

t can be smashed as soon as we unlock timers

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:218: t[i] = t[p]
On 2014/08/23 20:41:40, ruiu wrote:
> I'd use multi-valued assignment.
> t[i], t[p] = t[p], tmp

Done.

https://codereview.appspot.com/123700044/diff/260001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:256: t[i] = t[c]
On 2014/08/23 20:41:40, ruiu wrote:
> Multi-valued assignment

Done.

[rsc, 2014-08-24 18:43:26]

LGTM

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

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.goc:42: // time.go knows the layout of this structure.
// time.go defines the layout of this structure.
// Keep in sync with time.go.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
hg cp -A and restore the old copyright notice.
You are modifying a file, not creating a new one. Make that clear in the hg
history and in the code review.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:12: // If this struct changes, adjust
../time/sleep.go:/runtimeTimer and netpoll.goc:timer.
change :timer to :/timer
These are acme addresses.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:43: func sleep(ns int64) {
There is already a symbol named runtime.sleep on Plan 9 and on Windows. This
must be named something else.
timeSleep maybe.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:48: t := new(timer)
It seems unfortunate to make time.Sleep start allocating but okay.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:53: addtimer1(t)
rename addtimer1 to addtimerLocked

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:134: timers.t = timers.t[:last]
before this line:
timers.t[last] = nil

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:175: timers.t = timers.t[:last]
above this line
timers.t[last] = nil

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:176: if last > 0 {
delete if, keep body. matches old code.
siftdownTimer handles len == 0 correctly.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:217: t[i], t[p] = t[p], tmp
i know someone else told you to do this, but don't.
unless the values being assigned are correlated in some standard way, like a
swap or splitting a slice, it's more confusing to see independent things jumbled
on one line.
put it back the way it was

t[i] = t[p]
t[i].i = i
t[p] = tmp
t[p].i = p
i = p

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:253: t[i], t[c] = t[c], tmp
t[i] = t[c]
t[i].i = i
t[c] = tmp
t[c].i = c
i = c

[dvyukov, 2014-08-25 11:43:31]

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

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/netpoll....
src/pkg/runtime/netpoll.goc:42: // time.go knows the layout of this structure.
On 2014/08/24 18:43:25, rsc wrote:
> // time.go defines the layout of this structure.
> // Keep in sync with time.go.

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:12: // If this struct changes, adjust
../time/sleep.go:/runtimeTimer and netpoll.goc:timer.
On 2014/08/24 18:43:25, rsc wrote:
> change :timer to :/timer
> These are acme addresses.

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:43: func sleep(ns int64) {
On 2014/08/24 18:43:25, rsc wrote:
> There is already a symbol named runtime.sleep on Plan 9 and on Windows. This
> must be named something else.
> timeSleep maybe.

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:53: addtimer1(t)
On 2014/08/24 18:43:25, rsc wrote:
> rename addtimer1 to addtimerLocked

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:134: timers.t = timers.t[:last]
On 2014/08/24 18:43:25, rsc wrote:
> before this line:
> timers.t[last] = nil

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:175: timers.t = timers.t[:last]
On 2014/08/24 18:43:25, rsc wrote:
> above this line
> timers.t[last] = nil

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:176: if last > 0 {
On 2014/08/24 18:43:25, rsc wrote:
> delete if, keep body. matches old code.
> siftdownTimer handles len == 0 correctly.

Is it?
Check out https://codereview.appspot.com/5437060/
if len(timers.t)==0, siftdown(0) will crash with out-of-bounds access

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:217: t[i], t[p] = t[p], tmp
On 2014/08/24 18:43:25, rsc wrote:
> i know someone else told you to do this, but don't.
> unless the values being assigned are correlated in some standard way, like a
> swap or splitting a slice, it's more confusing to see independent things
jumbled
> on one line.
> put it back the way it was
> 
> t[i] = t[p]
> t[i].i = i
> t[p] = tmp
> t[p].i = p
> i = p

Done.

[dvyukov, 2014-08-25 11:44:34]

On 2014/08/24 18:43:26, rsc wrote:
> LGTM


Russ, please check siftdownTimer call in timerproc. I have not adjusted it
according to your comments.

[dvyukov, 2014-08-25 11:52:03]

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go
File src/pkg/runtime/time.go (right):

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
On 2014/08/24 18:43:25, rsc wrote:
> hg cp -A and restore the old copyright notice.
> You are modifying a file, not creating a new one. Make that clear in the hg
> history and in the code review.

Done.

https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
src/pkg/runtime/time.go:48: t := new(timer)
On 2014/08/24 18:43:25, rsc wrote:
> It seems unfortunate to make time.Sleep start allocating but okay.
> 

Acknowledged.

[DMorsing, 2014-08-25 13:37:33]

On 2014/08/25 11:52:03, dvyukov wrote:
>
https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
> src/pkg/runtime/time.go:48: t := new(timer)
> On 2014/08/24 18:43:25, rsc wrote:
> > It seems unfortunate to make time.Sleep start allocating but okay.
> > 
> 
> Acknowledged.

Per-G timer struct?

[dvyukov, 2014-08-25 13:43:08]

On 2014/08/25 13:37:33, DMorsing wrote:
> On 2014/08/25 11:52:03, dvyukov wrote:
> >
>
https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
> > src/pkg/runtime/time.go:48: t := new(timer)
> > On 2014/08/24 18:43:25, rsc wrote:
> > > It seems unfortunate to make time.Sleep start allocating but okay.
> > > 
> > 
> > Acknowledged.
> 
> Per-G timer struct?

I think it will do more harm as we will have a timer allocated always, and most
goroutines do not ever sleep.
We could merge timer and sudog, and use the per-P sudog pool here. But I don't
want to do it in this CL.

[DMorsing, 2014-08-25 13:54:45]

On 2014/08/25 13:43:08, dvyukov wrote:
> On 2014/08/25 13:37:33, DMorsing wrote:
> > On 2014/08/25 11:52:03, dvyukov wrote:
> > >
> >
>
https://codereview.appspot.com/123700044/diff/290001/src/pkg/runtime/time.go#...
> > > src/pkg/runtime/time.go:48: t := new(timer)
> > > On 2014/08/24 18:43:25, rsc wrote:
> > > > It seems unfortunate to make time.Sleep start allocating but okay.
> > > > 
> > > 
> > > Acknowledged.
> > 
> > Per-G timer struct?
> 
> I think it will do more harm as we will have a timer allocated always, and
most
> goroutines do not ever sleep.
> We could merge timer and sudog, and use the per-P sudog pool here. But I don't
> want to do it in this CL.

OK. I might have a look at it after this lands.

[dvyukov, 2014-08-25 16:17:03]

On 2014/08/25 11:44:34, dvyukov wrote:
> On 2014/08/24 18:43:26, rsc wrote:
> > LGTM
> 
> 
> Russ, please check siftdownTimer call in timerproc. I have not adjusted it
> according to your comments.

Right

func main() {
	<-time.NewTimer(time.Second).C
}

crashes with "panic: runtime error: index out of range" without the 'if'
most likely previously it could corrupt timer heap as the deltimer code before
the fix

[rsc, 2014-08-25 16:18:31]

LGTM

You're right about siftdownTimer. My reasoning was that the C code didn't have
the check, but the C code is taking advantage of the fact that it can access [0]
even when timers.len has just been decremented 0. (The access would not cause a
problem if it were allowed, it's just that Go's bounds check disallows it.)

Thanks.

[dvyukov, 2014-08-25 16:25:28]

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

runtime: convert timers to Go

LGTM=rsc
R=golang-codereviews, ruiu, rsc, daniel.morsing
CC=golang-codereviews, khr
https://codereview.appspot.com/123700044

[gobot, 2014-08-26 02:09:54]

This CL appears to have broken the nacl-amd64p32 builder.
See http://build.golang.org/log/7a4cbaded34ea1a3a98622a15208f0af73529062