From 5f2c4ce74f33cf7da5628f6a28c2c06841e16c90 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 22 Apr 2020 10:28:20 +0800 Subject: [PATCH] vm: fix vm.measureMemory() and introduce execution option https://github.com/nodejs/node-v8/pull/147 broke the `vm.measureMemory()` API. It only created a `MeasureMemoryDelegate` and without actually calling `v8::Isolate::MeasureMemory()` so the returned promise will never resolve. This was not caught by the tests because the promise resolvers were not wrapped with `common.mustCall()`. This patch migrates the API properly and also introduce the newly added execution option to the API. It also removes support for specifying contexts to measure - instead we'll just return the measurements for all contexts in the detailed mode, which is what the `performance.measureMemory()` prototype in V8 currently does. We can consider implementing our own `v8::MeasureMemoryDelegate` to select the target context in `ShouldMeasure()` in the future, but then we'll also need to implement `MeasurementComplete()` to assemble the result. For now it's probably too early to do that. Since this API is still experimental (and guarded with a warning), such breakage should be acceptable. Refs: https://github.com/nodejs/node-v8/pull/147 PR-URL: https://github.com/nodejs/node/pull/32988 Reviewed-By: Ben Noordhuis Reviewed-By: Jiawen Geng Reviewed-By: James M Snell --- doc/api/vm.md | 62 ++++++++++++------ lib/vm.js | 17 +++-- src/node_contextify.cc | 58 +++++++++-------- test/common/measure-memory.js | 57 +++++++++++++++++ test/parallel/test-vm-measure-memory-lazy.js | 37 +++++++++++ .../test-vm-measure-memory-multi-context.js | 28 ++++++++ test/parallel/test-vm-measure-memory.js | 64 +++++-------------- 7 files changed, 226 insertions(+), 97 deletions(-) create mode 100644 test/common/measure-memory.js create mode 100644 test/parallel/test-vm-measure-memory-lazy.js create mode 100644 test/parallel/test-vm-measure-memory-multi-context.js diff --git a/doc/api/vm.md b/doc/api/vm.md index 7957c9bb3e51bd..5bf0c6e62b75d9 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -303,15 +303,21 @@ added: v13.10.0 > Stability: 1 - Experimental -Measure the memory known to V8 and used by the current execution context -or a specified context. +Measure the memory known to V8 and used by all contexts known to the +current V8 isolate, or the main context. * `options` {Object} Optional. - * `mode` {string} Either `'summary'` or `'detailed'`. + * `mode` {string} Either `'summary'` or `'detailed'`. In summary mode, + only the memory measured for the main context will be returned. In + detailed mode, the measure measured for all contexts known to the + current V8 isolate will be returned. **Default:** `'summary'` - * `context` {Object} Optional. A [contextified][] object returned - by `vm.createContext()`. If not specified, measure the memory - usage of the current context where `vm.measureMemory()` is invoked. + * `execution` {string} Either `'default'` or `'eager'`. With default + execution, the promise will not resolve until after the next scheduled + garbage collection starts, which may take a while (or never if the program + exits before the next GC). With eager execution, the GC will be started + right away to measure the memory. + **Default:** `'default'` * Returns: {Promise} If the memory is successfully measured the promise will resolve with an object containing information about the memory usage. @@ -319,28 +325,48 @@ The format of the object that the returned Promise may resolve with is specific to the V8 engine and may change from one version of V8 to the next. The returned result is different from the statistics returned by -`v8.getHeapSpaceStatistics()` in that `vm.measureMemory()` measures -the memory reachable by V8 from a specific context, while -`v8.getHeapSpaceStatistics()` measures the memory used by an instance -of V8 engine, which can switch among multiple contexts that reference -objects in the heap of one engine. +`v8.getHeapSpaceStatistics()` in that `vm.measureMemory()` measure the +memory reachable by each V8 specific contexts in the current instance of +the V8 engine, while the result of `v8.getHeapSpaceStatistics()` measure +the memory occupied by each heap space in the current V8 instance. ```js const vm = require('vm'); -// Measure the memory used by the current context and return the result -// in summary. +// Measure the memory used by the main context. vm.measureMemory({ mode: 'summary' }) - // Is the same as vm.measureMemory() + // This is the same as vm.measureMemory() .then((result) => { // The current format is: - // { total: { jsMemoryEstimate: 2211728, jsMemoryRange: [ 0, 2211728 ] } } + // { + // total: { + // jsMemoryEstimate: 2418479, jsMemoryRange: [ 2418479, 2745799 ] + // } + // } console.log(result); }); -const context = vm.createContext({}); -vm.measureMemory({ mode: 'detailed' }, context) +const context = vm.createContext({ a: 1 }); +vm.measureMemory({ mode: 'detailed', execution: 'eager' }) .then((result) => { - // At the moment the detailed format is the same as the summary one. + // Reference the context here so that it won't be GC'ed + // until the measurement is complete. + console.log(context.a); + // { + // total: { + // jsMemoryEstimate: 2574732, + // jsMemoryRange: [ 2574732, 2904372 ] + // }, + // current: { + // jsMemoryEstimate: 2438996, + // jsMemoryRange: [ 2438996, 2768636 ] + // }, + // other: [ + // { + // jsMemoryEstimate: 135736, + // jsMemoryRange: [ 135736, 465376 ] + // } + // ] + // } console.log(result); }); ``` diff --git a/lib/vm.js b/lib/vm.js index cffca355720dae..fd81e9da8b0515 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -385,20 +385,27 @@ const measureMemoryModes = { detailed: constants.measureMemory.mode.DETAILED, }; +const measureMemoryExecutions = { + default: constants.measureMemory.execution.DEFAULT, + eager: constants.measureMemory.execution.EAGER, +}; + function measureMemory(options = {}) { emitExperimentalWarning('vm.measureMemory'); validateObject(options, 'options'); - const { mode = 'summary', context } = options; + const { mode = 'summary', execution = 'default' } = options; if (mode !== 'summary' && mode !== 'detailed') { throw new ERR_INVALID_ARG_VALUE( 'options.mode', options.mode, 'must be either \'summary\' or \'detailed\''); } - if (context !== undefined && - (typeof context !== 'object' || context === null || !_isContext(context))) { - throw new ERR_INVALID_ARG_TYPE('options.context', 'vm.Context', context); + if (execution !== 'default' && execution !== 'eager') { + throw new ERR_INVALID_ARG_VALUE( + 'options.execution', options.execution, + 'must be either \'default\' or \'eager\''); } - const result = _measureMemory(measureMemoryModes[mode], context); + const result = _measureMemory(measureMemoryModes[mode], + measureMemoryExecutions[execution]); if (result === undefined) { return PromiseReject(new ERR_CONTEXT_NOT_INITIALIZED()); } diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 34dbdf08420329..99adccbcef9d64 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -53,6 +53,7 @@ using v8::Isolate; using v8::Local; using v8::Maybe; using v8::MaybeLocal; +using v8::MeasureMemoryExecution; using v8::MeasureMemoryMode; using v8::Name; using v8::NamedPropertyHandlerConfiguration; @@ -1211,29 +1212,22 @@ static void WatchdogHasPendingSigint(const FunctionCallbackInfo& args) { static void MeasureMemory(const FunctionCallbackInfo& args) { CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsInt32()); int32_t mode = args[0].As()->Value(); + int32_t execution = args[1].As()->Value(); Isolate* isolate = args.GetIsolate(); - Environment* env = Environment::GetCurrent(args); - Local context; - if (args[1]->IsUndefined()) { - context = isolate->GetCurrentContext(); - } else { - CHECK(args[1]->IsObject()); - ContextifyContext* sandbox = - ContextifyContext::ContextFromContextifiedSandbox(env, - args[1].As()); - CHECK_NOT_NULL(sandbox); - context = sandbox->context(); - if (context.IsEmpty()) { // Not yet fully initialized - return; - } - } + + Local current_context = isolate->GetCurrentContext(); Local resolver; - if (!Promise::Resolver::New(context).ToLocal(&resolver)) return; - std::unique_ptr i = + if (!Promise::Resolver::New(current_context).ToLocal(&resolver)) return; + std::unique_ptr delegate = v8::MeasureMemoryDelegate::Default( - isolate, context, resolver, static_cast(mode)); - CHECK_NOT_NULL(i); + isolate, + current_context, + resolver, + static_cast(mode)); + isolate->MeasureMemory(std::move(delegate), + static_cast(execution)); v8::Local promise = resolver->GetPromise(); args.GetReturnValue().Set(promise); @@ -1265,13 +1259,27 @@ void Initialize(Local target, Local constants = Object::New(env->isolate()); Local measure_memory = Object::New(env->isolate()); - Local memory_mode = Object::New(env->isolate()); - MeasureMemoryMode SUMMARY = MeasureMemoryMode::kSummary; - MeasureMemoryMode DETAILED = MeasureMemoryMode::kDetailed; - NODE_DEFINE_CONSTANT(memory_mode, SUMMARY); - NODE_DEFINE_CONSTANT(memory_mode, DETAILED); - READONLY_PROPERTY(measure_memory, "mode", memory_mode); + Local memory_execution = Object::New(env->isolate()); + + { + Local memory_mode = Object::New(env->isolate()); + MeasureMemoryMode SUMMARY = MeasureMemoryMode::kSummary; + MeasureMemoryMode DETAILED = MeasureMemoryMode::kDetailed; + NODE_DEFINE_CONSTANT(memory_mode, SUMMARY); + NODE_DEFINE_CONSTANT(memory_mode, DETAILED); + READONLY_PROPERTY(measure_memory, "mode", memory_mode); + } + + { + MeasureMemoryExecution DEFAULT = MeasureMemoryExecution::kDefault; + MeasureMemoryExecution EAGER = MeasureMemoryExecution::kEager; + NODE_DEFINE_CONSTANT(memory_execution, DEFAULT); + NODE_DEFINE_CONSTANT(memory_execution, EAGER); + READONLY_PROPERTY(measure_memory, "execution", memory_execution); + } + READONLY_PROPERTY(constants, "measureMemory", measure_memory); + target->Set(context, env->constants_string(), constants).Check(); env->SetMethod(target, "measureMemory", MeasureMemory); diff --git a/test/common/measure-memory.js b/test/common/measure-memory.js new file mode 100644 index 00000000000000..67c8fb3b9d2d67 --- /dev/null +++ b/test/common/measure-memory.js @@ -0,0 +1,57 @@ +/* eslint-disable node-core/require-common-first, node-core/required-modules */ +'use strict'; + +const assert = require('assert'); +const common = require('./'); + +// The formats could change when V8 is updated, then the tests should be +// updated accordingly. +function assertResultShape(result) { + assert.strictEqual(typeof result.jsMemoryEstimate, 'number'); + assert.strictEqual(typeof result.jsMemoryRange[0], 'number'); + assert.strictEqual(typeof result.jsMemoryRange[1], 'number'); +} + +function assertSummaryShape(result) { + assert.strictEqual(typeof result, 'object'); + assert.strictEqual(typeof result.total, 'object'); + assertResultShape(result.total); +} + +function assertDetailedShape(result, contexts = 0) { + assert.strictEqual(typeof result, 'object'); + assert.strictEqual(typeof result.total, 'object'); + assert.strictEqual(typeof result.current, 'object'); + assertResultShape(result.total); + assertResultShape(result.current); + if (contexts === 0) { + assert.deepStrictEqual(result.other, []); + } else { + assert.strictEqual(result.other.length, contexts); + for (const item of result.other) { + assertResultShape(item); + } + } +} + +function assertSingleDetailedShape(result) { + assert.strictEqual(typeof result, 'object'); + assert.strictEqual(typeof result.total, 'object'); + assert.strictEqual(typeof result.current, 'object'); + assert.deepStrictEqual(result.other, []); + assertResultShape(result.total); + assertResultShape(result.current); +} + +function expectExperimentalWarning() { + common.expectWarning('ExperimentalWarning', + 'vm.measureMemory is an experimental feature. ' + + 'This feature could change at any time'); +} + +module.exports = { + assertSummaryShape, + assertDetailedShape, + assertSingleDetailedShape, + expectExperimentalWarning +}; diff --git a/test/parallel/test-vm-measure-memory-lazy.js b/test/parallel/test-vm-measure-memory-lazy.js new file mode 100644 index 00000000000000..513cfbc3672451 --- /dev/null +++ b/test/parallel/test-vm-measure-memory-lazy.js @@ -0,0 +1,37 @@ +// Flags: --expose-gc + +'use strict'; +const common = require('../common'); +const { + assertSummaryShape, + expectExperimentalWarning +} = require('../common/measure-memory'); +const vm = require('vm'); + +expectExperimentalWarning(); + +// Test lazy memory measurement - we will need to global.gc() +// or otherwise these may not resolve. +{ + vm.measureMemory() + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} + +{ + vm.measureMemory({}) + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} + +{ + vm.measureMemory({ mode: 'summary' }) + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} + +{ + vm.measureMemory({ mode: 'detailed' }) + .then(common.mustCall(assertSummaryShape)); + global.gc(); +} diff --git a/test/parallel/test-vm-measure-memory-multi-context.js b/test/parallel/test-vm-measure-memory-multi-context.js new file mode 100644 index 00000000000000..3a3065a8edb631 --- /dev/null +++ b/test/parallel/test-vm-measure-memory-multi-context.js @@ -0,0 +1,28 @@ +'use strict'; +const common = require('../common'); +const { + assertDetailedShape, + expectExperimentalWarning +} = require('../common/measure-memory'); +const vm = require('vm'); +const assert = require('assert'); + +expectExperimentalWarning(); +{ + const arr = []; + const count = 10; + for (let i = 0; i < count; ++i) { + const context = vm.createContext({ + test: new Array(100).fill('foo') + }); + arr.push(context); + } + // Check that one more context shows up in the result + vm.measureMemory({ mode: 'detailed', execution: 'eager' }) + .then(common.mustCall((result) => { + // We must hold on to the contexts here so that they + // don't get GC'ed until the measurement is complete + assert.strictEqual(arr.length, count); + assertDetailedShape(result, count); + })); +} diff --git a/test/parallel/test-vm-measure-memory.js b/test/parallel/test-vm-measure-memory.js index 7e620304e0a276..6b18db9be7a714 100644 --- a/test/parallel/test-vm-measure-memory.js +++ b/test/parallel/test-vm-measure-memory.js @@ -1,42 +1,25 @@ 'use strict'; const common = require('../common'); +const { + assertSummaryShape, + assertSingleDetailedShape, + expectExperimentalWarning +} = require('../common/measure-memory'); const assert = require('assert'); const vm = require('vm'); -common.expectWarning('ExperimentalWarning', - 'vm.measureMemory is an experimental feature. ' + - 'This feature could change at any time'); +expectExperimentalWarning(); -// The formats could change when V8 is updated, then the tests should be -// updated accordingly. -function assertSummaryShape(result) { - assert.strictEqual(typeof result, 'object'); - assert.strictEqual(typeof result.total, 'object'); - assert.strictEqual(typeof result.total.jsMemoryEstimate, 'number'); - assert(Array.isArray(result.total.jsMemoryRange)); - assert.strictEqual(typeof result.total.jsMemoryRange[0], 'number'); - assert.strictEqual(typeof result.total.jsMemoryRange[1], 'number'); -} - -function assertDetailedShape(result) { - // For now, the detailed shape is the same as the summary shape. This - // should change in future versions of V8. - return assertSummaryShape(result); -} - -// Test measuring memory of the current context +// Test eager memory measurement { - vm.measureMemory() - .then(assertSummaryShape); + vm.measureMemory({ execution: 'eager' }) + .then(common.mustCall(assertSummaryShape)); - vm.measureMemory({}) - .then(assertSummaryShape); + vm.measureMemory({ mode: 'detailed', execution: 'eager' }) + .then(common.mustCall(assertSingleDetailedShape)); - vm.measureMemory({ mode: 'summary' }) - .then(assertSummaryShape); - - vm.measureMemory({ mode: 'detailed' }) - .then(assertDetailedShape); + vm.measureMemory({ mode: 'summary', execution: 'eager' }) + .then(common.mustCall(assertSummaryShape)); assert.throws(() => vm.measureMemory(null), { code: 'ERR_INVALID_ARG_TYPE' @@ -47,24 +30,7 @@ function assertDetailedShape(result) { assert.throws(() => vm.measureMemory({ mode: 'random' }), { code: 'ERR_INVALID_ARG_VALUE' }); -} - -// Test measuring memory of the sandbox -{ - const context = vm.createContext(); - vm.measureMemory({ context }) - .then(assertSummaryShape); - - vm.measureMemory({ mode: 'summary', context },) - .then(assertSummaryShape); - - vm.measureMemory({ mode: 'detailed', context }) - .then(assertDetailedShape); - - assert.throws(() => vm.measureMemory({ mode: 'summary', context: null }), { - code: 'ERR_INVALID_ARG_TYPE' - }); - assert.throws(() => vm.measureMemory({ mode: 'summary', context: {} }), { - code: 'ERR_INVALID_ARG_TYPE' + assert.throws(() => vm.measureMemory({ execution: 'random' }), { + code: 'ERR_INVALID_ARG_VALUE' }); }