Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

move setImmediate from timer to uv_check #3872

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

shigeki commented Aug 16, 2012

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.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 16, 2012

src/node.cc
+ 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);
@bnoordhuis

bnoordhuis Aug 16, 2012

Owner

Use MakeCallback() here.

@shigeki

shigeki Aug 16, 2012

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

@shigeki

shigeki Aug 17, 2012

@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.

@bnoordhuis bnoordhuis commented on the diff Aug 16, 2012

src/node.cc
+
+ 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);
+ }
+}
+
@bnoordhuis

bnoordhuis Aug 16, 2012

Owner

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

@shigeki

shigeki Aug 16, 2012

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.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 16, 2012

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;
@bnoordhuis

bnoordhuis Aug 16, 2012

Owner

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

@shigeki

shigeki Aug 17, 2012

That's right. Removed.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 16, 2012

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
@bnoordhuis

bnoordhuis Aug 16, 2012

Owner

Superfluous comment.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 16, 2012

src/node.cc
@@ -2163,6 +2217,9 @@ static void DebugPortSetter(Local<String> property,
process->Set(String::NewSymbol("pid"), Integer::New(getpid()));
process->Set(String::NewSymbol("features"), GetFeatures());
+ process->SetAccessor(String::New("_needImmediateCallback"),
+ NeedImmediateCallbackGetter,
+ NeedImmediateCallbackSetter);
@bnoordhuis

bnoordhuis Aug 16, 2012

Owner

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

@shigeki

shigeki Aug 17, 2012

tabs were included. Fixed.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 16, 2012

src/node.cc
+
+ // 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 */
@bnoordhuis

bnoordhuis Aug 16, 2012

Owner

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

@shigeki

shigeki Aug 17, 2012

fixed. Thanks for your all comments.

Owner

bnoordhuis commented Aug 16, 2012

@tjfontaine You may want to chime in here.

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 commented Aug 17, 2012

@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

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 commented Aug 17, 2012

@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.

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 commented Aug 17, 2012

But I yield to @bnoordhuis and his judgement.

Me, too.

shigeki commented Aug 17, 2012

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.

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 24, 2012

src/node.cc
+}
+
+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);
@bnoordhuis

bnoordhuis Aug 24, 2012

Owner

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

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Aug 24, 2012

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) {
@bnoordhuis

bnoordhuis Aug 24, 2012

Owner

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

@shigeki

shigeki Aug 25, 2012

fixed. Thanks for reviewing, Ben.

Owner

bnoordhuis commented Aug 24, 2012

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 commented Sep 13, 2012

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 commented Feb 5, 2013

@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 Feb 5, 2013

Owner

bnoordhuis commented Feb 5, 2013

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 Feb 5, 2013

shigeki commented Feb 6, 2013

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

shigeki added some commits Feb 6, 2013

@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 commented Feb 16, 2013

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 Feb 16, 2013

shigeki commented Feb 16, 2013

@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