src: Add does_zap_garbage to v8 HeapStatistics by gareth-ellis · Pull Request #8610 · nodejs/node

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

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

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

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 c5ce7f4Compare 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 83c7a88Compare 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
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

gareth-ellis commented Sep 17, 2016

Checklist

Affected core subsystem(s) src, doc

Description of change

Uh oh!

mscdexSep 17, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxSep 17, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuisSep 17, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 17, 2016

Uh oh!

gareth-ellis commented Sep 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Sep 17, 2016

Uh oh!

addaleax commented Sep 22, 2016

Uh oh!

gareth-ellis commented Sep 22, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

gareth-ellis commented Sep 22, 2016

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuisSep 23, 2016• 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!

bnoordhuisSep 23, 2016

Choose a reason for hiding this comment

Uh oh!

gareth-ellis commented Sep 23, 2016

Uh oh!

jasnell commented Oct 6, 2016

Uh oh!

jasnell commented Oct 7, 2016

Uh oh!

addaleax commented Oct 7, 2016

Uh oh!

gareth-ellis commented Oct 9, 2016

Uh oh!

addaleax commented Nov 20, 2016

Uh oh!

addaleax commented Nov 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gareth-ellis commented Sep 17, 2016•
edited
Loading

bnoordhuisSep 23, 2016•
edited
Loading