doc: modified docs to reflect how to invoke gc on Wrapping C++ objects by isurusiri · Pull Request #20431 · nodejs/node

isurusiri · 2018-04-30T16:28:56Z

Currently the documentation for Wrapping C++ Objects doesn't explain
how to destruct an object by explicitly invoking the garbage collector.
This commit includes a modification to docs that explains how to force
the garbage collector to clear objects using V8's command line flags.

Fixes: #19876

Checklist

documentation is changed or added
commit message follows commit guidelines

Currently the documentation for Wrapping C++ Objects doesn't explain how to destruct an object by explicitly invoking the garbage collector. This commit includes a modification to docs that explains how to force the garbage collector to clear objects using V8's command line flags. Fixes: #19876

vsemozhetbyt · 2018-05-01T04:11:53Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/642/

Trott · 2018-05-01T06:04:45Z

@nodejs/addon-api Is this information we'd want to include?

Since these are V8 internal flags, @nodejs/v8 also might have opinions on whether we should include this information in our docs or not.

If we do want to include it, the text could benefit from a bit of editing. Maybe @nodejs/documentation can offer some suggested alternative wording etc.

Trott · 2018-05-01T06:07:03Z

Prior conversation for context: #20269

hashseed · 2018-05-01T06:55:23Z

Personally I don't think using --gc-global and --gc-interval is any better than triggering gc directly via gc(). None of these are public APIs. Though people are probably more likely to avoid --gc-inteval since it seriously degrades performance.

Can't we write these demo cases or tests in a way that at the end it keeps allocating until the destroy callback fires? Failing tests become timeouts, which is not ideal, but aside from that this is nicer than recommending V8's debugging flags.

isurusiri · 2018-05-02T01:05:10Z

Conclusion of previous discussions (#20269 (comment)) is that we shouldn't document something like allocating a large number of objects to trigger GC mainly because it is going to official docs and coding a bad practice that could be misleading. Therefore we could document v8 flags highlighting those are only for testing purposes and provided by V8 would be enough.

Trott · 2018-05-02T03:28:10Z

+` --gc_interval ` forces V8 to perform garbage collection after a given amount
+of allocations. Although, it is recommended to limit V8 command line flags
+for testing purposes only, since these are primarily debug flags.
+


How about this?:

The destructor for a wrapper object will run when the object is garbage-collected. For destructor testing, there are command-line flags that can be used to make it possible to force garbage collection. These flags are provided by the underlying V8 JavaScript engine. They are subject to change or removal at any time. They are not documented by Node.js or V8, and they should never be used outside of testing.

Or if that's too mysterious, maybe this?:

The destructor for a wrapper object will run when the object is garbage-collected.

Leave it as an exercise for the reader to figure out how to force garbage collection for testing purposes. People can find that in blog posts and StackOverflow etc. (For example, https://stackoverflow.com/q/27321997/436641.) But we don't have to document it.

Agree with leaving reader to figure it out since we are not recommending to use command line flags outside of testing. First options is informative and I'll proceed with that.

Currently the documentation for Wrapping C++ Objects doesn't explain how the destructor executes when garbage-collector runs. This commit includes a modification to docs that instructs how the destructor for wrapper objects will run and options available to test destructor Fixes: http://github.com/nodejs/node/issues/19876

Trott · 2018-05-03T05:38:10Z

@bnoordhuis @hashseed PTAL

addaleax · 2018-05-06T19:36:45Z

@hashseed I think for --expose-gc not having to be considered public API the ship has sailed a long time ago. Not saying that’s a good thing – I just think it’s something we’re locked into. A lot of test suites for Node addons use it in the way described here, to the degree that some test runners/frameworks have explicit builtin support for making tests run with this flag (e.g. mocha).

Regarding this PR, I like the idea but it’s probably not very meaningful to say “There are undocumented flags that do exactly what you need right now, please don’t look them up because that’s a bad thing”. I’d be okay with officially recommending --expose-gc, but don’t really want to do that over the V8 team’s head either.

BridgeAR · 2018-05-29T20:03:38Z

@hashseed I think @addaleax has a good point here. That flag is indeed pretty commonly used.

jasnell · 2018-09-10T18:12:14Z

Ping... what's the status on this one?

addaleax · 2018-09-11T10:34:40Z

It might be good to hear what @hashseed thinks of the above – again, I’m really not saying I’m a fan of us effectively making a V8 flag public API, but we have to face the fact that it effectively is.

Trott · 2018-09-17T04:32:25Z

+The destructor for a wrapper object will run when the object is 
+garbage-collected. For destructor testing, there are command-line flags that
+can be used to make it possible to force garbage collection. These flags are
+provided by the underlying V8 JavaScript engine. They are subjected to change


Nit: subjected -> subject

hashseed · 2018-09-17T05:20:29Z

I'm happy with the current wording.

Trott · 2018-09-17T05:42:37Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/902/

addaleax · 2018-09-17T15:54:54Z

CI again: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/910/

Trott · 2018-09-18T05:20:22Z

Ci again again: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/919/

Trott · 2018-09-18T05:30:54Z

Hmmm, the same unrelated TLS test failing three times. Let's try a fresh run rather than a Replay and see if that clears up some git stuff leftover from another run or something weird like that which might be causing it. Yeah, I'm making it up. Let's try it anyway: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/920/

Trott · 2018-09-18T05:36:37Z

That time, it was a hard build failure. I've pinged build folks to take a look and hopefully it will be fixed soon and we can try again. Sorry about the delay.

Trott · 2018-09-18T13:45:17Z

Looks like I kicked off the job incorrectly. Let's try again: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/926/

Trott · 2018-09-18T19:15:07Z

Giving up on lite CI, going for full CI: https://ci.nodejs.org/job/node-test-pull-request/17299/

Trott · 2018-09-19T04:36:59Z

Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20844/

Currently the documentation for Wrapping C++ Objects doesn't explain how to destruct an object by explicitly invoking the garbage collector. This commit includes a modification to docs that explains how to force the garbage collector to clear objects using V8's command line flags. Fixes: nodejs#19876 PR-URL: nodejs#20431 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yang Guo <[email protected]>

Trott · 2018-09-19T05:58:50Z

Finally...

Landed in a1381fa

Currently the documentation for Wrapping C++ Objects doesn't explain how to destruct an object by explicitly invoking the garbage collector. This commit includes a modification to docs that explains how to force the garbage collector to clear objects using V8's command line flags. Fixes: #19876 PR-URL: #20431 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yang Guo <[email protected]>

isurusiri added 2 commits April 30, 2018 21:43

nodejs-github-bot added addons Issues and PRs related to native addons. doc Issues and PRs related to the documentations. labels Apr 30, 2018

Trott reviewed May 2, 2018

View reviewed changes

jasnell approved these changes May 23, 2018

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

Trott reviewed Sep 17, 2018

View reviewed changes

hashseed approved these changes Sep 17, 2018

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 17, 2018

squash! nit fix

1056c8f

Trott closed this Sep 19, 2018

targos mentioned this pull request Sep 19, 2018

Release proposal: v10.11.0 #22932

Merged

Uh oh!

Conversation

isurusiri commented Apr 30, 2018

Checklist

Uh oh!

vsemozhetbyt commented May 1, 2018

Uh oh!

Trott commented May 1, 2018

Uh oh!

Trott commented May 1, 2018

Uh oh!

hashseed commented May 1, 2018

Uh oh!

isurusiri commented May 2, 2018

Uh oh!

Trott May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isurusiri May 3, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented May 3, 2018

Uh oh!

addaleax commented May 6, 2018

Uh oh!

BridgeAR commented May 29, 2018

Uh oh!

jasnell commented Sep 10, 2018

Uh oh!

addaleax commented Sep 11, 2018

Uh oh!

Trott Sep 17, 2018

Choose a reason for hiding this comment

Uh oh!

hashseed commented Sep 17, 2018

Uh oh!

Trott commented Sep 17, 2018

Uh oh!

addaleax commented Sep 17, 2018

Uh oh!

Trott commented Sep 18, 2018

Uh oh!

Trott commented Sep 18, 2018

Uh oh!

Trott commented Sep 18, 2018

Uh oh!

Trott commented Sep 18, 2018

Uh oh!

Trott commented Sep 18, 2018

Uh oh!

Trott commented Sep 19, 2018

Uh oh!

Trott commented Sep 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Trott May 2, 2018 •

edited

Loading