This looks interesting overall. As a name, Stop seems fine, since that's already used by ...
13 years, 3 months ago
(2011-01-19 13:03:07 UTC)
#2
This looks interesting overall.
As a name, Stop seems fine, since that's already used by Ticker.
Event might perhaps be better named as Timer.
Regarding the change in After()'s prototype, I'd prefer to avoid it.
The reason why After() was introduced is exactly to support the use
case which this change will prevent (hg log -v -r 6480). Hopefully
the select statement will grow internal support for timeouts at some
point, but this is a nice stopgap for now.
Also, from a different angle, After() currently matches the prototype
of Tick(), and getting the functionality of After+Stop is easy
with AfterFunc.
On 19 January 2011 13:03, <n13m3y3r@gmail.com> wrote: > This looks interesting overall. > > As ...
13 years, 3 months ago
(2011-01-19 13:10:52 UTC)
#3
On 19 January 2011 13:03, <n13m3y3r@gmail.com> wrote:
> This looks interesting overall.
>
> As a name, Stop seems fine, since that's already used by Ticker.
> Event might perhaps be better named as Timer.
yeah, maybe Timer is better.
i'll do that.
> Regarding the change in After()'s prototype, I'd prefer to avoid it.
> The reason why After() was introduced is exactly to support the use
> case which this change will prevent (hg log -v -r 6480). Hopefully
> the select statement will grow internal support for timeouts at some
> point, but this is a nice stopgap for now.
>
> Also, from a different angle, After() currently matches the prototype
> of Tick(), and getting the functionality of After+Stop is easy
> with AfterFunc.
not *quite* as AfterFunc always starts a new goroutine, whereas
After doesn't need to as it knows that it's writing to a buffered channel.
when events are being serviced very quickly, this could make a difference.
have you got an idea for a name for the two-valued After?
If events are being serviced very quickly there's no advantage in stopping them in terms ...
13 years, 3 months ago
(2011-01-19 13:32:19 UTC)
#4
If events are being serviced very quickly there's no advantage
in stopping them in terms of resource consumption. They won't go
away in either case before the next garbage collection.
If they are being stopped very quickly, the routine won't run
with AfterFunc either.
> have you got an idea for a name for the two-valued After?
A possible interface might revolve around a NewTimer function.
E.g.
t := NewTimer(ns)
Then, access to the channel, analogous to Ticker:
<-t.C
After behaves exactly like Tick:
return NewTimer(ns).C
PTAL. BTW one other possibility is to have After return a *Timer, and to have ...
13 years, 3 months ago
(2011-01-19 13:32:57 UTC)
#5
PTAL.
BTW one other possibility is to have After return a *Timer,
and to have the timer contain the channel (similar to Ticker).
then you'd do:
<-time.After(1e9).C
i'm slightly reluctant to do this, as it means the Timer type
uses an extra word of storage, and some people are talking
about having 100s of thousands of timers.
but maybe i should just bite the bullet.
On 19 January 2011 13:03, <n13m3y3r@gmail.com> wrote:
> This looks interesting overall.
>
> As a name, Stop seems fine, since that's already used by Ticker.
> Event might perhaps be better named as Timer.
>
> Regarding the change in After()'s prototype, I'd prefer to avoid it.
> The reason why After() was introduced is exactly to support the use
> case which this change will prevent (hg log -v -r 6480). Hopefully
> the select statement will grow internal support for timeouts at some
> point, but this is a nice stopgap for now.
>
> Also, from a different angle, After() currently matches the prototype
> of Tick(), and getting the functionality of After+Stop is easy
> with AfterFunc.
>
>
> http://codereview.appspot.com/4063043/
>
On 19 January 2011 13:32, <n13m3y3r@gmail.com> wrote: > If events are being serviced very quickly ...
13 years, 3 months ago
(2011-01-19 13:42:36 UTC)
#6
On 19 January 2011 13:32, <n13m3y3r@gmail.com> wrote:
> If events are being serviced very quickly there's no advantage
> in stopping them in terms of resource consumption. They won't go
> away in either case before the next garbage collection.
i'm talking about the case where there's a very fast loop
with a long timeout.
this will quickly use very large quantities of memory:
c := make(chan bool)
go func(){
for { c <- true }
}()
for {
select {
case <-time.After(300*1e9):
log.Exitln("timeout")
case <-c:
}
}
>> have you got an idea for a name for the two-valued After?
>
> A possible interface might revolve around a NewTimer function.
>
> E.g.
>
> t := NewTimer(ns)
>
> Then, access to the channel, analogous to Ticker:
>
> <-t.C
>
> After behaves exactly like Tick:
>
> return NewTimer(ns).C
we had a similar idea, it seems.
i think i might do this.
here's an odd anomaly: running gotest -benchmarks . in pkg/time results in all the concurrent ...
13 years, 3 months ago
(2011-01-19 14:57:03 UTC)
#8
here's an odd anomaly: running gotest -benchmarks . in
pkg/time results in all the concurrent benchmarks slowing
down *except* the Stop benchmark, which about triples
in speed (16056 to 5736 ns/op on my machine).
it seems to be due to lock contention, but why in this case only?
On 19 January 2011 14:20, <rogpeppe@gmail.com> wrote:
> Hello r, niemeyer (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/4063043/
>
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go File src/pkg/time/sleep.go (right): http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go#newcode16 src/pkg/time/sleep.go:16: // the current time will be sent on C; ...
13 years, 3 months ago
(2011-01-19 15:58:46 UTC)
#9
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go File src/pkg/time/sleep.go (right): http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go#newcode16 src/pkg/time/sleep.go:16: // the current time will be sent on C; ...
13 years, 3 months ago
(2011-01-19 16:25:08 UTC)
#10
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go
File src/pkg/time/sleep.go (right):
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go#newcode16
src/pkg/time/sleep.go:16: // the current time will be sent on C; otherwise
On 2011/01/19 15:58:46, niemeyer wrote:
> The current time won't be sent on C *when the Timer was created*.
Done.
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go#newcode96
src/pkg/time/sleep.go:96: }
On 2011/01/19 15:58:46, niemeyer wrote:
> What happens when Stop is called twice and, more importantly, what happens
with
> all of the indexes in the heap once e.i gets pulled off?
The only way that heap can remove items from the heap is through
heap.Interface.Pop, which is implemented by timerHeap.Pop, which sets the index
to -1. All the indexes in the heap are kept up to date by timerHeap.Swap (made
easy as a nice consequence of the way that the heap polymorphism works).
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go#newcod...
src/pkg/time/sleep.go:109: return &Timer{}
On 2011/01/19 15:58:46, niemeyer wrote:
> Feels like this should panic instead of silently misbehaving.
it's not really misbehaving. if the user asks for a timer to go off 500 years
into the future, i think it's fair enough that it will never actually go off.
they'll never know :-)
http://codereview.appspot.com/4063043/diff/33001/src/pkg/time/sleep.go#newcod...
src/pkg/time/sleep.go:120: if t0 > t && (t0 == forever || ns < maxSleepTime) {
On 2011/01/19 15:58:46, niemeyer wrote:
> There's a change being introduced here which looks
> irrelevant to introducing Stop, and which should
> probably be pushed in its own CL.
>
> With this change the sleeper will busy wait and wake
> up every second irrespective of the time requested,
> instead of actually sleeping, so if there's a timer
> scheduled for 10 minutes ahead, there will be a busy
> loop taking resources from the system for no reason
> for 10 minutes.
>
> I personally prefer the original approach.
i did mention this in the CL description.
waking up every second is not exactly busy-waiting and the actual interval is
configurable. maybe once every 5s would be more acceptable.
the problem is that if many events are being manipulated with long deadlines,
many goroutines will remain around (as actual OS threads) until the original
deadline has expired, even if the event has been stopped. it seems reasonable to
me that a sleeper occasionally checks to see if it has been cancelled. in the
future, either Go will provide an interface that allows a goroutine stuck in
syscall.Sleep to be interrupted or timing will get integrated into select and
all this will go. either way i feel this is a reasonable interim solution.
> The only way that heap can remove items from the heap is through > ...
13 years, 3 months ago
(2011-01-19 16:46:24 UTC)
#11
> The only way that heap can remove items from the heap is through
> heap.Interface.Pop, which is implemented by timerHeap.Pop, which sets the
index
> to -1. All the indexes in the heap are kept up to date by timerHeap.Swap (made
> easy as a nice consequence of the way that the heap polymorphism works).
I see, that makes sense.
> it's not really misbehaving. if the user asks for a timer to go off 500 years
> into the future, i think it's fair enough that it will never actually go off.
> they'll never know :-)
So if the user application has a bug, you're saying you don't
mind to add another one. panic() sounds appropriate.
> i did mention this in the CL description.
> waking up every second is not exactly busy-waiting and the actual interval is
> configurable. maybe once every 5s would be more acceptable.
Ok, let's call it dumb waiting then. The actual interval is configurable
by you, who are coding it, not by the user.
Intervals will generally be taken from a set rather than selected randomly.
In a short period, the smallest interval regularly scheduled will dominate
the algorithm with a single goroutine, with no waste at all.
> the problem is that if many events are being manipulated with long deadlines,
> many goroutines will remain around (as actual OS threads) until the original
The deadline doesn't matter.. the clock moves forward either way. If that
deadline is 10 minutes, the first call will schedule a goroutine for 10
minutes. The second call will have a time of 10 minutes + 1ns, and will
not create any goroutines anymore, and that will continue forever.
I vote against changing the current algorithm and introducing a dumb loop
which polls involving a global lock irrespective of the wait time.
On 19 January 2011 16:46, <n13m3y3r@gmail.com> wrote: > >> The only way that heap can ...
13 years, 3 months ago
(2011-01-19 17:15:21 UTC)
#12
On 19 January 2011 16:46, <n13m3y3r@gmail.com> wrote:
>
>> The only way that heap can remove items from the heap is through
>> heap.Interface.Pop, which is implemented by timerHeap.Pop, which sets
>
> the index
>>
>> to -1. All the indexes in the heap are kept up to date by
>
> timerHeap.Swap (made
>>
>> easy as a nice consequence of the way that the heap polymorphism
>
> works).
>
> I see, that makes sense.
>
>> it's not really misbehaving. if the user asks for a timer to go off
>
> 500 years
>>
>> into the future, i think it's fair enough that it will never actually
>
> go off.
>>
>> they'll never know :-)
>
> So if the user application has a bug, you're saying you don't
> mind to add another one. panic() sounds appropriate.
i wouldn't say it's necessarily a bug. why is 500 years in the future
a bug where 400 years is not? the real bug would be if ns wraps
negative, which i should probably check for and panic if so.
> Intervals will generally be taken from a set rather than selected
> randomly.
> In a short period, the smallest interval regularly scheduled will
> dominate
> the algorithm with a single goroutine, with no waste at all.
you're assuming a particular way of using time.After that may
or may not be the case. as a counter example, say i was writing
a calendar server, sending reminders to many events for many
people. when i start the server, perhaps i read all the events
from a file in which they're stored in date order into a linked list,
reversing it in the process. then i start a Timer for each event in the
list. this is perfectly legitimate usage - but there might be thousands of
items in the list, and each one is going
to create a real OS thread. cue one crashed application.
>> the problem is that if many events are being manipulated with long
>
> deadlines,
>>
>> many goroutines will remain around (as actual OS threads) until the
>
> original
>
> The deadline doesn't matter.. the clock moves forward either way. If
> that
> deadline is 10 minutes, the first call will schedule a goroutine for 10
> minutes. The second call will have a time of 10 minutes + 1ns, and will
> not create any goroutines anymore, and that will continue forever.
again, this assumes one particular pattern of usage.
> I vote against changing the current algorithm and introducing a dumb
> loop
> which polls involving a global lock irrespective of the wait time.
i don't understand this.
> i wouldn't say it's necessarily a bug. why is 500 years in the future ...
13 years, 3 months ago
(2011-01-19 17:28:31 UTC)
#13
> i wouldn't say it's necessarily a bug. why is 500 years in the future
> a bug where 400 years is not? the real bug would be if ns wraps
> negative, which i should probably check for and panic if so.
It's a bug because your implementation ignores the timer
entirely. If you don't handle it, the proper way is to
panic IMO.
> you're assuming a particular way of using time.After that may
Yes, I'm assuming a realistic use case, but that's just my
opinion. I'll leave it up to the core developers to decide.
In the test, AfterFunc()'s Timer seems to be ignored entirely.
Should it be tested as well?
Besides these points, LGTM.
On 19 January 2011 17:28, <n13m3y3r@gmail.com> wrote: > >> i wouldn't say it's necessarily a ...
13 years, 3 months ago
(2011-01-19 18:27:43 UTC)
#14
On 19 January 2011 17:28, <n13m3y3r@gmail.com> wrote:
>
>> i wouldn't say it's necessarily a bug. why is 500 years in the future
>> a bug where 400 years is not? the real bug would be if ns wraps
>> negative, which i should probably check for and panic if so.
>
> It's a bug because your implementation ignores the timer
> entirely. If you don't handle it, the proper way is to
> panic IMO.
if the not-handled behaviour is indistinguishable from the handled
behaviour for 500 years, i don't think we care too much.
actually, i've just realised that it doesn't matter if events are
inserted after the sentinel, so i've removed that check
and put in a check for wrapped ints. this way it works for
all [minint64 .. maxint64-now] which is about as good as
you can get. overkill really, but you never know where
a security hole might come from.
of course, we assume that we never get to 1<<62ns
so it still doesn't handle the event, same as before.
>> you're assuming a particular way of using time.After that may
>
> Yes, I'm assuming a realistic use case, but that's just my
> opinion. I'll leave it up to the core developers to decide.
>
> In the test, AfterFunc()'s Timer seems to be ignored entirely.
> Should it be tested as well?
done.
http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go File src/pkg/time/sleep.go (right): http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go#newcode15 src/pkg/time/sleep.go:15: // When the Timer was created with NewTimer, s/When/If/ ...
13 years, 3 months ago
(2011-01-21 19:26:53 UTC)
#16
http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go File src/pkg/time/sleep.go (right): http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go#newcode15 src/pkg/time/sleep.go:15: // When the Timer was created with NewTimer, A ...
13 years, 3 months ago
(2011-01-21 19:38:33 UTC)
#17
http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go File src/pkg/time/sleep.go (right): http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go#newcode15 src/pkg/time/sleep.go:15: // When the Timer was created with NewTimer, it's ...
13 years, 3 months ago
(2011-01-21 19:47:38 UTC)
#18
PTAL http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go File src/pkg/time/sleep.go (right): http://codereview.appspot.com/4063043/diff/53001/src/pkg/time/sleep.go#newcode15 src/pkg/time/sleep.go:15: // When the Timer was created with NewTimer, ...
13 years, 3 months ago
(2011-01-24 14:01:10 UTC)
#19
*** Submitted as http://code.google.com/p/go/source/detail?r=c1d8b16efebd *** time: allow cancelling of After events. Also simplify sleeper algorithm ...
13 years, 3 months ago
(2011-01-25 20:25:55 UTC)
#21
Issue 4063043: code review 4063043: time: allow cancelling of After events.
(Closed)
Created 13 years, 3 months ago by rog
Modified 13 years, 3 months ago
Reviewers:
Base URL:
Comments: 28