node: avoid automatic microtask runs

[vkurchatkin]

As discussed in #8325 we should take advantage of Isolate::SetAutorunMicrotasks. This requires running microtasks manually prior to checking tick_info->length() as process.nextTick can be called inside of a microtask.

This change helps to avoid unnecessary automatic microtask runs and should positively affect performance.
/cc @trevnorris

[trevnorris]

@vkurchatkin The additional tests are great. For posterity can you explain in a little more detail why not running microtasks automatically will help (e.g. the same number of nextTick callbacks need to be added, so what's the advantage)?

[vkurchatkin]

@trevnorris in fact it's not about some real advantage, but Doing The Right Thing™. Since we are taking control of microtask queue, it makes sense to disable autorun and only run microtasks when necessary.

If we just call isolate->SetAutorunMicrotasks(false); it would be break the tests, as _tickCallback wouldn't be called. Previously this case was handled by automatic microtask run after callback.

In case we have nextTick callbacks microtasks will run (with autorun enabled):

• after callback invocation;
• inside _tickCallback;
• after _tickCallback invocation.

The third one is totally unnecessary as microtask queue is guaranteed to be empty at this point. With autorun disabled we make first call manually and avoid the third call. There is a way to avoid the first one too:

if (tick_info->length() == 0) {
  env()->isolate()->RunMicrotasks();
  if (tick_info->length() == 0) {
    tick_info->set_index(0);
    return ret;
  }
 }

but I don't know if it's a good idea or not.

[trevnorris]

I don't think there's a need to stack the if checks:

if (tick_info->length() == 0) {
  env()->isolate()->RunMicrotasks();
}

if (tick_info->length() == 0) {
  tick_info->set_index(0);
  return ret;
}

And that seems a perfectly reasonable solution to me.

[trevnorris]

On the side, it does seem there's some sort of bug in V8 wrt the microtask scheduler. When running the tests in debug mode I get the following:

=== debug test-microtask-queue-integration ===                                 
Path: simple/test-microtask-queue-integration
#
# Fatal error in ../deps/v8/src/isolate.cc, line 2248
# CHECK(handle_scope_implementer()->CallDepthIsZero()) failed
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::internal::Isolate::RunMicrotasks()
 3: v8::Isolate::RunMicrotasks()
 4: node::RunMicrotasks(v8::FunctionCallbackInfo<v8::Value> const&)
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
 6: ??
 7: ??
 8: ??
 9: ??
Command: out/Debug/node /var/projects/node/test/simple/test-microtask-queue-integration.js
--- CRASHED ---
=== debug test-microtask-queue-integration-domain ===                    
Path: simple/test-microtask-queue-integration-domain
#
# Fatal error in ../deps/v8/src/isolate.cc, line 2248
# CHECK(handle_scope_implementer()->CallDepthIsZero()) failed
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::internal::Isolate::RunMicrotasks()
 3: v8::Isolate::RunMicrotasks()
 4: node::RunMicrotasks(v8::FunctionCallbackInfo<v8::Value> const&)
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
 6: ??
 7: ??
 8: ??
 9: ??
Command: out/Debug/node /var/projects/node/test/simple/test-microtask-queue-integration-domain.js
--- CRASHED ---

This happens before and after this patch. Running the same against V8 3.28 (#8476) they don't fail.

[trevnorris]

Merged it in 8dc6be1, updating the commit log with our conversation and adding the extra check in MakeCallback() to conditionally run the microtask queue.

Thanks for all your work on this.

[vkurchatkin]

And that seems a perfectly reasonable solution to me.

looks the same except in case when tick_info->length() != 0 second check is redundant