src: retrieve binding data from the context #33139
Conversation
nodejs-github-bot
added
c++
addaleax
force-pushed
the
context-indepedent-bindings
branch
from
c4e9967
to
b6aaf21
Compare
src/env-inl.h
Outdated
| return v8::MaybeLocal<v8::Object>(); | ||
| } | ||
| T* data = new T(this, obj); | ||
| inline std::pair<T*, uint32_t> Environment::NewBindingData( |
There was a problem hiding this comment.
Why was the BindingDataBase abstraction removed? I think it's beneficial to have a common base class for them (for one, they share the characteristic that they are one-per-context, so for example we can add some checks for that, or do bookkeeping otherwise)
There was a problem hiding this comment.
@joyeecheung Because the class doesn’t currently add any extra value – it just forwards directly to BaseObject without functionality on its own. The only reason that my PR originally introduced it was to store the v8::External associated with it, but this PR is rendering that unnecessary.
If we do run into a situation in which we want to add something here it, adding an intermediate class should be straightforward.
Can you elaborate? I either don't understand:
|
src/env-inl.h
Outdated
| context->GetAlignedPointerFromEmbedderData( | ||
| ContextEmbedderIndex::kBindingListIndex)); | ||
| DCHECK_NOT_NULL(list); | ||
| size_t index = list->size(); |
There was a problem hiding this comment.
Maybe we should use a map to store these bindings, and add static methods to all the BindingDataBase subclass to return the same name as their internalBinding() identifier. Then we can just get the bindings via a look up using T::binding_name() as key and there will be no need for BindingScope (the binding initializers can make sure a pair of T::binding_name(), new T(env, target) is inserted into the map) or adding any data parameter to the Function Templates. Also then there's no need for the default callback data (because we'll just get the Environment from the Environment slot).
If we just use unordered map the lookup overhead is constant like std::vector anyways (but it probably doesn't matter that much given the size of this map)
There was a problem hiding this comment.
@joyeecheung No strong feelings here, and happy to make the switch if you think that should happen in this PR.
There was a problem hiding this comment.
I was thinking about doing changes like this to my patch before I open a PR myself, so I guess yes I'd like to see it happen here as I saw the old patch as incomplete.
There was a problem hiding this comment.
@joyeecheung Done, including the removal of NoBindingData and BindingScope 🙂 PTAL
|
@bnoordhuis Currently the function templates are created with a v8::Object as its attached data, and v8::Objects are context-dependent (because they reference the Object constructor of one particular context through the prototype chain), and need to go into the context snapshot, whereas function templates are context-independent and goes into the isolate snapshot - then attaching context-dependent objects to context-independent templates makes the templates non-snapshottable.
The idea is to make it possible to create "non-main" contexts with their Node.js builtins - that is, "non-main but node" contexts |
Instead of passing them through the data bound to function templates, store references to them in a list embedded inside the context, and store the integer index (which is context-independent) in the function template data. This makes the function templates more context-independent, and makes it possible to embed binding data in non-main contexts.
Co-authored-by: Anna Henningsen <anna@addaleax.net>
addaleax
force-pushed
the
context-indepedent-bindings
branch
from
b6aaf21
to
2ff3173
Compare
addaleax
added
author ready
Instead of passing them through the data bound to function templates, store references to them in a list embedded inside the context. This makes the function templates more context-independent, and makes it possible to embed binding data in non-main contexts. Co-authored-by: Anna Henningsen <anna@addaleax.net> PR-URL: #33139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
Landed in 86fdaa7 |
Instead of passing them through the data bound to function templates, store references to them in a list embedded inside the context. This makes the function templates more context-independent, and makes it possible to embed binding data in non-main contexts. Co-authored-by: Anna Henningsen <anna@addaleax.net> PR-URL: #33139 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
|
@addaleax there are a lot of conflict for this on 12.x - could you please open a manual backport? |
|
⚠ The following ancestor commits of 86fdaa7 are not on v12.x-staging
FYI these are the missing ancestor commits |
This is mostly taken from the common subset of #32761 and #32984, with a few modifications on top of it (notably,
BindingDatadoes not make sense as a class any longer and can be replaced byBaseObjectdirectly).Instead of passing them through the data bound to function
templates, store references to them in a list embedded inside
the context, and store the integer index
(which is context-independent) in the function template data.
This makes the function templates more context-independent,
and makes it possible to embed binding data in non-main contexts.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes