doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20431

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

doc/api/addons.md

+` --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

doc/api/addons.md

+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 <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org>

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 <jasnell@gmail.com> Reviewed-By: Yang Guo <yangguo@chromium.org>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20431

doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20431

isurusiri commented Apr 30, 2018

vsemozhetbyt commented May 1, 2018

Trott commented May 1, 2018

Trott commented May 1, 2018

hashseed commented May 1, 2018

isurusiri commented May 2, 2018

Trott May 2, 2018 •

edited

Loading

isurusiri May 3, 2018

Trott commented May 3, 2018

addaleax commented May 6, 2018

BridgeAR commented May 29, 2018

jasnell commented Sep 10, 2018

addaleax commented Sep 11, 2018

Trott Sep 17, 2018

hashseed commented Sep 17, 2018

Trott commented Sep 17, 2018

addaleax commented Sep 17, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 19, 2018

Trott commented Sep 19, 2018

doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20431

doc: modified docs to reflect how to invoke gc on Wrapping C++ objects #20431

Conversation

isurusiri commented Apr 30, 2018

Checklist

vsemozhetbyt commented May 1, 2018

Trott commented May 1, 2018

Trott commented May 1, 2018

hashseed commented May 1, 2018

isurusiri commented May 2, 2018

Trott May 2, 2018 • edited Loading

Choose a reason for hiding this comment

isurusiri May 3, 2018

Choose a reason for hiding this comment

Trott commented May 3, 2018

addaleax commented May 6, 2018

BridgeAR commented May 29, 2018

jasnell commented Sep 10, 2018

addaleax commented Sep 11, 2018

Trott Sep 17, 2018

Choose a reason for hiding this comment

hashseed commented Sep 17, 2018

Trott commented Sep 17, 2018

addaleax commented Sep 17, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 18, 2018

Trott commented Sep 19, 2018

Trott commented Sep 19, 2018

Trott May 2, 2018 •

edited

Loading