-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib: reintroduce v8 module #131
Conversation
built with node. These interfaces are subject to change by upstream and are | ||
therefore not covered under the stability index. | ||
|
||
### Event: 'gc' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decision time: remove the documentation for the 'gc' event or document that it's dangerous? I'm leaning toward the former. That would let us remove the gc machinery from src/. I don't need it for strong-agent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Removing 'gc'
event from documentation sounds like a better plan.
bf99367
to
24bf8a8
Compare
This is actually a really cool idea! It also introduces scope for other v8 based functionality in the future. |
|
||
Set additional V8 command line flags. Use with care; changing settings | ||
after the VM has started may result in unpredictable behavior, including | ||
crashes and data loss. Or it may simply do nothing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could reference one of the docs here for this.
I tried to research and test what flags have effect and which don't.
More work needs to be done here, but it's a good place to start looking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can others weigh in? I appreciate Thorsten's effort but I don't want to give off an implied promise of support by linking to documentation. Or is that overly cautious? I lean towards pointing people to --v8-options
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be in favor of something like:
The V8 options available for a version of node may be determined by running `node --v8-options`. An unofficial, community-maintained list of options and their effects is available [here](https://github.com/thlorenz/v8-flags/blob/master/flags-0.11.md).
Since it stresses that the linked guide isn't the "source of truth" -- that lies within v8 -- but gives casual readers a chance to see what's available through this API.
(Also, great work @thlorenz!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnoordhuis Totally agree that implied promises given by linking are bad especially if linked content ends up not being maintained.
On the upside, the v8-flags documentation is entirely generated and can be updated whenever the API changes by simply running npm run flag-doc
.
The only part of the docs that is manual and gets merged with the auto generated one is specified here. I was secretly hoping for some v8 folks to jump in and help with adding more details via simple update steps
If linking to a third party thing is worrisome I'm open to adapt the generated docs to be included somewhere within the official node docs.
Thanks @chrisdickinson macros are lots of fun :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thlorenz I incorporated @chrisdickinson's feedback earlier today; it makes it clear that the linked page is a community effort so I think we're good. Is https://github.com/thlorenz/v8-flags/blob/master/flags-0.11.md the best page to link to or is there a better one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until we generate docs for 12, this is the best one.
In general I believe it's a good idea to keep things like v8-flags in user land. I'd imagine you'd get quite a lot of issues just related to the above in this repo if you include it with node proper. Obviously a point for including them here is the fact that they depend on v8 APIs and thus it is easier to keep them from breaking all the time. I solved this in v8-flags by basically generating the code for the specific node version used via macros, but it's kinda hacky at best. |
@trevnorris Undocumented the 'gc' event, PTAL. I'll follow up with a PR that removes its machinery when this PR lands. |
I don't know, I think the documentation makes it clear that unexpected things can happen. I hope and assume people are smart enough not to file bug reports about that. |
@trevnorris Ping. This is blocking another PR. |
@chrisdickinson Can I persuade you to review this PR? |
@bnoordhuis will do. reviewing now. |
@@ -35,3 +35,4 @@ | |||
@include debugger | |||
@include cluster | |||
@include smalloc | |||
@include v8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also want to add V8 to api/doc/_toc.markdown
(for it to show up on the index.)
Pending the _toc.markdown and doc change, LGTM. |
8e90f36
to
7219b27
Compare
@chrisdickinson Cheers. Incorporated the feedback but I changed |
I introduced this module over a year ago in a pull request as the v8 module but it was quickly subsumed by the tracing module. The tracing module was recently removed again and that is why this commit introduces the v8 module again, including the new features it picked up commits d23ac0e and f8076c4. PR-URL: nodejs#131 Reviewed-By: Chris Dickinson <[email protected]> Reviewed-By: Christian Tellnes <[email protected]> Reviewed-By: Thorsten Lorenz <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
LGTM! |
7219b27
to
db595b2
Compare
Thanks everyone, landed in db595b2. |
I introduced this module over a year ago in a pull request as the v8
module but it was quickly subsumed by the tracing module.
The tracing module was recently removed again and that is why this
commit introduces the v8 module again, including the new features it
picked up commits d23ac0e and f8076c4.
R=@trevnorris