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

vm: implement vm.measureMemory() for per-context memory measurement #31824

Closed
wants to merge 4 commits into from

Conversation

@joyeecheung
Copy link
Member

joyeecheung commented Feb 17, 2020

This patch implements vm.measureMemory() with the new
v8::Isolate::MeasureMemory() API to measure per-context memory
usage. This should be experimental, since detailed memory
measurement requires further integration with the V8 API
that should be available in a future V8 update.

Refs: https://github.com/ulan/performance-measure-memory

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
@joyeecheung joyeecheung added the vm label Feb 17, 2020
@joyeecheung joyeecheung force-pushed the joyeecheung:measure-memory branch 2 times, most recently Feb 17, 2020
This patch implements `vm.measureMemory()` with the new
`v8::Isolate::MeasureMemory()` API to measure per-context memory
usage. This should be experimental, since detailed memory
measurement requires further integration with the V8 API
that should be available in a future V8 update.
@joyeecheung joyeecheung force-pushed the joyeecheung:measure-memory branch to 6c04e3c Feb 17, 2020
@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 17, 2020

Is there a reason we don't want to expose this on performance instead?

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Feb 17, 2020

Is there a reason we don't want to expose this on performance instead?

@devsnek This is currently different from the API proposed in https://github.com/ulan/performance-measure-memory in that an options argument gives the user more control over how the memory can be measured and it supports cross-context measurement for contexts created with vm.createContext(). For Web compat, a performance API could be eventually built on top of vm but I think it's useful to provide a more powerful API for user to experiment with.

lib/vm.js Outdated Show resolved Hide resolved
src/node_contextify.cc Show resolved Hide resolved
test/parallel/test-vm-measure-memory.js Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
lib/vm.js Outdated Show resolved Hide resolved
src/node_contextify.cc Show resolved Hide resolved
test/parallel/test-vm-measure-memory.js Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
doc/api/vm.md Outdated Show resolved Hide resolved
@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 18, 2020

@joyeecheung did you have any interest in the Context.current() approach I mentioned in irc?

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Feb 18, 2020

@devsnek That sounds interesting but seems orthogonal to what this PR does?

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Feb 18, 2020

@joyeecheung well if we wanted to use that approach instead i assume we wouldn't also have the static method this pr adds, which is why i ask

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Feb 18, 2020

@devsnek A measureMemory method could be added to the prototype of the Context class but that when the API does land, yes, but I think it's better to decouple the functionality of measuring the per-context memory from an API that has not landed yet, and this provides the functionality for contextified objects which is something that does exist in the wild (it could also take the new class when the time comes, and avoids the overhead of creating an extra wrapper for the invoking context).

joyeecheung added 2 commits Feb 18, 2020
…ement
@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Feb 18, 2020

@bnoordhuis @addaleax @jasnell @lundibundi Thanks for the reviews. I've updated the PR, PTAL.

  • Use strings instead of vm.constants
  • Pass the context via options.context instead of a second argument
  • Return a rejection containing ERR_CONTEXT_NOT_INITIALIZED when the context is not initialized instead of failing silently
doc/api/errors.md Outdated Show resolved Hide resolved
…ement

Co-Authored-By: Colin Ihrig <cjihrig@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 26, 2020
This patch implements `vm.measureMemory()` with the new
`v8::Isolate::MeasureMemory()` API to measure per-context memory
usage. This should be experimental, since detailed memory
measurement requires further integration with the V8 API
that should be available in a future V8 update.

PR-URL: #31824
Refs: https://github.com/ulan/performance-measure-memory
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Feb 26, 2020

Landed in fb73045, thanks!

codebytere added a commit that referenced this pull request Feb 27, 2020
This patch implements `vm.measureMemory()` with the new
`v8::Isolate::MeasureMemory()` API to measure per-context memory
usage. This should be experimental, since detailed memory
measurement requires further integration with the V8 API
that should be available in a future V8 update.

PR-URL: #31824
Refs: https://github.com/ulan/performance-measure-memory
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@codebytere codebytere mentioned this pull request Feb 29, 2020
codebytere added a commit that referenced this pull request Mar 1, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 3, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
codebytere added a commit that referenced this pull request Mar 4, 2020
Notable changes:

* async_hooks
  * introduce async-context API (vdeturckheim) #26540
* stream
  * support passing generator functions into pipeline() (Robert Nagy) #31223
* tls
  * expose SSL\_export\_keying\_material (simon) #31814
* vm
  * implement vm.measureMemory() for per-context memory measurement (Joyee Cheung) #31824

PR-URL: #32027
@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented Mar 26, 2020

It seems the example code is wrong, the docs suggest the function should be called like this vm.measureMemory({ mode: 'detailed' }, context), shouldn't it be vm.measureMemory({ mode: 'detailed', context })?

@gengjiawen

This comment has been minimized.

Copy link
Member

gengjiawen commented Mar 26, 2020

It seems the example code is wrong, the docs suggest the function should be called like this vm.measureMemory({ mode: 'detailed' }, context), shouldn't it be vm.measureMemory({ mode: 'detailed', context })?

Do you mind send a PR for this ?

@SimonSchick

This comment has been minimized.

Copy link
Contributor

SimonSchick commented Mar 26, 2020

I'm currently busy updating the node type definitions to v11 (hence this comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.