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 <info@bnoordhuis.nl>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

7 files changed, 226 insertions, 97 deletions

diff --git a/doc/api/vm.md b/doc/api/vm.md
index f45b537368..6411419e54 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 cffca35572..fd81e9da8b 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 34dbdf0842..99adccbcef 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<Value>& args) {
 
 static void MeasureMemory(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsInt32());
+  CHECK(args[1]->IsInt32());
   int32_t mode = args[0].As<v8::Int32>()->Value();
+  int32_t execution = args[1].As<v8::Int32>()->Value();
   Isolate* isolate = args.GetIsolate();
-  Environment* env = Environment::GetCurrent(args);
-  Local<Context> context;
-  if (args[1]->IsUndefined()) {
-    context = isolate->GetCurrentContext();
-  } else {
-    CHECK(args[1]->IsObject());
-    ContextifyContext* sandbox =
-        ContextifyContext::ContextFromContextifiedSandbox(env,
-                                                          args[1].As<Object>());
-    CHECK_NOT_NULL(sandbox);
-    context = sandbox->context();
-    if (context.IsEmpty()) {  // Not yet fully initialized
-      return;
-    }
-  }
+
+  Local<Context> current_context = isolate->GetCurrentContext();
   Local<Promise::Resolver> resolver;
-  if (!Promise::Resolver::New(context).ToLocal(&resolver)) return;
-  std::unique_ptr<v8::MeasureMemoryDelegate> i =
+  if (!Promise::Resolver::New(current_context).ToLocal(&resolver)) return;
+  std::unique_ptr<v8::MeasureMemoryDelegate> delegate =
       v8::MeasureMemoryDelegate::Default(
-          isolate, context, resolver, static_cast<v8::MeasureMemoryMode>(mode));
-  CHECK_NOT_NULL(i);
+          isolate,
+          current_context,
+          resolver,
+          static_cast<v8::MeasureMemoryMode>(mode));
+  isolate->MeasureMemory(std::move(delegate),
+                         static_cast<v8::MeasureMemoryExecution>(execution));
   v8::Local<v8::Promise> promise = resolver->GetPromise();
 
   args.GetReturnValue().Set(promise);
@@ -1265,13 +1259,27 @@ void Initialize(Local<Object> target,
 
   Local<Object> constants = Object::New(env->isolate());
   Local<Object> measure_memory = Object::New(env->isolate());
-  Local<Object> 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<Object> memory_execution = Object::New(env->isolate());
+
+  {
+    Local<Object> 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 0000000000..67c8fb3b9d
--- /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 0000000000..513cfbc367
--- /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 0000000000..3a3065a8ed
--- /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 7e620304e0..6b18db9be7 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'
   });
 }