doc: update V8 fast API guidance #58999
Conversation
|
Review requested:
|
| <!-- | ||
| Deliberately omitted: | ||
| * `void *` (let's not go down this road...) | ||
| * `v8::Local<v8::Object>` (V8 advises that this should only be used for | ||
| receiver arguments, but it's more consistent to use | ||
| v8::Value handles throughout) | ||
| --> |
There was a problem hiding this comment.
void *is interpreted by V8 as a pointer to an embedder type. This doesn't feel like something to entertain, but there may be differing opinions.- V8 suggests a
v8::Objecthandle to be used as the type for the receiver argument, and av8::Valuehandle for other arguments. This isn't enforced, and sticking withv8::Valuethroughout is probably the better option, both for consistency and because the receiver for most fast callbacks is going to beundefined.
| or `undefined`), you will need to ensure that the first argument is discarded: | ||
|
|
||
| ```cpp | ||
| bool FastIsObject(v8::Local<v8::Value>, // receiver, discarded |
There was a problem hiding this comment.
Should the convention be for a named or unnamed discard parameter here? Both are currently used within the codebase.
| target, | ||
| "func", | ||
| SlowFunc, | ||
| fast_func_callbacks); |
There was a problem hiding this comment.
Passing the array directly (corresponding to the array signature of the v8::MemorySpan constructor) seems like the more straightforward recommendation. The only example in the codebase at present uses the iterator signature:
Lines 525 to 526 in 0992d28
| If a fast callback creates any `v8::Local` handles within the fast callback, | ||
| then it must first initialize a new `v8::HandleScope` to ensure that the | ||
| handles are correctly scoped and garbage-collected. |
There was a problem hiding this comment.
This doesn't have any equivalent in the previous guidance, since stack access is a new feature. This seems like the correct advice – I'm fairly sure that CI has flagged exceptions from V8 relating to this specifically – but others will be more informed than me!
Renegade334
force-pushed
the
doc-fast-api
branch
2 times, most recently
from
78795ed to
43884b2
Compare
| * `uint64_t` | ||
| * `float` | ||
| * `double` | ||
| * `v8::Local<v8::Value>` (analogous to `any`) |
There was a problem hiding this comment.
| As above, initializing a `v8::HandleScope` is mandatory before any operations | ||
| which create local handles. |
There was a problem hiding this comment.
As with #58999 (comment), I'm fairly sure that invoking the ERROR macros (which build the error within a local object handle) necessitates a HandleScope here, but someone else may have a definitive answer?
Draft, to garner feedback.
The existing guidance dates from the original V8 implementation, as included in Node.js before v23. The "new" Fast API is quite different, particularly regarding error handling and stack access. This document therefore needs a significant refresh.
There are one or two places where changes to the API necessitates a convention to be defined for the Node.js codebase; I've flagged these for discussion.
cc: @anonrig, @targos (as authors of this document)
cc: @nodejs/v8