src: use v8 fast api on runMicroTasks()

[anonrig]

I'm having crashes due to GetCreationContextChecked(). Any suggestions? @joyeecheung @legendecas

Example crash:

=== release test-file-write-stream4 ===
Path: parallel/test-file-write-stream4
FATAL ERROR: v8::Object::GetCreationContextChecked No creation context available
----- Native stack trace -----

 1: 0x1045df464 node::OnFatalError(char const*, char const*) [/Users/yagiz/coding/node/out/Release/node]
 2: 0x1047c0ec8 v8::Object::GetCreationContextChecked() [/Users/yagiz/coding/node/out/Release/node]
 3: 0x1046900fc node::task_queue::FastRunMicrotasks(v8::Local<v8::Object>) [/Users/yagiz/coding/node/out/Release/node]
 4: 0x10a364c70
 5: 0x1050a28ac Builtins_JSEntryTrampoline [/Users/yagiz/coding/node/out/Release/node]
 6: 0x1050a2594 Builtins_JSEntry [/Users/yagiz/coding/node/out/Release/node]
 7: 0x10491fdbc v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) [/Users/yagiz/coding/node/out/Release/node]
 8: 0x10491f6dc v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [/Users/yagiz/coding/node/out/Release/node]
 9: 0x1047c20e0 v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*) [/Users/yagiz/coding/node/out/Release/node]
10: 0x1044ff138 node::InternalCallbackScope::Close() [/Users/yagiz/coding/node/out/Release/node]
11: 0x1044ff390 node::InternalMakeCallback(node::Environment*, v8::Local<v8::Object>, v8::Local<v8::Object>, v8::Local<v8::Function>, int, v8::Local<v8::Value>*, node::async_context) [/Users/yagiz/coding/node/out/Release/node]
12: 0x104513784 node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) [/Users/yagiz/coding/node/out/Release/node]
13: 0x1045e5158 node::fs::FSReqCallback::Resolve(v8::Local<v8::Value>) [/Users/yagiz/coding/node/out/Release/node]
14: 0x1045e5e44 node::fs::AfterInteger(uv_fs_s*) [/Users/yagiz/coding/node/out/Release/node]
15: 0x1045d9acc node::MakeLibuvRequestCallback<uv_fs_s, void (*)(uv_fs_s*)>::Wrapper(uv_fs_s*) [/Users/yagiz/coding/node/out/Release/node]
16: 0x105080a70 uv__work_done [/Users/yagiz/coding/node/out/Release/node]
17: 0x1050844b8 uv__async_io [/Users/yagiz/coding/node/out/Release/node]
18: 0x10509751c uv__io_poll [/Users/yagiz/coding/node/out/Release/node]
19: 0x105084a50 uv_run [/Users/yagiz/coding/node/out/Release/node]
20: 0x1044ffbd0 node::SpinEventLoopInternal(node::Environment*) [/Users/yagiz/coding/node/out/Release/node]
21: 0x1046225c4 node::NodeMainInstance::Run(node::ExitCode*, node::Environment*) [/Users/yagiz/coding/node/out/Release/node]
22: 0x104622370 node::NodeMainInstance::Run() [/Users/yagiz/coding/node/out/Release/node]
23: 0x1045a43d8 node::Start(int, char**) [/Users/yagiz/coding/node/out/Release/node]
24: 0x18121d0e0 start [/usr/lib/dyld]
Command: out/Release/node /Users/yagiz/coding/node/test/parallel/test-file-write-stream4.js
--- CRASHED (Signal: 6) ---

[billywhizz]

I'm having crashes due to GetCreationContextChecked(). Any suggestions? @joyeecheung @legendecas

you can't use a fastcall if whatever you are calling into will end up calling into JS or allocating on the v8 heap?

[anonrig]

I'm having crashes due to GetCreationContextChecked(). Any suggestions? @joyeecheung @legendecas

you can't use a fastcall if whatever you are calling into will end up calling into JS or allocating on the v8 heap?

That statement is true, but I don't think this applies to here. For example, the following PR run the tests properly: https://github.com/nodejs/node/pull/49893/files#diff-70e3325bd2115867617ae2a16321c5de53c070ff2841976fe5b849675a5111e6R1106, and it uses recv->GetCreationContextChecked()

[billywhizz]

I'm having crashes due to GetCreationContextChecked(). Any suggestions? @joyeecheung @legendecas

you can't use a fastcall if whatever you are calling into will end up calling into JS or allocating on the v8 heap?

That statement is true, but I don't think this applies to here. For example, the following PR run the tests properly: https://github.com/nodejs/node/pull/49893/files#diff-70e3325bd2115867617ae2a16321c5de53c070ff2841976fe5b849675a5111e6R1106, and it uses recv->GetCreationContextChecked()

ah ok. are you sure it's crashing on that call and not when the microtask is being called though? i always assumed i couldn't wrap that call in a fastcall myself, but i'm probably wrong.

Edit: i've just seen your stacktrace. yeah, i'm out of ideas. sorry.

[santigimeno]

That particular error should be fixed by this:

diff --git a/lib/internal/process/task_queues.js b/lib/internal/process/task_queues.js
index bcb5eef841d..dc65e5bb36e 100644
--- a/lib/internal/process/task_queues.js
+++ b/lib/internal/process/task_queues.js
@@ -5,12 +5,13 @@ const {
   FunctionPrototypeBind,
 } = primordials;
 
+const taskQueue = internalBinding('task_queue');
+
 const {
   // For easy access to the nextTick state in the C++ land,
   // and to avoid unnecessary calls into JS land.
   tickInfo,
   // Used to run V8's micro task queue.
-  runMicrotasks,
   setTickCallback,
   enqueueMicrotask,
 } = internalBinding('task_queue');
@@ -57,7 +58,7 @@ const queue = new FixedQueue();
 // Should be in sync with RunNextTicksNative in node_task_queue.cc
 function runNextTicks() {
   if (!hasTickScheduled() && !hasRejectionToWarn())
-    runMicrotasks();
+    taskQueue.runMicrotasks();
   if (!hasTickScheduled() && !hasRejectionToWarn())
     return;
 
@@ -92,7 +93,7 @@ function processTicksAndRejections() {
 
       emitAfter(asyncId);
     }
-    runMicrotasks();
+    taskQueue.runMicrotasks();
   } while (!queue.isEmpty() || processPromiseRejections());
   setHasTickScheduled(false);
   setHasRejectionToWarn(false);

but there are bigger problems than that. It seems that the function may lead to JS code execution down the line which is specifically not allowed for fast call API's. As an example after applying the patch:

$ out/Release/node test/parallel/test-gc-tls-external-memory.js 
NOTE: The test started as a child_process using these flags: [ '--expose-gc' ] Use NODE_SKIP_FLAG_CHECK to run the test with the original flags.

#
# Fatal error in , line 0
# Invoke in DisallowJavascriptExecutionScope
#
#
#
#FailureMessage Object: 0x7fffffc51020
----- Native stack trace -----

 1: 0x557659a91925  [/home/sgimeno/software/node/out/Release/node]
 2: 0x55765b05df16 V8_Fatal(char const*, ...) [/home/sgimeno/software/node/out/Release/node]
 3: 0x557659e02d0e  [/home/sgimeno/software/node/out/Release/node]
 4: 0x557659e03562 v8::internal::Execution::TryRunMicrotasks(v8::internal::Isolate*, v8::internal::MicrotaskQueue*) [/home/sgimeno/software/node/out/Release/node]
 5: 0x557659e38587 v8::internal::MicrotaskQueue::RunMicrotasks(v8::internal::Isolate*) [/home/sgimeno/software/node/out/Release/node]
 6: 0x557659e3898d v8::internal::MicrotaskQueue::PerformCheckpoint(v8::Isolate*) [/home/sgimeno/software/node/out/Release/node]
 7: 0x5575da8e6d73 
Trace/breakpoint trap

[legendecas]

As mentioned above, v8::MicrotaskQueue::PerformCheckpoint can run JavaScript code like promise handlers, which inevitably trigger garbage collection. This doesn't meet the requirement of a fast API: https://github.com/nodejs/node/blob/main/doc/contributing/adding-v8-fast-api.md#limitations.

[joyeecheung]

Yes, the whole point of runMicroTasks() is basically running promise handlers so it cannot be converted to a fast API call.