Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

move setImmediate from timer to uv_check #3872

Closed
wants to merge 2 commits into from

4 participants

@shigeki

uv_check is the robust place to invoke setImmediate callbacks after process.nextTick and before timers(setTimeout/setInterval) for the following two reason.

1. immediateTimer.start(0, 1) does not keep zero timeout value

The following test of setImmediate recursion shows its callbacks need time more than 1msec at every time. This is because of uv_timer_again() as commented in joyent/libuv@90a75b0#include-uv-h-P19 This would cause missing invocation of callbacks immediately after uv_poll().

function recur(n) {
  var i = 0, max = n * 100, start = Date.now();
  (function f() {
    if ( i++ < max) {
      setImmediate(f);
    } else {
      var etime = Date.now() - start;
      console.log('iter=', max,', elapsed time=', etime , ', ave=', etime/max);
      if (n < 10) recur(++n);
      return;
    }
  })();
}
recur(1);

The resut is

> ./node ~/setImmedaite_recur_test.js
iter= 100 , elapsed time= 130 , ave= 1.3
iter= 200 , elapsed time= 263 , ave= 1.315
iter= 300 , elapsed time= 397 , ave= 1.3233333333333333
iter= 400 , elapsed time= 528 , ave= 1.32
iter= 500 , elapsed time= 650 , ave= 1.3
iter= 600 , elapsed time= 780 , ave= 1.3
iter= 700 , elapsed time= 900 , ave= 1.2857142857142858
iter= 800 , elapsed time= 1039 , ave= 1.29875
iter= 900 , elapsed time= 1179 , ave= 1.31
iter= 1000 , elapsed time= 1308 , ave= 1.308

2. callback order changes when timeout value collides with another timers

It has a possibility for setImmediate to have the same timeout value as that of another timers. If this happens, the callback order is defined by the value of pointer of handles as
https://github.com/joyent/libuv/blob/master/src/unix/timer.c#L30-33
I think that setImmediate callback should be invoked after process.nextTick and before another timers. If not, the order has to be absolutely defined.

In both cases above, we can resolve them by fixing libuv, but I think that it is best to move the place of setImmediate to uv_check where old process.nextTick was invoked.

src/node.cc
((9 lines not shown))
+ if (!need_immediate_cb) return;
+
+ HandleScope scope;
+
+ if (immediate_callback_sym.IsEmpty()) {
+ // Lazily set the symbol
+ immediate_callback_sym = NODE_PSYMBOL("_immediateCallback");
+ }
+
+ Local<Value> cb_v = process->Get(immediate_callback_sym);
+ if (!cb_v->IsFunction()) return;
+ Local<Function> cb = Local<Function>::Cast(cb_v);
+
+ TryCatch try_catch;
+
+ cb->Call(process, 0, NULL);

Use MakeCallback() here.

@shigeki
shigeki added a note

Thanks.I fixed it and force updated the commit. Please review it again.

@shigeki
shigeki added a note

@bnoordhuis I found that MakeCallback() invokes process.nextTick() just after its execution while callbacks of setTimeout/setInterval does not. This might not cause any problems but there exists a different behavior between setImmediate and timers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on the diff
src/node.cc
((15 lines not shown))
+
+ if (need_immediate_cb == bool_value) return;
+
+ need_immediate_cb = bool_value;
+
+ if (need_immediate_cb) {
+ uv_check_start(&check_immediate_watcher, node::CheckImmediate);
+ uv_unref((uv_handle_t*) &check_immediate_watcher);
+ /* idle handle is needed only to maintain event loop */
+ uv_idle_start(&idle_immediate_dummy, node::IdleImmediateDummy);
+ } else {
+ uv_check_stop(&check_immediate_watcher);
+ uv_idle_stop(&idle_immediate_dummy);
+ }
+}
+

Why a getter/setter instead of process._needImmediateCallback()? (Like process._needTickCallback())

@shigeki
shigeki added a note

This is because setImmediate() needs to be canceled by clearImmediate().

I prefer process._needImmediateCallback = false rather than process._needImmediateCallback(false) so I used getter/setter.

Needs to line up with the String::New(...) statement.

@shigeki
shigeki added a note

tabs were included. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/node.cc
@@ -279,6 +284,28 @@ static void StartTickSpinner() {
return Undefined();
}
+static void CheckImmediate(uv_check_t* handle, int status) {
+ assert(handle == &check_immediate_watcher);
+ assert(status == 0);
+
+ // Avoid entering a V8 scope.
+ if (!need_immediate_cb) return;

Shouldn't you stop the check handle in that case?

@shigeki
shigeki added a note

That's right. Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/node.cc
@@ -279,6 +284,28 @@ static void StartTickSpinner() {
return Undefined();
}
+static void CheckImmediate(uv_check_t* handle, int status) {
+ assert(handle == &check_immediate_watcher);
+ assert(status == 0);
+
+ // Avoid entering a V8 scope.
+ if (!need_immediate_cb) return;
+
+ HandleScope scope;
+
+ if (immediate_callback_sym.IsEmpty()) {
+ // Lazily set the symbol

Superfluous comment.

@shigeki
shigeki added a note

Removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/node.cc
((7 lines not shown))
+
+ // Avoid entering a V8 scope.
+ if (!need_immediate_cb) return;
+
+ HandleScope scope;
+
+ if (immediate_callback_sym.IsEmpty()) {
+ // Lazily set the symbol
+ immediate_callback_sym = NODE_PSYMBOL("_immediateCallback");
+ }
+
+ MakeCallback(process, immediate_callback_sym, 0, NULL);
+}
+
+static void IdleImmediateDummy(uv_idle_t* handle, int status) {
+ /* Do nothing. Only for maintaining event loop */

You mix // and /* */ comments. // is preferred.

@shigeki
shigeki added a note

fixed. Thanks for your all comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis

@tjfontaine You may want to chime in here.

@tjfontaine
Owner

I guess the question is if IO should try and be serviced before the first queued immediate, if there are multiple queued immediates I don't see the placement explicitly after nextTick providing much besides saving a global timer allocation.

As far as the 1ms displacement, we could requeue the timer instead of relying on libuv to do it.

diff --git a/lib/timers.js b/lib/timers.js
index 897b64f..1009776 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -281,7 +281,7 @@ Timeout.prototype.close = function() {


 var immediateTimer = null;
-var immediateQueue = { started: false };
+var immediateQueue = {};
 L.init(immediateQueue);


@@ -294,10 +294,7 @@ function lazyImmediateInit() { // what's in a name?

 function processImmediate() {
   var immediate;
-  if (L.isEmpty(immediateQueue)) {
-    immediateTimer.stop();
-    immediateQueue.started = false;
-  } else {
+  if (!L.isEmpty(immediateQueue)) {
     immediate = L.shift(immediateQueue);

     if (immediate.domain) immediate.domain.enter();
@@ -305,6 +302,8 @@ function processImmediate() {
     immediate._onTimeout();

     if (immediate.domain) immediate.domain.exit();
+
+    immediateTimer.start(0, 0);
   }
 };

@@ -325,8 +324,7 @@ exports.setImmediate = function(callback) {

   if (!immediateQueue.started) {
     lazyImmediateInit();
-    immediateTimer.start(0, 1);
-    immediateQueue.started = true;
+    immediateTimer.start(0, 0);
   }

   if (process.domain) immediate.domain = process.domain;
@@ -343,9 +341,4 @@ exports.clearImmediate = function(immediate) {
   immediate._onTimeout = undefined;

   L.remove(immediate);
-
-  if (L.isEmpty(immediateQueue)) {
-    immediateTimer.stop();
-    immediateQueue.started = false;
-  }
 };

The results from the test script then are

iter= 100 , elapsed time= 3 , ave= 0.03
iter= 200 , elapsed time= 4 , ave= 0.02
iter= 300 , elapsed time= 1 , ave= 0.0033333333333333335
iter= 400 , elapsed time= 1 , ave= 0.0025
iter= 500 , elapsed time= 2 , ave= 0.004
iter= 600 , elapsed time= 2 , ave= 0.0033333333333333335
iter= 700 , elapsed time= 2 , ave= 0.002857142857142857
iter= 800 , elapsed time= 2 , ave= 0.0025
iter= 900 , elapsed time= 3 , ave= 0.0033333333333333335
iter= 1000 , elapsed time= 2 , ave= 0.002
@shigeki

@tjfontaine I drew a figure below to show the semantics of setImmediate in this PR. I think it is clearly understandable for the order of execution.

setImmediate semantics

@tjfontaine
Owner

I understand the flow, I just don't necessarily see how this influences much. Unless--as I said before--it's important that we service IO before the execution of the first queued immediate

@shigeki

@tjfontaine I think the problems would occur between setImmediate and other timers(setTimeout/setInterval). In the current setImmediate, before fix 1msec delay, the following code shows an odd result.

var i = 0, j = 0, max = 5;
var tmout = setInterval(function() {
  console.log('setInterval :', i++);
  if (i > max) clearTimeout(tmout);
}, 0);
(function f() {
  setImmediate(function () {
    console.log('setImmediate :', j++);
    if (j > max) return;
    f();
  });
})();
> ./node ~/test.js
setImmediate : 0
setInterval : 0
setInterval : 1
setImmediate : 1
setInterval : 2
setImmediate : 2
setInterval : 3
setImmediate : 3
setInterval : 4
setImmediate : 4
setInterval : 5
setImmediate : 5

The order of execution is changed in Linux. On Windows it varies time by time. This is because setImmediate and setInterval have the same timeout value and depends its order of execution on their pointer value.

I tried to put start_id on timer handles at every uv_timer_start for the second index to compare each elements in RBTree of timers but I found that moving setImmediate from timer to uv_check is simple and clearly understandable rather than it.

@tjfontaine
Owner

I do not disagree there is a deficiency in uv timers, and I'm sure they'll want to address that, but I think simply fixing my 1ms requeue mistake is sufficient compared to making setImmediate behave differently than the other timers.

But I yield to @bnoordhuis and his judgement.

@shigeki

But I yield to @bnoordhuis and his judgement.

Me, too.

@shigeki

I tried to put start_id on timer handles at every uv_timer_start for the second index to compare each elements in RBTree of timers but I found that moving setImmediate from timer to uv_check is simple and clearly understandable rather than it.

The patch is here. shigeki/libuv@50d55ed
But I don't think it is sophisticated.

src/node.cc
((7 lines not shown))
+}
+
+static void NeedImmediateCallbackSetter(Local<String> property,
+ Local<Value> value,
+ const AccessorInfo& info) {
+ HandleScope scope;
+
+ bool bool_value = value->BooleanValue();
+
+ if (need_immediate_cb == bool_value) return;
+
+ need_immediate_cb = bool_value;
+
+ if (need_immediate_cb) {
+ uv_check_start(&check_immediate_watcher, node::CheckImmediate);
+ uv_unref((uv_handle_t*) &check_immediate_watcher);

You can do this immediately after the call to uv_check_init().

@shigeki
shigeki added a note

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/node.cc
@@ -2070,6 +2093,33 @@ static void DebugPortSetter(Local<String> property,
static Handle<Value> DebugPause(const Arguments& args);
static Handle<Value> DebugEnd(const Arguments& args);
+Handle<Value> NeedImmediateCallbackGetter(Local<String> property,
+ const AccessorInfo& info) {

Needs to align. (Same for NeedImmediateCallbackSetter()).

@shigeki
shigeki added a note

fixed. Thanks for reviewing, Ben.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis

I'm not sure what the best behavior is here. Homogeneity is important (i.e. don't be different from other timers) but having a check watcher makes it nicely predictable.

@isaacs, @piscisaureus Opinions?

@shigeki

I've just rebased this to the current master and fixed a small bug.
@isaacs @piscisaureus @bnoordhuis Any more opinions? I'm sure that moving setImmediate to uv_check() is beneficial because it avoids future issues caused by racing conditions between timers and setImmediate.

@shigeki

@bnoordhuis node-v0.10 is around the corner. I wonder if this 1msec delay on setImmediate() cause bugs, they would be hard for users to be identified. I will not hold this PR any more and @tjfontaine 's patch for not using a repeat timer is still valid. So please fix it before releasing node-v0.10.

@shigeki shigeki closed this
@bnoordhuis

For the record, I like the idea of a check handle (pending joyent/libuv#699) and no one seems to really disagree. I move to go with your approach.

@bnoordhuis bnoordhuis reopened this
@shigeki

Oh, thanks very much, Ben. I'm going to rebase this to work on the current master.

shigeki added some commits
@shigeki shigeki timer: move setImmediate from timer to uv_check
uv_check is the robust place to invoke setImmediate callbacks after
process.nextTick and before timers(setTimeout/setInterval)
6bc47cd
@shigeki shigeki doc: add setImmediate execute timing description 00e1bfd
@isaacs
Owner

Landed on master. Thanks, @shigeki! By the way, I really love those process loop diagrams. Do you have them collected somewhere? Can we use them in the node documentation?

@bnoordhuis As far as I can tell, libuv#699 is an issue whether we land this or not, and we can fix it whenever. Moving forward.

@isaacs isaacs closed this
@shigeki

@isaacs Thanks. It's my pleasure to have your offer. I wrote a kind of more simpler diagram in the process.nextTick chapter of a Node book in Japanese for node-v0.8. I will check it again and submit PR to a doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 6, 2013
  1. @shigeki

    timer: move setImmediate from timer to uv_check

    shigeki authored
    uv_check is the robust place to invoke setImmediate callbacks after
    process.nextTick and before timers(setTimeout/setInterval)
  2. @shigeki
This page is out of date. Refresh to see the latest.
Showing with 81 additions and 27 deletions.
  1. +4 −3 doc/api/timers.markdown
  2. +15 −24 lib/timers.js
  3. +62 −0 src/node.cc
View
7 doc/api/timers.markdown
@@ -49,9 +49,10 @@ request the timer hold the program open. If the timer is already `ref`d calling
## setImmediate(callback, [arg], [...])
-To schedule the "immediate" execution of `callback`. Returns an `immediateId`
-for possible use with `clearImmediate()`. Optionally you can also pass
-arguments to the callback.
+To schedule the "immediate" execution of `callback` after I/O events
+callbacks and before `setTimeout` and `setInterval` . Returns an
+`immediateId` for possible use with `clearImmediate()`. Optionally you
+can also pass arguments to the callback.
Immediates are queued in the order created, and are popped off the queue once
per loop iteration. This is different from `process.nextTick` which will
View
39 lib/timers.js
@@ -292,29 +292,21 @@ Timeout.prototype.close = function() {
};
-var immediateTimer = null;
-var immediateQueue = { started: false };
+var immediateQueue = {};
L.init(immediateQueue);
-function lazyImmediateInit() { // what's in a name?
- if (immediateTimer) return;
- immediateTimer = new Timer;
- immediateTimer.ontimeout = processImmediate;
-}
-
-
function processImmediate() {
- var immediate;
+ var immediate = L.shift(immediateQueue);
+
if (L.isEmpty(immediateQueue)) {
- immediateTimer.stop();
- immediateQueue.started = false;
- } else {
- immediate = L.shift(immediateQueue);
+ process._needImmediateCallback = false;
+ }
+ if (immediate._onImmediate) {
if (immediate.domain) immediate.domain.enter();
- immediate._onTimeout();
+ immediate._onImmediate();
if (immediate.domain) immediate.domain.exit();
}
@@ -326,19 +318,19 @@ exports.setImmediate = function(callback) {
L.init(immediate);
- immediate._onTimeout = callback;
+ immediate._onImmediate = callback;
if (arguments.length > 1) {
args = Array.prototype.slice.call(arguments, 1);
- immediate._onTimeout = function() {
+
+ immediate._onImmediate = function() {
callback.apply(null, args);
};
}
- if (!immediateQueue.started) {
- lazyImmediateInit();
- immediateTimer.start(0, 1);
- immediateQueue.started = true;
+ if (!process._needImmediateCallback) {
+ process._needImmediateCallback = true;
+ process._immediateCallback = processImmediate;
}
if (process.domain) immediate.domain = process.domain;
@@ -352,12 +344,11 @@ exports.setImmediate = function(callback) {
exports.clearImmediate = function(immediate) {
if (!immediate) return;
- immediate._onTimeout = undefined;
+ immediate._onImmediate = undefined;
L.remove(immediate);
if (L.isEmpty(immediateQueue)) {
- immediateTimer.stop();
- immediateQueue.started = false;
+ process._needImmediateCallback = false;
}
};
View
62 src/node.cc
@@ -134,6 +134,11 @@ static uv_idle_t tick_spinner;
static bool need_tick_cb;
static Persistent<String> tick_callback_sym;
+static uv_check_t check_immediate_watcher;
+static uv_idle_t idle_immediate_dummy;
+static bool need_immediate_cb;
+static Persistent<String> immediate_callback_sym;
+
#ifdef OPENSSL_NPN_NEGOTIATED
static bool use_npn = true;
@@ -211,6 +216,27 @@ static Handle<Value> NeedTickCallback(const Arguments& args) {
}
+static void CheckImmediate(uv_check_t* handle, int status) {
+ assert(handle == &check_immediate_watcher);
+ assert(status == 0);
+
+ HandleScope scope;
+
+ if (immediate_callback_sym.IsEmpty()) {
+ immediate_callback_sym = NODE_PSYMBOL("_immediateCallback");
+ }
+
+ MakeCallback(process, immediate_callback_sym, 0, NULL);
+}
+
+
+static void IdleImmediateDummy(uv_idle_t* handle, int status) {
+ // Do nothing. Only for maintaining event loop
+ assert(handle == &idle_immediate_dummy);
+ assert(status == 0);
+}
+
+
static inline const char *errno_string(int errorno) {
#define ERRNO_CASE(e) case e: return #e;
switch (errorno) {
@@ -2184,6 +2210,35 @@ static Handle<Value> DebugProcess(const Arguments& args);
static Handle<Value> DebugPause(const Arguments& args);
static Handle<Value> DebugEnd(const Arguments& args);
+
+Handle<Value> NeedImmediateCallbackGetter(Local<String> property,
+ const AccessorInfo& info) {
+ return Boolean::New(need_immediate_cb);
+}
+
+
+static void NeedImmediateCallbackSetter(Local<String> property,
+ Local<Value> value,
+ const AccessorInfo& info) {
+ HandleScope scope;
+
+ bool bool_value = value->BooleanValue();
+
+ if (need_immediate_cb == bool_value) return;
+
+ need_immediate_cb = bool_value;
+
+ if (need_immediate_cb) {
+ uv_check_start(&check_immediate_watcher, node::CheckImmediate);
+ // idle handle is needed only to maintain event loop
+ uv_idle_start(&idle_immediate_dummy, node::IdleImmediateDummy);
+ } else {
+ uv_check_stop(&check_immediate_watcher);
+ uv_idle_stop(&idle_immediate_dummy);
+ }
+}
+

Why a getter/setter instead of process._needImmediateCallback()? (Like process._needTickCallback())

@shigeki
shigeki added a note

This is because setImmediate() needs to be canceled by clearImmediate().

I prefer process._needImmediateCallback = false rather than process._needImmediateCallback(false) so I used getter/setter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
Handle<Object> SetupProcessObject(int argc, char *argv[]) {
HandleScope scope;
@@ -2277,6 +2332,9 @@ Handle<Object> SetupProcessObject(int argc, char *argv[]) {
process->Set(String::NewSymbol("pid"), Integer::New(getpid(), node_isolate));
process->Set(String::NewSymbol("features"), GetFeatures());
+ process->SetAccessor(String::New("_needImmediateCallback"),
+ NeedImmediateCallbackGetter,
+ NeedImmediateCallbackSetter);
// -e, --eval
if (eval_string) {
@@ -2859,6 +2917,10 @@ char** Init(int argc, char *argv[]) {
uv_idle_init(uv_default_loop(), &tick_spinner);
+ uv_check_init(uv_default_loop(), &check_immediate_watcher);
+ uv_unref((uv_handle_t*) &check_immediate_watcher);
+ uv_idle_init(uv_default_loop(), &idle_immediate_dummy);
+
V8::SetFatalErrorHandler(node::OnFatalError);
// Fetch a reference to the main isolate, so we have a reference to it
Something went wrong with that request. Please try again.