src: invoke per-isolate setters earlier #25608
Conversation
This patch moves the invocation of: - `SetHostInitializeImportMetaObjectCallback` - `SetHostImportModuleDynamicallyCallback` - `SetPromiseRejectCallback` and the DTrace-related: - `AddGCPrologueCallback(DTraceGCStartCallback)` - `AddGCEpilogueCallback(DTraceGCEndCallback)`. into `node::NewIsolate()`, and moves the performance-timeline-related: - `AddGCPrologueCallback(performance::MarkGarbageCollectionStart)` - `AddGCEpilogueCallback(performance::MarkGarbageCollectionEnd)` that needs per-Environment data into the `Environment` constructor, instead of scattering the initialization code around in bindings.
nodejs-github-bot
added
c++
|
|
||
| // NOTE: the following two callbacks currently CHECKs that the isolates | ||
| // have associated environments. | ||
| isolate->SetHostInitializeImportMetaObjectCallback( |
There was a problem hiding this comment.
I am not entirely sure if we should do this here, or in Environment constructor, since these two callbacks do not handle the cases when the isolates' current context does not have Environments associated. Does it make sense to simply crash if the user tries to access import() or import.meta from a context without an associated environment? Or should we return an empty Maybe/do nothing in the callbacks instead? Or should we postpone this to the Environment initialization? (But then the Environments would control per-isolate states)
There was a problem hiding this comment.
It’s tricky… I think for now crashing is okay (and the best we can do), and anything more would require interaction with embedders (which would be the main source for this not working).
There was a problem hiding this comment.
i don't think these esm ones should have been moved. they are set in response to core opting into handling them. if the handlers don't get attached, we shouldn't have these handlers enabled.
There was a problem hiding this comment.
I think I agree with @devsnek. Embedders can install their own after the call to NewIsolate().
joyeecheung
requested review from
addaleax and
bnoordhuis
and removed request for
addaleax
|
|
||
| // NOTE: the following two callbacks currently CHECKs that the isolates | ||
| // have associated environments. | ||
| isolate->SetHostInitializeImportMetaObjectCallback( |
There was a problem hiding this comment.
It’s tricky… I think for now crashing is okay (and the best we can do), and anything more would require interaction with embedders (which would be the main source for this not working).
| @@ -440,8 +432,6 @@ void Initialize(Local<Object> target, | |||
| env->constants_string(), | |||
| constants, | |||
| attr).ToChecked(); | |||
|
|
|||
| SetupGarbageCollectionTracking(env); | |||
There was a problem hiding this comment.
I would actually prefer for this to stay here. It makes the code more local and more obvious to me, and only adds the callbacks when they are actually used.
There was a problem hiding this comment.
@addaleax this is currently unconditionally called when the isolate goes through bootstrapping - maybe this should be delayed until the first time the user adds an gc performance observer..
There was a problem hiding this comment.
@joyeecheung If that’s feasible, it sounds good to me, but I’m more worried about code organization than performance.
Also, I’m just noticing that we never remove these listeners … that’s a bug that we should fix if we can?
There was a problem hiding this comment.
See #25647 – I’m happy to close it if you want to do sth in this PR :)
|
|
||
| // NOTE: the following two callbacks currently CHECKs that the isolates | ||
| // have associated environments. | ||
| isolate->SetHostInitializeImportMetaObjectCallback( |
There was a problem hiding this comment.
I think I agree with @devsnek. Embedders can install their own after the call to NewIsolate().
| isolate->SetHostImportModuleDynamicallyCallback( | ||
| loader::ModuleWrap::ImportModuleDynamically); | ||
|
|
||
| #if defined HAVE_DTRACE || defined HAVE_ETW |
There was a problem hiding this comment.
Since you're here: #if defined(HAVE_DTRACE) || defined(HAVE_ETW) - the paren-less style is unidiomatic.
|
Blocked by #25853 |
|
On a second thought, I think the current invocation order looks more reasonable, so I am closing this PR. |
This patch moves the invocation of:
SetHostInitializeImportMetaObjectCallbackSetHostImportModuleDynamicallyCallbackSetPromiseRejectCallbackand the DTrace-related:
AddGCPrologueCallback(DTraceGCStartCallback)AddGCEpilogueCallback(DTraceGCEndCallback).into
node::NewIsolate(), and moves the performance-timeline-related:AddGCPrologueCallback(performance::MarkGarbageCollectionStart)AddGCEpilogueCallback(performance::MarkGarbageCollectionEnd)that needs per-Environment data into the
Environmentconstructor,instead of scattering the initialization code around in bindings.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes