process: report ArrayBuffer memory in `memoryUsage()` #31550

+* `arrayBuffers` refers to memory allocated for `ArrayBuffer`s and
+  `SharedArrayBuffer`s, including all Node.js [`Buffer`][]s.
+  This is also included in the `external` value. When Node.js is used as an
+  embedded library, this value may be `0`.


sorry for being raw here, but I have these questions:

When Node.js is used as an embedded library, this value may be 0.

why? is it because Node.js cannot truly account for the allocations of the embedder?

where? which part of the code takes the decision based on the embedded state?

may be 0 - why again - why this decision cannot be deterministic as opposed to probabilistic? which factors decide the outcome? how would one use those factors to decide whether the value is trustworthy or not?

sorry for being raw here, but I have these questions:

When Node.js is used as an embedded library, this value may be 0.

why? is it because Node.js cannot truly account for the allocations of the embedder?

Because embedders can specify their own ArrayBuffer::Allocator, which would not necessarily track allocations (e.g. Electron does this).

where? which part of the code takes the decision based on the embedded state?

Inside the C++ MemoryUsage() binding, there’s a array_buffer_allocator == nullptr check. If there is no NodeArrayBufferAllocator* that the embedder informed us about, then the method sets the field for array buffers to 0.

may be 0 - why again - why this decision cannot be deterministic as opposed to probabilistic?

I don’t really understand this question, sorry – there is nothing probabilistic involved here, I think?

which factors decide the outcome?

I think that’s also answered by the first question, but let me know if I’m misunderstanding.

how would one use those factors to decide whether the value is trustworthy or not?

I think it’s safe to say that the value is trustworthy when it’s not 0.

I didn’t want to go into a ton of detail in the documentation here – imo there should be an indication that the value may not always be provided, and that tools should not make the expectation that the value is non-zero. But for a tool it’s not really going to matter why exactly the value can’t be reported, I think, other than that it’s related to the fact that the Node.js code is running in a specific environment that behaves differently.

there is nothing probabilistic involved here, I think?

I termed it as probabilistic because of may be 0 phrase. But based on your explanation to the rest of the questions, I understand it as:

if embedders use only non-node.js allocators without a tracker, then this value WILL be 0 (not truly reflecting actual consumption), and if embedders rely on node.js' allocators, then the value will be accurate - zero or non-zero

Are we on the same page?

@gireeshpunathil Yes, we are 👍 I feel like the most important thing is that the doc makes clear that this does not affect standard Node.js binaries.

Maybe a simple "because such allocations may not be tracked" would be good?

@jasnell Sure, done 👍

Trott · 2020-01-31T00:34:36Z

Landed in abe6a2e

Report memory allocations performed by the `ArrayBuffer::Allocator`. PR-URL: #31550 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

abe6a2e introduced a test that verifies that ArrayBuffer allocations are tracked. However, V8 supports concurrent freeing of such allocations on background threads, meaning that the results may be subject to race conditions sometimes. Disabling concurrent freeing makes the test pass consistently. Refs: nodejs#31550

abe6a2e introduced a test that verifies that ArrayBuffer allocations are tracked. However, V8 supports concurrent freeing of such allocations on background threads, meaning that the results may be subject to race conditions sometimes. Disabling concurrent freeing makes the test pass consistently. Refs: #31550 PR-URL: #31602 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Report memory allocations performed by the `ArrayBuffer::Allocator`. PR-URL: #31550 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

abe6a2e introduced a test that verifies that ArrayBuffer allocations are tracked. However, V8 supports concurrent freeing of such allocations on background threads, meaning that the results may be subject to race conditions sometimes. Disabling concurrent freeing makes the test pass consistently. Refs: #31550 PR-URL: #31602 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * async_hooks * add executionAsyncResource (Matteo Collina) #30959 * crypto * add crypto.diffieHellman (Tobias Nießen) #31178 * add DH support to generateKeyPair (Tobias Nießen) #31178 * simplify DH groups (Tobias Nießen) #31178 * add key type 'dh' (Tobias Nießen) #31178 * test * skip keygen tests on arm systems (Tobias Nießen) #31178 * perf_hooks * add property flags to GCPerformanceEntry (Kirill Fomichev) #29547 * process * report ArrayBuffer memory in `memoryUsage()` (Anna Henningsen) #31550 * readline * make tab size configurable (Ruben Bridgewater) #31318 * report * add support for Workers (Anna Henningsen) #31386 * worker * add ability to take heap snapshot from parent thread (Anna Henningsen) #31569 * added new collaborators * add ronag to collaborators (Robert Nagy) #31498 PR-URL: #31837

Report memory allocations performed by the `ArrayBuffer::Allocator`. PR-URL: nodejs#31550 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

abe6a2e introduced a test that verifies that ArrayBuffer allocations are tracked. However, V8 supports concurrent freeing of such allocations on background threads, meaning that the results may be subject to race conditions sometimes. Disabling concurrent freeing makes the test pass consistently. Refs: nodejs#31550 PR-URL: nodejs#31602 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Report memory allocations performed by the `ArrayBuffer::Allocator`. PR-URL: #31550 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

abe6a2e introduced a test that verifies that ArrayBuffer allocations are tracked. However, V8 supports concurrent freeing of such allocations on background threads, meaning that the results may be subject to race conditions sometimes. Disabling concurrent freeing makes the test pass consistently. Refs: #31550 PR-URL: #31602 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs/node#31550

process: report ArrayBuffer memory in memoryUsage()

20947c7

Report memory allocations performed by the `ArrayBuffer::Allocator`.

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 28, 2020

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Jan 28, 2020

richardlau reviewed Jan 28, 2020

View reviewed changes

src/api/environment.cc Outdated Show resolved Hide resolved

fixup! process: report ArrayBuffer memory in memoryUsage()

5e624ba

richardlau approved these changes Jan 28, 2020

View reviewed changes

doc/api/process.md Outdated Show resolved Hide resolved

fixup! process: report ArrayBuffer memory in memoryUsage()

ba44913

Co-Authored-By: Richard Lau <riclau@uk.ibm.com>

cjihrig approved these changes Jan 28, 2020

View reviewed changes

devsnek approved these changes Jan 28, 2020

View reviewed changes

bnoordhuis reviewed Jan 29, 2020

View reviewed changes

src/api/environment.cc Outdated Show resolved Hide resolved

src/node_internals.h Outdated Show resolved Hide resolved

fixup! fixup! process: report ArrayBuffer memory in memoryUsage()

a6e74b3

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2020

bnoordhuis reviewed Jan 29, 2020

View reviewed changes

src/api/environment.cc Outdated Show resolved Hide resolved

fixup! process: report ArrayBuffer memory in memoryUsage()

4267523

bnoordhuis approved these changes Jan 29, 2020

View reviewed changes

jasnell approved these changes Jan 29, 2020

View reviewed changes

Trott approved these changes Jan 30, 2020

View reviewed changes

gireeshpunathil reviewed Jan 30, 2020

View reviewed changes

gireeshpunathil approved these changes Jan 30, 2020

View reviewed changes

fixup! process: report ArrayBuffer memory in memoryUsage()

a3014ce

Trott closed this Jan 31, 2020

addaleax mentioned this pull request Feb 1, 2020

test: fix flaky test-memory-usage #31602

Closed

3 tasks

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

codebytere added a commit to electron/electron that referenced this pull request May 27, 2020

Fixup V8 Reallocate patch

2eef256

nodejs/node#31550

codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020

Fixup V8 Reallocate patch

37d287f

nodejs/node#31550

jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020

Fixup V8 Reallocate patch

92bbc1b

nodejs/node#31550

jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020

Fixup V8 Reallocate patch

e98178f

nodejs/node#31550

codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020

Fixup V8 Reallocate patch

45d103d

nodejs/node#31550

codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020

Fixup V8 Reallocate patch

77cfb19

nodejs/node#31550

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: report ArrayBuffer memory in `memoryUsage()` #31550

process: report ArrayBuffer memory in `memoryUsage()` #31550

addaleax commented Jan 28, 2020

nodejs-github-bot commented Jan 28, 2020

nodejs-github-bot commented Jan 28, 2020

nodejs-github-bot commented Jan 29, 2020

nodejs-github-bot commented Jan 29, 2020

nodejs-github-bot commented Jan 29, 2020

gireeshpunathil Jan 30, 2020

addaleax Jan 30, 2020

gireeshpunathil Jan 30, 2020

addaleax Jan 30, 2020

jasnell Jan 30, 2020

addaleax Jan 30, 2020

Trott commented Jan 31, 2020

process: report ArrayBuffer memory in memoryUsage() #31550

process: report ArrayBuffer memory in memoryUsage() #31550

Conversation

addaleax commented Jan 28, 2020

Checklist

nodejs-github-bot commented Jan 28, 2020

nodejs-github-bot commented Jan 28, 2020

nodejs-github-bot commented Jan 29, 2020

nodejs-github-bot commented Jan 29, 2020

nodejs-github-bot commented Jan 29, 2020

gireeshpunathil Jan 30, 2020

Choose a reason for hiding this comment

addaleax Jan 30, 2020

Choose a reason for hiding this comment

gireeshpunathil Jan 30, 2020

Choose a reason for hiding this comment

addaleax Jan 30, 2020

Choose a reason for hiding this comment

jasnell Jan 30, 2020

Choose a reason for hiding this comment

addaleax Jan 30, 2020

Choose a reason for hiding this comment

Trott commented Jan 31, 2020

process: report ArrayBuffer memory in `memoryUsage()` #31550

process: report ArrayBuffer memory in `memoryUsage()` #31550