Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: retrieve binding data from the context #33139

Closed
wants to merge 5 commits into from

Conversation

addaleax
Copy link
Member

This is mostly taken from the common subset of #32761 and #32984, with a few modifications on top of it (notably, BindingData does not make sense as a class any longer and can be replaced by BaseObject directly).


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), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 29, 2020
@addaleax addaleax force-pushed the context-indepedent-bindings branch from c4e9967 to b6aaf21 Compare April 29, 2020 06:07
@nodejs-github-bot
Copy link
Collaborator

src/env-inl.h Outdated
return v8::MaybeLocal<v8::Object>();
}
T* data = new T(this, obj);
inline std::pair<T*, uint32_t> Environment::NewBindingData(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@bnoordhuis
Copy link
Member

This makes the function templates more context-independent, and makes it possible to embed binding data in non-main contexts.

Can you elaborate? I either don't understand:

  1. what currently doesn't work with non-main contexts, or
  2. why that is useful if "non-main" means "non-node"

src/env-inl.h Outdated
context->GetAlignedPointerFromEmbedderData(
ContextEmbedderIndex::kBindingListIndex));
DCHECK_NOT_NULL(list);
size_t index = list->size();
Copy link
Member

@joyeecheung joyeecheung Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung No strong feelings here, and happy to make the switch if you think that should happen in this PR.

Copy link
Member

@joyeecheung joyeecheung Apr 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung Done, including the removal of NoBindingData and BindingScope 🙂 PTAL

@joyeecheung
Copy link
Member

@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.

why that is useful if "non-main" means "non-node"

The idea is to make it possible to create "non-main" contexts with their Node.js builtins - that is, "non-main but node" contexts

joyeecheung and others added 3 commits May 5, 2020 17:49
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 addaleax force-pushed the context-indepedent-bindings branch from b6aaf21 to 2ff3173 Compare May 5, 2020 23:13
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the review wanted PRs that need reviews. label May 5, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nits, thanks

src/env-inl.h Outdated Show resolved Hide resolved
src/env-inl.h Outdated Show resolved Hide resolved
src/README.md Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels May 6, 2020
@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request May 6, 2020
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
Copy link
Member Author

addaleax commented May 6, 2020

Landed in 86fdaa7

@addaleax addaleax closed this May 6, 2020
@addaleax addaleax deleted the context-indepedent-bindings branch May 6, 2020 04:44
codebytere pushed a commit that referenced this pull request May 7, 2020
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>
@codebytere codebytere mentioned this pull request May 18, 2020
@codebytere
Copy link
Member

codebytere commented Jun 7, 2020

@addaleax there are a lot of conflict for this on 12.x - could you please open a manual backport?

@joyeecheung
Copy link
Member

⚠ The following ancestor commits of 86fdaa7 are not on v12.x-staging

FYI these are the missing ancestor commits git node backport found

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants