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/
10 years, 6 months ago
(2014-08-22 18:07:02 UTC)
#1
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.goc#newcode42 src/pkg/runtime/netpoll.goc:42: // time.go knows the layout of this structure. On ...
10 years, 6 months ago
(2014-08-25 11:43:31 UTC)
#5
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#newcode1 src/pkg/runtime/time.go:1: // Copyright 2014 The Go Authors. All rights reserved. ...
10 years, 6 months ago
(2014-08-25 11:52:03 UTC)
#7
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#newcode48 ...
10 years, 6 months ago
(2014-08-25 13:43:08 UTC)
#9
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.
On 2014/08/25 13:43:08, dvyukov wrote: > On 2014/08/25 13:37:33, DMorsing wrote: > > On 2014/08/25 ...
10 years, 6 months ago
(2014-08-25 13:54:45 UTC)
#10
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.
On 2014/08/25 11:44:34, dvyukov wrote: > On 2014/08/24 18:43:26, rsc wrote: > > LGTM > ...
10 years, 6 months ago
(2014-08-25 16:17:03 UTC)
#11
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
LGTM You're right about siftdownTimer. My reasoning was that the C code didn't have the ...
10 years, 6 months ago
(2014-08-25 16:18:31 UTC)
#12
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.
Issue 123700044: code review 123700044: runtime: convert timers to Go
(Closed)
Created 10 years, 7 months ago by dvyukov
Modified 10 years, 6 months ago
Reviewers: gobot
Base URL:
Comments: 35