Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Fix timers #8120

Closed
wants to merge 4 commits into from
Closed

Fix timers #8120

wants to merge 4 commits into from

Conversation

loyd
Copy link

@loyd loyd commented Aug 9, 2014

Fixed:

  1. this within unref'd timer point to _handle, not timer.
  2. Now listOnTimeout updates now for every element of the list. It gives independence from the time of callbacks. See Wrong behavior of nested setTimeout #8133 and setTimeout drifts after busy wait #8105.
  3. Behavior of timers doesn't comply with browser version. See setInterval doesn't comply with DOM-version #8066.

@loyd loyd changed the title Fix set interval Fix setInterval Aug 9, 2014
@bnoordhuis
Copy link
Member

Behavior of timers doesn't comply with browser version.

What do browsers do when the callback overruns its time slot, i.e. what happens when the interval is 10 ms but the callback takes 12 ms to complete? Is every other interval dropped?

Have you measured the impact of backing all interval timers with a libuv handle? A handle is a weak persistent reference that requires special post-processing by the garbage collector. You might see a noticeable uptick in time spent inside the GC with this change.

@loyd
Copy link
Author

loyd commented Aug 10, 2014

What do browsers do when the callback overruns its time slot?

See the second ph in #8066.

Have you measured the impact of backing all interval timers with a libuv handle?

Increasing GC work definitely will be. However, I can not imagine a usercase where often creation and deletion of intervals is reasonably. If we start from the "effective everywhere" then let's use "idle
pattern" (one real handle per list) for unref'd timers too.

Moreover, little interest to the issue (for a long time) indicates that intervals are used in rarely.

@loyd
Copy link
Author

loyd commented Aug 11, 2014

Right version of "setInterval" based on "idle pattern" conflicts w/ tests of async_listener. (No matter after #8110).

@loyd loyd changed the title Fix setInterval Fix timers Aug 11, 2014
@indutny
Copy link
Member

indutny commented Aug 27, 2014

Hm... this is very likely to seriously impact timer performance...

@loyd
Copy link
Author

loyd commented Aug 27, 2014

@indutny Likely you're talking about f45eb2d. Are there any benchmarks to assess the actual loss?

@indutny
Copy link
Member

indutny commented Aug 27, 2014

@loyd, no I'm talking about calling out C++ methods from timers

@loyd
Copy link
Author

loyd commented Aug 27, 2014

@indutny Then, instead of Timer.now(), we can use Date.now() before calling callbacks (for updating diff) and Timer.now() before loop (or combination of Date.now() and Timer.updateTime()).

diff --git a/lib/timers.js b/lib/timers.js
index cf9f8b0..066246a 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -98,8 +98,10 @@ function listOnTimeout() {
   debug('timeout callback %d', msecs);

   var now, diff, first, hasQueue, threw;
+  var st = Date.now() - Timer.now();
+
   while (first = L.peek(list)) {
-    now = Timer.now();
+    now = Date.now() - st;
     debug('now: %d', now);
     diff = now - first._idleStart;

But test-timers-ordering accidentally falls (sometimes the diff is less than 1ms).

@loyd
Copy link
Author

loyd commented Aug 29, 2014

Ok, it happens because Date.now and Timer.now are shifted relative to each other in time (less ms) and Date.now() - Timer.now() eats the shift.
What if we replace Timer.now with Timer.updateTime + Date.now (all tests are passed)?

@trevnorris
Copy link

@indutny Calling Timer.now() adds another ~60ns in v0.12 and ~75ns in v0.10. The performance is about equal with Date.now(). I don't think this is a performance concern for the bug that's being fixed.

@loyd loyd mentioned this pull request Jan 13, 2015
@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

@trevnorris ... any further thoughts on this? Any reason to keep this open?

@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

Given the nature of this change, a new PR would need to be opened against master on http://github.com/nodejs/node if this is something that should be pursued. Closing this here.

@jasnell jasnell closed this Aug 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants