src: move Environment::context out of strong properties #27430
Conversation
nodejs-github-bot
added
the
c++
src/env.h
Outdated
There was a problem hiding this comment.
You could merge this with ENVIRONMENT_STRONG_PERSISTENT_VALUES - v8::Value is a subclass of v8::Data too and everything in ENVIRONMENT_STRONG_PERSISTENT_VALUES is a subclass of v8::Value.
That said, since everything in ENVIRONMENT_STRONG_PERSISTENT_DATA is a subclass of v8::Template now, you should name it something like PER_ISOLATE_TEMPLATE_PROPERTIES and make them per-isolate.
There was a problem hiding this comment.
Good point about making these v8::Template, though conceptually I think these are per-Environment, as to add these into the snapshot I'll also need to register the external references for them, which is WIP, and the reason that these can be excluded from the current minimal snapshot is that all of these should be empty prior to Environment creation.
Rename `ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES` to `ENVIRONMENT_STRONG_PERSISTENT_DATA`, and move `context` out of the list, so that the data can be iterated separately.
s/DATA/TEMPLATE/
This comment has been minimized.
This comment has been minimized.
|
Rebased and renamed |
This comment has been minimized.
This comment has been minimized.
s/TEMPLATE/TEMPLATES/
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
LGTM but can you add a TODO about changing them to per-isolate properties? Cheers.
@bnoordhuis What's the difference between per-isolate and per-Environment? So far in my brain, per-isolate stuff are more primitive and can be initialized without an Environment (i.e. before bootstrap, so it makes sense for them to be data members of IsolateData), whereas per-Environment stuff are more sophisticated and can only be initialized after there is an associated Environment (so it makes sense for them to be data members of Environment). By that standard the templates should be per-Environment because they are all empty before an associated Environment is available. |
|
I don’t have a strong opinion on per-Isolate vs per-Environment, but I don’t think it’s a bad idea to move Context-independent values to per-Isolate storage in general. I think practical implications would only be there if we have multiple Environments per Isolate, which should be rare enough – 1. reducing the number of templates we create and 2. enabling the use of native objects from Environment A in Environment B… that doesn’t seem like an inherently bad thing. |
|
@addaleax @bnoordhuis I took a look at the templates and I think so far most of them are per-Environment - for example most of the function templates have the Environment itself attached as callback data (for It may make sense to make these per-Isolate if we create these templates during IsolateData creation, and detach the |
|
Landed in Landed in 6f02f15. We'll see if these templates can be made per-Isolate afterwards.. |
Rename `ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES` to `ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES`, and move `context` out of the list, so that the data can be iterated separately. PR-URL: #27430 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
Rename `ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES` to `ENVIRONMENT_STRONG_PERSISTENT_TEMPLATES`, and move `context` out of the list, so that the data can be iterated separately. PR-URL: #27430 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
5 participants
Rename
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIEStoENVIRONMENT_STRONG_PERSISTENT_DATA, and movecontextout of the list, so that the data can be iterated separately.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes