src: Add does_zap_garbage to v8 HeapStatistics #8610

gareth-ellis · 2016-09-17T10:01:32Z

Checklist

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

Affected core subsystem(s) src, doc

Description of change

Following nodejs/code-and-learn#56 I have exposed does_zap_garbage to HeapStatistics.
The other fields, malloced_memory and peak_malloced_memory don't seem to be in the current version of v8 in master.

I have also updated the docs and test so that it matches the

mscdex · 2016-09-17T14:21:49Z

doc/api/v8.md

@@ -22,6 +22,7 @@ Returns an object with the following properties:
 * `total_available_size` {number}
 * `used_heap_size` {number}
 * `heap_size_limit` {number}
+* `does_zap_garbage` {bool}


I think this should be {number} instead, if we're not actually returning a boolean type (true or false).

Ack, although it might be nice to have a short description here saying that this does return a 0/1 boolean.

I don't think it's all that useful without explaining why it matters if garbage is getting zapped or not.

--zap_code_space makes V8 overwrite heap garbage with a bit pattern. The RSS footprint (resident memory set) gets bigger because it continuously touches all heap pages and that makes them less likely to get swapped out by the operating system.

cjihrig

LGTM with the requested doc change and CI of course.

Fishrock123

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

addaleax · 2016-09-17T19:21:56Z

The other fields, malloced_memory and peak_malloced_memory don't seem to be in the current version of v8 in master.

Yeah, I guess that counts as my bad. The other fields would come in with #8317 – Since it looks like that one is going to land soon, it seems reasonable to leave this PR open until #8317 is merged and it can be updated with all of the fields that are going to be in v7?

gareth-ellis · 2016-09-17T20:35:39Z

I'll keep an eye on #8317 and push the changes once that's landed and I've tested the extra two fields.

The ci run fail seems like it could be some machine issues. It'll need rerunning once the extra changes go in anyway....

addaleax · 2016-09-17T20:41:48Z

I'll keep an eye on #8317 and push the changes once that's landed and I've tested the extra two fields.

Great! It should really not take long (I’m marking this as blocked for the time being).

The ci run fail seems like it could be some machine issues. It'll need rerunning once the extra changes go in anyway....

Yeah, it’s only machine issues, don’t worry about it. We’ll run CI again when this is ready. :)

addaleax · 2016-09-22T14:06:11Z

@gareth-ellis The other fields are available on master now, so this should no longer be blocked. \o/

gareth-ellis · 2016-09-22T15:45:32Z

Updated to reflect the new fields being added to v8. ptal

addaleax

CI: https://ci.nodejs.org/job/node-test-commit/5244/

LGTM if it’s green. It would be ideal if you could use a one-line commit subject, e.g. v8: extend HeapStatistics with new fields?

gareth-ellis · 2016-09-22T19:02:42Z

Done, I kept src as the subsystem, hope that's ok

bnoordhuis

LGTM with nits.

bnoordhuis · 2016-09-23T09:38:12Z

doc/api/v8.md

+* `peak_malloced_memory` {number}
+* `does_zap_garbage` {number}
+
+does_zap_garbage is a 0/1 boolean, which signifies whether the zap_code_space


Can you put does_zap_garbage and zap_code_space between backticks and put two dashes before the latter? People will have a hard time guessing it's a command line switch otherwise.

bnoordhuis · 2016-09-23T09:39:30Z

doc/api/v8.md

+
+does_zap_garbage is a 0/1 boolean, which signifies whether the zap_code_space
+option is enabled or not. This makes V8 overwrite heap garbage with a bit
+ pattern. The RSS footprint (resident memory set) gets bigger because it 


Nit: no initial space.

src: Add does_zap_garbage, malloced_memory and peak_malloced_memory to v8 HeapStatistics Following nodejs/code-and-learn#56 I have exposed does_zap_garbage to HeapStatistics. The other fields, malloced_memory and peak_malloced_memory don't seem to be in the current version of v8 in master.

gareth-ellis · 2016-09-23T09:55:07Z

Made changes to address @bnoordhuis 's comments

jasnell · 2016-10-06T23:02:11Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/4415/

jasnell · 2016-10-07T18:22:22Z

Last CI was too red, trying again: https://ci.nodejs.org/job/node-test-pull-request/4425/

addaleax · 2016-10-07T22:22:49Z

CI was green except for FreeBSD, so here’s one more try: https://ci.nodejs.org/job/node-test-commit/5503/

gareth-ellis · 2016-10-09T08:46:30Z

Seems the freebsd failiure is three timeouts. However, it seems to be failing more often than not

addaleax · 2016-11-20T01:24:54Z

Sorry this got a bit lost, new CI: https://ci.nodejs.org/job/node-test-commit/6092/

addaleax · 2016-11-20T02:35:38Z

Landed in 440057e, thanks for the PR!

src: Add does_zap_garbage, malloced_memory and peak_malloced_memory to v8 HeapStatistics Following nodejs/code-and-learn#56 I have exposed does_zap_garbage to HeapStatistics. The other fields, malloced_memory and peak_malloced_memory don't seem to be in the current version of v8 in master. PR-URL: #8610 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * crypto: The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 * dns: Implemented `{ttl: true}` for `resolve4()` and `resolve6()`. (Ben Noordhuis) #9296 & #9296 * libuv: Upgrade to v1.10.1 (cjihrig) #9647 * process: Added a new `external` property to the data returned by `memoryUsage()`. (Fedor Indutny) #9587 * V8 (dep): Upgrade to v5.4.500.43 (Michaël Zasso) #9697 * v8: The data returned by `getHeapStatistics()` now includes three new fields: `malloced_memory`, `peak_malloced_memory`, and `does_zap_garbage`. (Gareth Ellis) #8610 PR-URL: #9745

This is a security release impacting Windows 10 users. Notable changes: * crypto: The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) #9398 * dns: Implemented `{ttl: true}` for `resolve4()` and `resolve6()`. (Ben Noordhuis) #9296 & #9296 * libuv: Upgrade to v1.10.1 (cjihrig) #9647 - Fixed a potential buffer overflow when writing data to console on Windows 10. (CVE-2016-9551) * process: Added a new `external` property to the data returned by `memoryUsage()`. (Fedor Indutny) #9587 * tls: Fixed a memory leak when writes were queued on TLS connection that was destroyed during handshake. (Fedor Indutny) #9626 * V8 (dep): Upgrade to v5.4.500.43 (Michaël Zasso) #9697 * v8: The data returned by `getHeapStatistics()` now includes three new fields: `malloced_memory`, `peak_malloced_memory`, and `does_zap_garbage`. (Gareth Ellis) #8610 PR-URL: #9745

This is a security release impacting Windows 10 users. Notable changes: * crypto: The `Decipher` methods `setAuthTag()` and `setAAD` now return `this`. (Kirill Fomichev) nodejs/node#9398 * dns: Implemented `{ttl: true}` for `resolve4()` and `resolve6()`. (Ben Noordhuis) nodejs/node#9296 & nodejs/node#9296 * libuv: Upgrade to v1.10.1 (cjihrig) nodejs/node#9647 - Fixed a potential buffer overflow when writing data to console on Windows 10. (CVE-2016-9551) * process: Added a new `external` property to the data returned by `memoryUsage()`. (Fedor Indutny) nodejs/node#9587 * tls: Fixed a memory leak when writes were queued on TLS connection that was destroyed during handshake. (Fedor Indutny) nodejs/node#9626 * V8 (dep): Upgrade to v5.4.500.43 (Michaël Zasso) nodejs/node#9697 * v8: The data returned by `getHeapStatistics()` now includes three new fields: `malloced_memory`, `peak_malloced_memory`, and `does_zap_garbage`. (Gareth Ellis) nodejs/node#8610 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Sep 17, 2016

mscdex reviewed Sep 17, 2016

View reviewed changes

addaleax added dont-land-on-v4.x labels Sep 17, 2016

cjihrig approved these changes Sep 17, 2016

View reviewed changes

Fishrock123 reviewed Sep 17, 2016

View reviewed changes

addaleax added the blocked PRs that are blocked by other issues or PRs. label Sep 17, 2016

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:10

addaleax removed the blocked PRs that are blocked by other issues or PRs. label Sep 22, 2016

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 22, 2016

addaleax approved these changes Sep 22, 2016

View reviewed changes

bnoordhuis approved these changes Sep 23, 2016

View reviewed changes

jasnell self-assigned this Oct 6, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

addaleax closed this Nov 20, 2016

addaleax unassigned jasnell Nov 20, 2016

Fishrock123 mentioned this pull request Nov 22, 2016

v7.2.0 proposal #9745

Merged

2 tasks

bnb mentioned this pull request Mar 11, 2020

Move to Electron's docs-parser tooling for Node.js documentation #32206

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: Add does_zap_garbage to v8 HeapStatistics #8610

src: Add does_zap_garbage to v8 HeapStatistics #8610

gareth-ellis commented Sep 17, 2016

mscdex Sep 17, 2016

addaleax Sep 17, 2016

bnoordhuis Sep 17, 2016

cjihrig left a comment

Fishrock123 left a comment

addaleax commented Sep 17, 2016

gareth-ellis commented Sep 17, 2016 •

edited

Loading

addaleax commented Sep 17, 2016

addaleax commented Sep 22, 2016

gareth-ellis commented Sep 22, 2016

addaleax left a comment

gareth-ellis commented Sep 22, 2016

bnoordhuis left a comment

bnoordhuis Sep 23, 2016 •

edited

Loading

bnoordhuis Sep 23, 2016

gareth-ellis commented Sep 23, 2016

jasnell commented Oct 6, 2016

jasnell commented Oct 7, 2016

addaleax commented Oct 7, 2016

gareth-ellis commented Oct 9, 2016

addaleax commented Nov 20, 2016

addaleax commented Nov 20, 2016

src: Add does_zap_garbage to v8 HeapStatistics #8610

src: Add does_zap_garbage to v8 HeapStatistics #8610

Conversation

gareth-ellis commented Sep 17, 2016

Checklist

Affected core subsystem(s) src, doc

Description of change

mscdex Sep 17, 2016

Choose a reason for hiding this comment

addaleax Sep 17, 2016

Choose a reason for hiding this comment

bnoordhuis Sep 17, 2016

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

addaleax commented Sep 17, 2016

gareth-ellis commented Sep 17, 2016 • edited Loading

addaleax commented Sep 17, 2016

addaleax commented Sep 22, 2016

gareth-ellis commented Sep 22, 2016

addaleax left a comment

Choose a reason for hiding this comment

gareth-ellis commented Sep 22, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Sep 23, 2016

Choose a reason for hiding this comment

gareth-ellis commented Sep 23, 2016

jasnell commented Oct 6, 2016

jasnell commented Oct 7, 2016

addaleax commented Oct 7, 2016

gareth-ellis commented Oct 9, 2016

addaleax commented Nov 20, 2016

addaleax commented Nov 20, 2016

gareth-ellis commented Sep 17, 2016 •

edited

Loading

bnoordhuis Sep 23, 2016 •

edited

Loading