Do not drain worker tasks on main thread

[dinfuehr]

Hi!

This PR stops draining worker tasks on the main thread. I believe it's not necessary since V8 will join its own worker tasks as part of v8::Isolate::Dispose. It may also cause deadlocks now that those worker tasks may ask the main thread to perform a GC, see the discussion in this V8 issue here. This should help enable concurrent sparkplug again, which will get/is disabled with this PR.

Do you know why Node is draining worker tasks here? Am I seeing right that NodePlatform::DrainTasks is also invoked from the event loop here? If so, this might be a problem for performance as we block the main thread until those worker tasks are finished when the event loop is empty.

I have to admit that I didn't run tests yet for that PR and it may require V8 patches as well (see the V8 issue linked above) but I opened the PR already to discuss this.

[targos]

Are https://chromium-review.googlesource.com/c/v8/v8/+/4403395 and https://chromium-review.googlesource.com/c/v8/v8/+/4405689 necessary for this to work?

[dinfuehr]

Yes, they are necessary for this.

[joyeecheung]

I think @addaleax would be the best person to answer the questions since she wrote some of the initial patches of the migration to v8 platform, but my 2 cents:

Am I seeing right that NodePlatform::DrainTasks is also invoked from the event loop here?

Yes.

Do you know why Node is draining worker tasks here?

This seems to date back to the initial patches of V8 platform integration, but the same question was asked in https://github.com/v8/node/pull/69/files#r187129604 too. From some local testing it seems if we don't drain the worker tasks here (specifically, in the event loop), some foreground task (e.g. a wasm async compile task, which probably comes from the cjs-module-lexer) could get posted after the event loop already finishes, and the general processing order can be messed up - which is why there are a lot of exit code 13 failures in the test here, that means we have some leftover microtasks not yet cleared from the queue during shutdown. We run the microtasks after running foreground tasks here

node/src/node_platform.cc

Line 423 in 284e6ac

InternalCallbackScope cb_scope(env, Object::New(isolate_), { 0, 0 },

(in the InternalCallbackScope destructor) when the Node.js environment is still around e.g. when we are still running the event loop. (Personally I am a bit puzzled why we are using InternalCallbackScope for this because its destructor does..a lot of things, probably way more than what PerIsolatePlatformData::RunForegroundTask needs).

[aduh95]

It looks like there are linter and test failures, could you please reabase and address those?

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[github-actions]

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.