Skip to content

Conversation

@joyeecheung
Copy link
Member

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

@joyeecheungjoyeecheung added the vm Issues and PRs related to the vm subsystem. label Feb 17, 2020
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 17, 2020
@joyeecheungjoyeecheungforce-pushed the measure-memory branch 2 times, most recently from ba8d46b to bc6f5feCompareFebruary 17, 2020 08:51
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.
@devsnek
Copy link
Member

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

@joyeecheung
Copy link
MemberAuthor

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.

@devsnek
Copy link
Member

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

@joyeecheung
Copy link
MemberAuthor

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

@devsnek
Copy link
Member

@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
Copy link
MemberAuthor

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
Copy link
MemberAuthor

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

@joyeecheungjoyeecheung added semver-minor PRs that contain new features and should be released in the next minor version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 19, 2020
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@joyeecheung
Copy link
MemberAuthor

Landed in fb73045, thanks!

codebytere pushed 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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@codebyterecodebytere 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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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

@limerickgds
Copy link

@joyeecheung i did a test, The data is not as expected,:

'use strict'; const vm = require('vm'); async function getMeasureMemory(context){global.gc(); return vm.measureMemory({mode: 'summary', context })} let globalStore = []; const operateStore ={add: () =>{let i = 1000; while (i--){globalStore.push('test'.repeat(10000))} }, remove: () =>{globalStore = []}, }; const script = ` var store = []; fn ={add: () =>{let i = 1000; while(i--){store.push('test'.repeat(10000)) } }, remove: () =>{store = []} } `; const context = vm.createContext({fn: () =>{}, console, }); vm.runInContext(script, context); async function measureMemoryTest(){let result ={}; result = await getMeasureMemory(context); console.log('start: \n', result); context.fn.add(); result = await getMeasureMemory(context); console.log('after add once: \n', result); context.fn.remove(); result = await getMeasureMemory(context); console.log('after remove all: \n', result); operateStore.add(); result = await getMeasureMemory(context); console.log('after add global context: \n', result); result = await getMeasureMemory(vm.createContext({})); console.log('after add global: \n', result); result = await getMeasureMemory(context); console.log('after add global context: \n', result)} measureMemoryTest(); 

new context size ~ global
image

@joyeecheung
Copy link
MemberAuthor

@limerickgds This no longer works after the recent V8 update - the promises returned by vm.measureMemory() will not keep the process alive, so it's uncertain how many requests will be completed before the program executes (in the output, there may be some lines being logged, or nothing at all). I added support for the execution option so that eager mode can be used instead of global.gc()#32988 with that the new numbers should look like this

'use strict';constvm=require('vm');require('util').inspect.defaultOptions.depth=100;functiongetMeasureMemory(){returnvm.measureMemory({mode: 'detailed',execution: 'eager'});}letglobalStore=[];constoperateStore={add: ()=>{leti=1000;while(i--){globalStore.push('test'.repeat(10000));}},remove: ()=>{globalStore=[];},};constscript=` var store = []; fn ={ add: () =>{ let i = 1000; while(i--){store.push('test'.repeat(10000)) } }, remove: () =>{ store = []; } }`;constcontext=vm.createContext({fn: ()=>{}, console,});vm.runInContext(script,context);asyncfunctionmeasureMemoryTest(){letresult={};result=awaitgetMeasureMemory();console.log('start: \n',result);context.fn.add();result=awaitgetMeasureMemory();console.log('after add once: \n',result);context.fn.remove();result=awaitgetMeasureMemory();console.log('after remove all: \n',result);operateStore.add();result=awaitgetMeasureMemory();console.log('after add global context: \n',result);}measureMemoryTest();
start:{total:{jsMemoryEstimate: 2584500, jsMemoryRange: [ 2584500, 2919297 ] }, current:{jsMemoryEstimate: 2447444, jsMemoryRange: [ 2447444, 2782241 ] }, other: [{jsMemoryEstimate: 137056, jsMemoryRange: [ 137056, 471853 ] } ] } after add once:{total:{jsMemoryEstimate: 3162108, jsMemoryRange: [ 3162108, 3753265 ] }, current:{jsMemoryEstimate: 3024820, jsMemoryRange: [ 3024820, 3615977 ] }, other: [{jsMemoryEstimate: 137288, jsMemoryRange: [ 137288, 728445 ] } ] } after remove all:{total:{jsMemoryEstimate: 2619380, jsMemoryRange: [ 2619380, 3210505 ] }, current:{jsMemoryEstimate: 2481652, jsMemoryRange: [ 2481652, 3072777 ] }, other: [{jsMemoryEstimate: 137728, jsMemoryRange: [ 137728, 728853 ] } ] } after add global context:{total:{jsMemoryEstimate: 3090828, jsMemoryRange: [ 3090828, 3680865 ] }, current:{jsMemoryEstimate: 2953060, jsMemoryRange: [ 2953060, 3543097 ] }, other: [{jsMemoryEstimate: 137768, jsMemoryRange: [ 137768, 727805 ] } ] } 

This seems to be about right (according to the ranges, instead of the estimates which are pretty rough since it's measured after marking, and before sweeping)

@targostargos added dont-land-on-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 25, 2020
Sign up for freeto 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++.lib / srcIssues and PRs related to general changes in the lib or src directory.semver-minorPRs that contain new features and should be released in the next minor version.vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@joyeecheung@devsnek@SimonSchick@gengjiawen@limerickgds@bnoordhuis@jasnell@addaleax@cjihrig@lundibundi@targos@nodejs-github-bot