Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process: report ArrayBuffer memory in memoryUsage() #31550

Closed
wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

Report memory allocations performed by the ArrayBuffer::Allocator.

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

Report memory allocations performed by the `ArrayBuffer::Allocator`.
@addaleax 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 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
@nodejs-github-bot
Copy link
Collaborator

src/api/environment.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

doc/api/process.md Outdated Show resolved Hide resolved
Co-Authored-By: Richard Lau <riclau@uk.ibm.com>
src/api/environment.cc Outdated Show resolved Hide resolved
src/node_internals.h Outdated Show resolved Hide resolved
@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

src/api/environment.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

* `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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Sure, done 👍

@Trott
Copy link
Member

Trott commented Jan 31, 2020

Landed in abe6a2e

Trott pushed a commit that referenced this pull request Jan 31, 2020
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>
@Trott Trott closed this Jan 31, 2020
addaleax added a commit to addaleax/node that referenced this pull request Feb 1, 2020
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
@addaleax addaleax mentioned this pull request Feb 1, 2020
3 tasks
addaleax added a commit that referenced this pull request Feb 1, 2020
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>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
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>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
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>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere added a commit that referenced this pull request Feb 18, 2020
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
codebytere added a commit that referenced this pull request Feb 18, 2020
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
@targos 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 pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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>
codebytere added a commit to electron/electron that referenced this pull request May 27, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2020
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 8, 2020
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 9, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 15, 2020
codebytere added a commit to electron/electron that referenced this pull request Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants