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: split out per-binding state from Environment #32538

Closed
wants to merge 15 commits into from

Conversation

addaleax
Copy link
Member

src: make creating per-binding data structures easier

Enable the state associated with the individual bindings, e.g. fs or
http2, to be moved out of the Environment class, in order for these
to be more modular and for Environment to be come less of a collection
of random data fields.

Do this by using a BaseObject as the data for callbacks, which can hold
the per-binding state. By default, no per-binding state is available,
although that can be configured when setting up the binding.

src: move HTTP/2 state out of Environment

Moves state that is specific to HTTP/2 into the HTTP/2 implementation
as a cleanup.

src: move v8 stats buffers out of Environment

Moves state that is specific to the v8 binding into the
v8 binding implementation as a cleanup.

src: move http parser state out of Environment

Moves state that is specific to HTTP/1 into the HTTP/1 implementation
as a cleanup.

src: move fs state out of Environment

Moves state that is specific to the fs binding into the
fs binding implementation as a cleanup.

src,doc: add documentation for per-binding state pattern

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax requested a review from jasnell March 28, 2020 23:19
@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 Mar 28, 2020
@nodejs-github-bot
Copy link
Collaborator

src/node_file.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 29, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30174/ (:heavy_check_mark:)

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 30, 2020

@nodejs-github-bot
Copy link
Collaborator

Enable the state associated with the individual bindings, e.g. fs or
http2, to be moved out of the Environment class, in order for these
to be more modular and for Environment to be come less of a collection
of random data fields.

Do this by using a BaseObject as the data for callbacks, which can hold
the per-binding state. By default, no per-binding state is available,
although that can be configured when setting up the binding.
Moves state that is specific to HTTP/2 into the HTTP/2 implementation
as a cleanup.
Moves state that is specific to the `v8` binding into the
`v8` binding implementation as a cleanup.
Moves state that is specific to HTTP/1 into the HTTP/1 implementation
as a cleanup.
Moves state that is specific to the `fs` binding into the
`fs` binding implementation as a cleanup.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 5, 2020
@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit that referenced this pull request Apr 6, 2020
Enable the state associated with the individual bindings, e.g. fs or
http2, to be moved out of the Environment class, in order for these
to be more modular and for Environment to be come less of a collection
of random data fields.

Do this by using a BaseObject as the data for callbacks, which can hold
the per-binding state. By default, no per-binding state is available,
although that can be configured when setting up the binding.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Apr 6, 2020
Moves state that is specific to HTTP/2 into the HTTP/2 implementation
as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Apr 6, 2020
Moves state that is specific to the `v8` binding into the
`v8` binding implementation as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Apr 6, 2020
Moves state that is specific to HTTP/1 into the HTTP/1 implementation
as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Apr 6, 2020
Moves state that is specific to the `fs` binding into the
`fs` binding implementation as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Apr 6, 2020
PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Apr 6, 2020

Landed in c2aedd0...6d6de56

@addaleax addaleax closed this Apr 6, 2020
@addaleax addaleax deleted the env-break-up2 branch April 6, 2020 00:07
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Enable the state associated with the individual bindings, e.g. fs or
http2, to be moved out of the Environment class, in order for these
to be more modular and for Environment to be come less of a collection
of random data fields.

Do this by using a BaseObject as the data for callbacks, which can hold
the per-binding state. By default, no per-binding state is available,
although that can be configured when setting up the binding.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Moves state that is specific to HTTP/2 into the HTTP/2 implementation
as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Moves state that is specific to the `v8` binding into the
`v8` binding implementation as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Moves state that is specific to HTTP/1 into the HTTP/1 implementation
as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Moves state that is specific to the `fs` binding into the
`fs` binding implementation as a cleanup.

PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
PR-URL: #32538
Reviewed-By: James M Snell <jasnell@gmail.com>
v8::Local<v8::Function> ctor;
v8::Local<v8::Object> obj;
if (!as_callback_data_template()->GetFunction(context()).ToLocal(&ctor) ||
!ctor->NewInstance(context()).ToLocal(&obj)) {
Copy link
Member

@joyeecheung joyeecheung Apr 10, 2020

Choose a reason for hiding this comment

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

Sorry for noticing this late - but I came across this change after a recent rebase, and I think this constraint about BaseObject is incorrect (see #26382 (comment)). Context-independent templates should not point to context-dependent objects, but now every templates created by Environment::FunctionTemplate, which are supposed to be context-independent, point to different callback data that are context-dependent v8::Objects that point to BaseObjects, making the templates non-snapshottable (and theoratically just incorrect, but this is only validated by V8's serializer and not at runtime). This was already violated by #26382 and this PR just splits one context-dependent callback data into multiple context-dependent ones, and force them to be that way since BaseObjects are...associated with v8::Object (which have to be context-dependent due to references to the constructors on the prototype chain).

Copy link
Member

@joyeecheung joyeecheung Apr 10, 2020

Choose a reason for hiding this comment

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

Note that even before #26382, when we were using v8::External, it was still somewhat limited - by obtaining the data directly through whatever passed into FunctiontTemplate::New, we'll always end up with the same set of callback data in the function template callback even if we switch the invoking context. This PR only makes the binding data variable by context, but it ties the FunctionTemplates to the set of the data created by the main context. If the intention is to make the binding data variable by the invoking context, (for example, so that different vm contexts have different set of builtins), we need to associate the binding data with the contexts and obtain them from there (e.g. through embedder slots, or private properties on the global object, something like GetBindingData<T>(args.GetIsolate()->GetCurrentContext())), instead of associating them with and and obtaining them from the function template callback data (Unwrap<T>(args.Data()) currently being done here), since there's always only one set of data per function template.

Copy link
Member Author

@addaleax addaleax Apr 10, 2020

Choose a reason for hiding this comment

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

@joyeecheung Yeah, I know about these issues, but this PR doesn’t really seem to make anything worse?

There is a commit in #32761 that would switch this back to using Externals for snapshotting, if we don’t end up with a better solution.

If the intention is to make the binding data variable by the invoking context, (for example, so that different vm contexts have different set of builtins), we need to associate the binding data with the contexts and obtain them from there (e.g. through embedder slots, or private properties on the global object, something like GetBindingData<T>(args.GetIsolate()->GetCurrentContext())), instead of associating them with and and obtaining them from the function template callback data (Unwrap<T>(args.Data()) currently being done here), since there's always only one set of data per function template.

Err, yeah, we could do that, but if you look it that way we’re just using FunctionTemplates and ObjectTemplates in a wrong way to begin with. It would be nicer if we could just create context-dependent templates, I would say.

Copy link
Member Author

Choose a reason for hiding this comment

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

… or, even better, if we could provide data to the per-context instantiations of the templates, separately.

Copy link
Member

@joyeecheung joyeecheung Apr 10, 2020

Choose a reason for hiding this comment

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

Yeah, I know about these issues, but this PR doesn’t really seem to make anything worse?

I think this makes it worse in that it now forces the FunctionTemplates to be associated with context-dependent BaseObjects with the BindingData machinery. In addition BaseObject implicitly implies that the object is tied to the main context. If we were to add an indirection, we could add an indirection based on v8::External directly, but an indirection based on BaseObject(which in turn relies on Object) still doesn't make too much sense to me.

It would be nicer if we could just create context-dependent templates, I would say.

From previous discussions I think that was discouraged and it is preferred to obtain the properties from contexts. FunctionTemplates and ObjectTemplates essentially belong to the isolate while the Function and Object instantiated from them belong to the context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I know about these issues, but this PR doesn’t really seem to make anything worse?

I think this makes it worse in that it now forces the FunctionTemplates to be associated with context-dependent BaseObjects with the BindingData machinery.

… but previously it was also associated with a context-dependent object?

In addition BaseObject implicitly implies that the object is tied to the main context

In what way? We can and do create BaseObjects in core that are associated with Contexts coming from vm.createContext().

If we were to add an indirection, we could add an indirection based on v8::External directly, but an indirection based on BaseObject(which in turn relies on Object) still doesn't make too much sense to me.

I’m not sure what you have in mind, but as said above, #32761 does contain a commit that would switch this back to v8::External for snapshotting.

It would be nicer if we could just create context-dependent templates, I would say.

From previous discussions I think that was discouraged and it is preferred to obtain the properties from contexts.

Yeah, hence the addition that it would be even better for the data associated with the template to be set or filled per-context.

FunctionTemplates and ObjectTemplates essentially belong to the isolate while the Function and Object instantiated from them belong to the context.

Yeah, I know. I don’t think we’re disagreeing here.

@targos targos added backport-requested-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants