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

WIP V8 API usage in Node.js #26929

Closed
wants to merge 2 commits into from
Closed

Conversation

sam-github
Copy link
Contributor

I've had these notes around for a while, as a result of me spending some time trying to figure out how the V8 API works. Even in this rough draft form, I hope it might lower the barrier to entry for anybody wanting to work on node's C++.

I'll keep working on this as I have time, though I expect it to be slow. I'd be happy to have comments on inaccuracies or XXX in the text, and the "allow edits from maintainers" is ticked. V8 experts, please don't be shy to push fixups if you are so inclined!

V8's API is not very documented, so many (most?) of us just find existing code that does what we want to do and copy it, but #26868 shows this can be problematic, because the expectations for how C++ should be written are evolving, and choosing the wrong existing code as a template can spread code practices that are now discouraged. Hopefully, this guide can point people to the current expectations.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 26, 2019
@sam-github sam-github added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. wip Issues and PRs that are still a work in progress. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. addons Issues and PRs related to native addons. and removed doc Issues and PRs related to the documentations. addons Issues and PRs related to native addons. labels Mar 26, 2019
@devsnek
Copy link
Member

devsnek commented Mar 26, 2019

Full build of v8 docs from the source comments and such is available here: https://v8.paulfryzel.com/docs/master/ (yes, you have to accept the invalid cert, paul always forgets to update his certs)

i also find https://cs.chromium.org/chromium/src/v8/include/v8.h very helpful, as you can start with a public api and work your way through it thanks to how cs.chromium.org links structures.

doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
JavaScript code to run in a single instance of V8. The motivation for using
contexts in V8 was so that each window and iframe in a browser can have its
own fresh JavaScript environment.
XXX Node uses one context, mostly, does vm. create new ones? anything else?
Copy link
Member

Choose a reason for hiding this comment

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

I think that’s about it.

One kind-of-open question is whether we want Node’s own APIs to eventually supports multiple contexts, but that would be a major change to some of our internals…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does vm (or anything else) create new Contexts?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the vm module does that. It’s currently the only public API for that.

own fresh JavaScript environment.
XXX Node uses one context, mostly, does vm. create new ones? anything else?

Can get from `isolate->GetCurrentContext()`
Copy link
Member

Choose a reason for hiding this comment

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

… which is why this should be the same as env->context() in almost all cases.

Can get from `isolate->GetCurrentContext()`


XXX Function vs FunctionTemplate ... wth?
Copy link
Member

Choose a reason for hiding this comment

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

The distinction isn’t that important to us because we don’t expose many APIs for more than one context (MessagePort being the only exception I can think of right now) – essentially, a FunctionTemplate can be used to create functionally identical function instances for multiple contexts.

Copy link
Member

Choose a reason for hiding this comment

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

FunctionTemplates are also useful because they give you an ObjectTemplate (through InstanceTemplate()) that you can use to define internal fields. You can't do that with regular Functions.

- `Local<Object> exports`: where to put exported properties, conventionally
called `target` in node
- `Local<Value> module`: conventionally unused in node
XXX what is this for? addon docs don't mention or use it
Copy link
Member

Choose a reason for hiding this comment

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

It should be the same value that module has in CJS scripts – I think this is mostly there so that it matches the addon API more closely? We can remove it if we want, I’d say.

XXX what is this for? addon docs don't mention or use it
- `Local<Context> context`:
- void* priv: not commonly used
XXX where is it ever used? for what?
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think anybody uses this in practice, neither Node.js itself nor addons. I think the idea was for it to function as sort of a opaque pointer, but that never became useful due to the way that we load addons?

@addaleax
Copy link
Member

Also, thanks for starting this!

doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
Can get from `isolate->GetCurrentContext()`


XXX Function vs FunctionTemplate ... wth?
Copy link
Member

Choose a reason for hiding this comment

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

FunctionTemplates are also useful because they give you an ObjectTemplate (through InstanceTemplate()) that you can use to define internal fields. You can't do that with regular Functions.


Commonly used convenience methods:
- ThrowError/TypeError/RangeError/ErrnoException/...
XXX why are some called Error and others called Exception?
Copy link
Member

Choose a reason for hiding this comment

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

Legacy :-) ThrowErrnoException() and ThrowUVException() are old, the others are newer. I'd be okay with renaming them if that clears up the confusion / cognitive dissonance.

doc/guides/V8-api-for-node.md Outdated Show resolved Hide resolved
(manually managed scope). Constructors (String::New) seem to return Locals.

`return Local<Array>();` ... seems to do exactly what you are not supposed
to do... whats up?
Copy link
Member

Choose a reason for hiding this comment

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

It's okay to return empty handles, just not handles that point to something without going through EscapableHandleScope::Escape().

@addaleax
Copy link
Member

addaleax commented Apr 8, 2019

@sam-github Btw, I’m still grateful for you doing this, but feel free to let us know if you would like someone to take over this PR :)

@sam-github
Copy link
Contributor Author

It's very close to the top of my list! And I'm in the process fixing that abort caused by a throwing setter on Object.prototype which you gave me a demo for, which I hope will help me with this.

@sam-github
Copy link
Contributor Author

I integrated a bunch of the extra information, and that's as far as I got for now.

Btw, I don't mind if anybody pushes additional commits directly to this branch.

@sam-github
Copy link
Contributor Author

@devsnek thanks for the links, are you sure they are up to date? https://v8.paulfryzel.com/docs/master/classv8_1_1_maybe.html doesn't have Check() and our v8.h does.

@@ -3,29 +3,50 @@
v8 docs, not particularly useful:
- https://v8.dev/docs

v8 docs, generated from master:
- https://v8.paulfryzel.com/docs/master/
Copy link
Member

Choose a reason for hiding this comment

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

I think these are out of date? Also Firefox warns that it's using an expired certificate.

Copy link
Member

Choose a reason for hiding this comment

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

It's documenting V8 6.9 so yeah, pretty stale.

Copy link
Member

Choose a reason for hiding this comment

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

Could we fork this https://github.com/paulfryzel/v8-doxygen, and redeploy it to GH pages or Nodejs.org

Copy link
Member

@gengjiawen gengjiawen May 7, 2019

Choose a reason for hiding this comment

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

Copy link
Member

@gengjiawen gengjiawen May 7, 2019

Choose a reason for hiding this comment

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

Online preview by gitlab: https://gengjiawen.gitlab.io/v8-docs/.

@@ -3,29 +3,50 @@
v8 docs, not particularly useful:
- https://v8.dev/docs

v8 docs, generated from master:
- https://v8.paulfryzel.com/docs/master/
Copy link
Member

Choose a reason for hiding this comment

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

It's documenting V8 6.9 so yeah, pretty stale.

the only references to an object are from weak persistent handles.
- A `v8::Global<>` (alias of a `UniquePersistent<>`) handle relies on C++
constructors and destructors to manage the lifetime of the underlying
object. We don’t use `UniquePersistent` in Node.js.
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop the notes about UniquePersistent, that's its old name and will probably go away entirely someday.

- Boolean becomes 1/0 as int, "true"/"false" as strings, etc.
- Numbers become false as Boolean (for any value), -3 casts to String "-3"
- Functions become numerically zero, and "function () { const hello=0; }" as a
String
Copy link
Member

Choose a reason for hiding this comment

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

Sam, can you remove these three bullet points?

- To<T> will convert values in fairly typical js way:
... never seems to be used by node?
AFAICT, is identical to the As<> route, except for Boolean, which is always
false with As<T>(), but is "expected" with ToT().
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this paragraph? There's no templated To<T>.

(There's Maybe<T>::To() but that's different.)

@gengjiawen gengjiawen mentioned this pull request May 3, 2019
4 tasks
@gengjiawen
Copy link
Member

Add a link to CONTRIBUTING.md ?


- `Local<...>`: A local handle is a pointer to an object. All V8 objects are
accessed using handles, they are necessary because of the way the V8 garbage
collector works. Local handles can only be allocated on the stack, in a
Copy link
Member

Choose a reason for hiding this comment

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

Extra space before Local.

AFAICT, is identical to the As<> route, except for Boolean, which is always
false with As<T>(), but is "expected" with ToT().

- FunctionCallbackInfo
Copy link
Member

Choose a reason for hiding this comment

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

Document args.Holder() ?

@addaleax addaleax self-assigned this Nov 19, 2019
addaleax added a commit to addaleax/node that referenced this pull request Nov 20, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from nodejs#26929.

Refs: nodejs#26929
addaleax added a commit that referenced this pull request Nov 22, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github
Copy link
Contributor Author

@addaleax did this better in #30552

@sam-github sam-github closed this Nov 28, 2019
@sam-github sam-github deleted the v8-api-for-node branch November 28, 2019 22:20
addaleax added a commit that referenced this pull request Nov 30, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Dec 1, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
This aims to help explain some of the internal patterns and utilities
that we use. It is by no means exhaustive, and suggestions for
additions are welcome.

Some of this is based on the existing work from #26929.

Refs: #26929

PR-URL: #30552
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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++. doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. meta Issues and PRs related to the general management of the project. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants