- Reduce the amount of messages being sent for Timer.run().

- Reduce the amount of messages being sent for Timer.run().
- Fast exit for empty timer queues.

R=sgjesse@google.com, zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43015

[Ivan Posva, 2015-01-20 09:26:39]

iposva@google.com changed reviewers:
+ sgjesse@google.com, zra@google.com

[zra, 2015-01-20 15:23:34]

lgtm

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:309: assert(!_heap.isEmpty);
Why is this assert needed?

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:315: (currentTime + 1) : _firstZeroTimer._wakeupTime;
Indentation

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:327: heapTime = _heap.isEmpty ? (currentTime + 1) :
_heap.first._wakeupTime;
Long line

[Søren Gjesse, 2015-01-20 15:36:27]

lgtm

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:293: if (_heap.isEmpty && (_firstZeroTimer == null))
{
maybe assert(_receivePort == null)

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:394: if (timer._repeating && (timer._callback !=
null)) {
I know this is not part of the change, but reading the code in
_advandeWakeupTime it looks as if the new timeout of the repeating timer could
already be timed-out, and even timed out before the tail in the list being
processed.

Isn't that a problem?

E.g.

Repeating timer have a wakeupTime of 200 ms before current time when
_handleTImeout is called. It has an interval of 100 ms. When it has been called
the new wakeupTime is still before the time when_handleTimeout was called, and
later timers can be on the collected list.

[Ivan Posva, 2015-01-20 16:01:17]

Thanks!

-Ivan

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart
File sdk/lib/io/timer_impl.dart (right):

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:293: if (_heap.isEmpty && (_firstZeroTimer == null))
{
On 2015/01/20 15:36:27, Søren Gjesse wrote:
> maybe assert(_receivePort == null)

Done.

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:309: assert(!_heap.isEmpty);
On 2015/01/20 15:23:34, zra wrote:
> Why is this assert needed?

It is more of a documentation and making sure the if-statement condition is not
changed.

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:315: (currentTime + 1) : _firstZeroTimer._wakeupTime;
On 2015/01/20 15:23:34, zra wrote:
> Indentation

Done.

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:327: heapTime = _heap.isEmpty ? (currentTime + 1) :
_heap.first._wakeupTime;
On 2015/01/20 15:23:34, zra wrote:
> Long line

Done.

https://codereview.chromium.org/858973002/diff/1/sdk/lib/io/timer_impl.dart#n...
sdk/lib/io/timer_impl.dart:394: if (timer._repeating && (timer._callback !=
null)) {
On 2015/01/20 15:36:26, Søren Gjesse wrote:
> I know this is not part of the change, but reading the code in
> _advandeWakeupTime it looks as if the new timeout of the repeating timer could
> already be timed-out, and even timed out before the tail in the list being
> processed.
> 
> Isn't that a problem?
> 
> E.g.
> 
> Repeating timer have a wakeupTime of 200 ms before current time when
> _handleTImeout is called. It has an interval of 100 ms. When it has been
called
> the new wakeupTime is still before the time when_handleTimeout was called, and
> later timers can be on the collected list.

I understand what you mean, but this is as if you have a repeating timer with a
0 repeat timeout. Newly scheduled timers only get a chance at the next run
through the event loop. There are no guarantees about ordering of timers, but we
do a best effort. There are similar issues about a repeating timer with a 10ms
that takes 15ms to process: You keep indefinitely falling behind. Not much we
can do about this at this point.

[Ivan Posva, 2015-01-20 16:02:37]

Committed patchset #1 (id:1) manually as r43015 (presubmit successful).