From c633fdb282d6a21df10d4e5f417f75b294963e77 Mon Sep 17 00:00:00 2001
From: Keyhan Vakil <kvakil@sylph.kvakil.me>
Date: Tue, 26 Jul 2022 04:56:28 +0000
Subject: [PATCH] worker: add flag to control old space size

This adds a new flag `--thread-max-old-space-size` (name completely
provisional). This has two advantages over the existing
`--max-old-space-size` flag:

1. It allows setting the old space size for the main thread and using
   `resourceLimits` for worker threads. Currently `resourceLimits` will
   be ignored when `--max-old-space-size` is set (see the attached
   issues).
2. It is implemented using V8's public API, rather than relying on V8's
   internal flags whose stability and functionality are not guaranteed.

The downside is that there are now two flags which (in most cases) do
the same thing, so it may cause some confusion. I also think that we
should deprecate `--max-old-space-size`, since the semantics feel pretty
error-prone, but that's a story for another day.

Refs: https://github.com/nodejs/node/issues/41066
Refs: https://github.com/nodejs/node/issues/43991
Refs: https://github.com/nodejs/node/pull/43992
---
 doc/api/cli.md                                | 22 ++++++++++++
 doc/node.1                                    |  5 +++
 src/api/environment.cc                        |  4 ++-
 src/env.cc                                    |  5 ++-
 src/env.h                                     |  3 +-
 src/node_main_instance.cc                     | 18 ++++++++--
 src/node_options.cc                           |  6 ++++
 src/node_options.h                            |  1 +
 test/common/allocate-and-check-limits.js      | 34 ++++++++++++++++++
 test/fixtures/thread-max-old-space-size.js    |  3 ++
 .../test-thread-max-old-space-size.js         | 22 ++++++++++++
 .../test-worker-max-old-space-size.js         | 35 +++++++++++++++++++
 test/parallel/test-worker-resource-limits.js  | 29 ++-------------
 13 files changed, 153 insertions(+), 34 deletions(-)
 create mode 100644 test/common/allocate-and-check-limits.js
 create mode 100644 test/fixtures/thread-max-old-space-size.js
 create mode 100644 test/parallel/test-thread-max-old-space-size.js
 create mode 100644 test/parallel/test-worker-max-old-space-size.js

diff --git a/doc/api/cli.md b/doc/api/cli.md
index f0a2e70b8d7b666..ee58f774b7ae6c1 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -1125,6 +1125,20 @@ added: v18.0.0
 Configures the test runner to only execute top level tests that have the `only`
 option set.
 
+### `--thread-max-old-space-size`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+Sets the max memory size of V8's old memory section for the main thread (in
+megabytes). As memory consumption approaches the limit, V8 will spend more time
+on garbage collection in an effort to free unused memory.
+
+Unlike [`--max-old-space-size`][], this option doesn't affect any additional
+[worker threads][]. To configure the old space size for worker threads, pass in
+an appropriate [`resourceLimits`][] to their constructor.
+
 ### `--throw-deprecation`
 
 <!-- YAML
@@ -1719,6 +1733,7 @@ Node.js options that are allowed are:
 * `--secure-heap-min`
 * `--secure-heap`
 * `--test-only`
+* `--thread-max-old-space-size`
 * `--throw-deprecation`
 * `--title`
 * `--tls-cipher-list`
@@ -2051,6 +2066,9 @@ Sets the max memory size of V8's old memory section. As memory
 consumption approaches the limit, V8 will spend more time on
 garbage collection in an effort to free unused memory.
 
+Unlike [`--thread-max-old-space-size`][], this sets the max old space size of
+all [worker threads][].
+
 On a machine with 2 GiB of memory, consider setting this to
 1536 (1.5 GiB) to leave some memory for other uses and avoid swapping.
 
@@ -2103,8 +2121,10 @@ done
 [`--diagnostic-dir`]: #--diagnostic-dirdirectory
 [`--experimental-wasm-modules`]: #--experimental-wasm-modules
 [`--heap-prof-dir`]: #--heap-prof-dir
+[`--max-old-space-size`]: #--max-old-space-sizesize-in-megabytes
 [`--openssl-config`]: #--openssl-configfile
 [`--redirect-warnings`]: #--redirect-warningsfile
+[`--thread-max-old-space-size`]: #--thread-max-old-space-size
 [`Atomics.wait()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/wait
 [`Buffer`]: buffer.md#class-buffer
 [`CRYPTO_secure_malloc_init`]: https://www.openssl.org/docs/man1.1.0/man3/CRYPTO_secure_malloc_init.html
@@ -2117,6 +2137,7 @@ done
 [`dnsPromises.lookup()`]: dns.md#dnspromiseslookuphostname-options
 [`import` specifier]: esm.md#import-specifiers
 [`process.setUncaughtExceptionCaptureCallback()`]: process.md#processsetuncaughtexceptioncapturecallbackfn
+[`resourceLimits`]: worker_threads.md#new-workerfilename-options
 [`tls.DEFAULT_MAX_VERSION`]: tls.md#tlsdefault_max_version
 [`tls.DEFAULT_MIN_VERSION`]: tls.md#tlsdefault_min_version
 [`unhandledRejection`]: process.md#event-unhandledrejection
@@ -2136,3 +2157,4 @@ done
 [semi-space]: https://www.memorymanagement.org/glossary/s.html#semi.space
 [timezone IDs]: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones
 [ways that `TZ` is handled in other environments]: https://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html
+[worker threads]: worker_threads.md
diff --git a/doc/node.1 b/doc/node.1
index 141519b74b5c6a1..75065013edbc567 100644
--- a/doc/node.1
+++ b/doc/node.1
@@ -394,6 +394,11 @@ Starts the Node.js command line test runner.
 Configures the test runner to only execute top level tests that have the `only`
 option set.
 .
+.It Fl -thread-max-old-space-size
+Sets the max memory size of V8's old memory section for the main thread (in
+megabytes). As memory consumption approaches the limit, V8 will spend more time
+on garbage collection in an effort to free unused memory.
+.
 .It Fl -throw-deprecation
 Throw errors for deprecations.
 .
diff --git a/src/api/environment.cc b/src/api/environment.cc
index 3257f160bdf9540..5a5edda5aa056b0 100644
--- a/src/api/environment.cc
+++ b/src/api/environment.cc
@@ -330,7 +330,9 @@ IsolateData* CreateIsolateData(Isolate* isolate,
                                uv_loop_t* loop,
                                MultiIsolatePlatform* platform,
                                ArrayBufferAllocator* allocator) {
-  return new IsolateData(isolate, loop, platform, allocator);
+  auto options = std::make_shared<PerIsolateOptions>(
+      *(per_process::cli_options->per_isolate));
+  return new IsolateData(isolate, loop, std::move(options), platform, allocator);
 }
 
 void FreeIsolateData(IsolateData* isolate_data) {
diff --git a/src/env.cc b/src/env.cc
index e775474306b7461..00a17ae126f2b74 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -444,17 +444,16 @@ void IsolateData::CreateProperties() {
 
 IsolateData::IsolateData(Isolate* isolate,
                          uv_loop_t* event_loop,
+                         std::shared_ptr<PerIsolateOptions> options,
                          MultiIsolatePlatform* platform,
                          ArrayBufferAllocator* node_allocator,
                          const IsolateDataSerializeInfo* isolate_data_info)
     : isolate_(isolate),
       event_loop_(event_loop),
+      options_(options),
       node_allocator_(node_allocator == nullptr ? nullptr
                                                 : node_allocator->GetImpl()),
       platform_(platform) {
-  options_.reset(
-      new PerIsolateOptions(*(per_process::cli_options->per_isolate)));
-
   if (isolate_data_info == nullptr) {
     CreateProperties();
   } else {
diff --git a/src/env.h b/src/env.h
index 047dfd2870b89a5..f8d09a1bf4ecc2b 100644
--- a/src/env.h
+++ b/src/env.h
@@ -594,6 +594,7 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
  public:
   IsolateData(v8::Isolate* isolate,
               uv_loop_t* event_loop,
+              std::shared_ptr<PerIsolateOptions> options,
               MultiIsolatePlatform* platform = nullptr,
               ArrayBufferAllocator* node_allocator = nullptr,
               const IsolateDataSerializeInfo* isolate_data_info = nullptr);
@@ -668,9 +669,9 @@ class NODE_EXTERN_PRIVATE IsolateData : public MemoryRetainer {
 
   v8::Isolate* const isolate_;
   uv_loop_t* const event_loop_;
+  std::shared_ptr<PerIsolateOptions> options_;
   NodeArrayBufferAllocator* const node_allocator_;
   MultiIsolatePlatform* platform_;
-  std::shared_ptr<PerIsolateOptions> options_;
   worker::Worker* worker_context_ = nullptr;
 };
 
diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
index 1287e795de5bbcb..6d720f78b12d940 100644
--- a/src/node_main_instance.cc
+++ b/src/node_main_instance.cc
@@ -40,8 +40,10 @@ NodeMainInstance::NodeMainInstance(Isolate* isolate,
       platform_(platform),
       isolate_data_(nullptr),
       snapshot_data_(nullptr) {
-  isolate_data_ =
-      std::make_unique<IsolateData>(isolate_, event_loop, platform, nullptr);
+  auto options = std::make_shared<PerIsolateOptions>(
+      *(per_process::cli_options->per_isolate));
+  isolate_data_ = std::make_unique<IsolateData>(
+      isolate_, event_loop, std::move(options), platform, nullptr);
 
   SetIsolateMiscHandlers(isolate_, {});
 }
@@ -77,16 +79,28 @@ NodeMainInstance::NodeMainInstance(const SnapshotData* snapshot_data,
 
   isolate_ = Isolate::Allocate();
   CHECK_NOT_NULL(isolate_);
+
+  auto options = std::make_shared<PerIsolateOptions>(
+      *(per_process::cli_options->per_isolate));
+
   // Register the isolate on the platform before the isolate gets initialized,
   // so that the isolate can access the platform during initialization.
   platform->RegisterIsolate(isolate_, event_loop);
   SetIsolateCreateParamsForNode(isolate_params_.get());
+
+  size_t thread_max_old_space_size = options->thread_max_old_space_size;
+  if (thread_max_old_space_size != 0) {
+    isolate_params_->constraints.set_max_old_generation_size_in_bytes(
+        thread_max_old_space_size * 1024 * 1024);
+  }
+
   Isolate::Initialize(isolate_, *isolate_params_);
 
   // If the indexes are not nullptr, we are not deserializing
   isolate_data_ = std::make_unique<IsolateData>(
       isolate_,
       event_loop,
+      std::move(options),
       platform,
       array_buffer_allocator_.get(),
       snapshot_data == nullptr ? nullptr : &(snapshot_data->isolate_data_info));
diff --git a/src/node_options.cc b/src/node_options.cc
index 7a41f9bc3b87f69..b23a84a5d50b9ec 100644
--- a/src/node_options.cc
+++ b/src/node_options.cc
@@ -661,6 +661,12 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
             &PerIsolateOptions::track_heap_objects,
             kAllowedInEnvironment);
 
+  AddOption(
+      "--thread-max-old-space-size",
+      "set the maximum old space heap size (in megabytes) for this isolate",
+      &PerIsolateOptions::thread_max_old_space_size,
+      kAllowedInEnvironment);
+
   // Explicitly add some V8 flags to mark them as allowed in NODE_OPTIONS.
   AddOption("--abort-on-uncaught-exception",
             "aborting instead of exiting causes a core file to be generated "
diff --git a/src/node_options.h b/src/node_options.h
index 827c6298f5ea28b..3365810874964a4 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -208,6 +208,7 @@ class PerIsolateOptions : public Options {
   bool report_uncaught_exception = false;
   bool report_on_signal = false;
   bool experimental_shadow_realm = false;
+  size_t thread_max_old_space_size = 0;
   std::string report_signal = "SIGUSR2";
   inline EnvironmentOptions* get_per_env_options();
   void CheckOptions(std::vector<std::string>* errors) override;
diff --git a/test/common/allocate-and-check-limits.js b/test/common/allocate-and-check-limits.js
new file mode 100644
index 000000000000000..939fbe9d0922c8e
--- /dev/null
+++ b/test/common/allocate-and-check-limits.js
@@ -0,0 +1,34 @@
+'use strict';
+const assert = require('assert');
+const v8 = require('v8');
+
+function allocateUntilCrash(resourceLimits) {
+  const array = [];
+  while (true) {
+    const usedMB = v8.getHeapStatistics().used_heap_size / 1024 / 1024;
+    const maxReservedSize = resourceLimits.maxOldGenerationSizeMb +
+                            resourceLimits.maxYoungGenerationSizeMb;
+    assert(usedMB < maxReservedSize);
+
+    let seenSpaces = 0;
+    for (const { space_name, space_size } of v8.getHeapSpaceStatistics()) {
+      if (space_name === 'new_space') {
+        seenSpaces++;
+        assert(
+          space_size / 1024 / 1024 < resourceLimits.maxYoungGenerationSizeMb * 2);
+      } else if (space_name === 'old_space') {
+        seenSpaces++;
+        assert(space_size / 1024 / 1024 < resourceLimits.maxOldGenerationSizeMb);
+      } else if (space_name === 'code_space') {
+        seenSpaces++;
+        assert(space_size / 1024 / 1024 < resourceLimits.codeRangeSizeMb);
+      }
+    }
+    assert.strictEqual(seenSpaces, 3);
+
+    for (let i = 0; i < 100; i++)
+      array.push([array]);
+  }
+}
+
+module.exports = { allocateUntilCrash };
diff --git a/test/fixtures/thread-max-old-space-size.js b/test/fixtures/thread-max-old-space-size.js
new file mode 100644
index 000000000000000..3e6f9336f358442
--- /dev/null
+++ b/test/fixtures/thread-max-old-space-size.js
@@ -0,0 +1,3 @@
+const { allocateUntilCrash } = require('../common/allocate-and-check-limits');
+const resourceLimits = JSON.parse(process.argv[2]);
+allocateUntilCrash(resourceLimits);
diff --git a/test/parallel/test-thread-max-old-space-size.js b/test/parallel/test-thread-max-old-space-size.js
new file mode 100644
index 000000000000000..dd2e6fd1189dfce
--- /dev/null
+++ b/test/parallel/test-thread-max-old-space-size.js
@@ -0,0 +1,22 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const fixture = fixtures.path('thread-max-old-space-size.js');
+const { spawnSync } = require('child_process');
+const resourceLimits = {
+  maxOldGenerationSizeMb: 16,
+  maxYoungGenerationSizeMb: 4,
+  // Set codeRangeSizeMb really high to effectively ignore it.
+  codeRangeSizeMb: 999999,
+  stackSizeMb: 4,
+};
+const res = spawnSync(process.execPath, [
+    `--stack-size=${1024 * resourceLimits.stackSizeMb}`,
+    `--thread-max-old-space-size=${resourceLimits.maxOldGenerationSizeMb}`,
+    `--max-semi-space-size=${resourceLimits.maxYoungGenerationSizeMb / 2}`,
+    fixture,
+    JSON.stringify(resourceLimits)
+]);
+assert(res.stderr.toString('utf8').includes('Allocation failed - JavaScript heap out of memory'));
diff --git a/test/parallel/test-worker-max-old-space-size.js b/test/parallel/test-worker-max-old-space-size.js
new file mode 100644
index 000000000000000..35f61ce8d5f38f2
--- /dev/null
+++ b/test/parallel/test-worker-max-old-space-size.js
@@ -0,0 +1,35 @@
+// Flags: --thread-max-old-space-size=1024
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { Worker, resourceLimits } = require('worker_threads');
+const { allocateUntilCrash } = require('../common/allocate-and-check-limits');
+
+const testResourceLimits = {
+  maxOldGenerationSizeMb: 16,
+  maxYoungGenerationSizeMb: 4,
+  codeRangeSizeMb: 16,
+  stackSizeMb: 1,
+};
+
+// Do not use isMainThread so that this test itself can be run inside a Worker.
+if (!process.env.HAS_STARTED_WORKER) {
+  process.env.HAS_STARTED_WORKER = 1;
+  const w = new Worker(__filename, { resourceLimits: testResourceLimits });
+  assert.deepStrictEqual(w.resourceLimits, testResourceLimits);
+  w.on('exit', common.mustCall((code) => {
+    assert.strictEqual(code, 1);
+    assert.deepStrictEqual(w.resourceLimits, {});
+  }));
+  w.on('error', common.expectsError({
+    code: 'ERR_WORKER_OUT_OF_MEMORY',
+    message: 'Worker terminated due to reaching memory limit: ' +
+    'JS heap out of memory'
+  }));
+  return;
+}
+
+assert.deepStrictEqual(resourceLimits, testResourceLimits);
+// resourceLimits should be used; --thread-max-old-space-size should only
+// affect the main thread.
+allocateUntilCrash(resourceLimits);
diff --git a/test/parallel/test-worker-resource-limits.js b/test/parallel/test-worker-resource-limits.js
index f79c31b2a18793e..2f5a2be1896723d 100644
--- a/test/parallel/test-worker-resource-limits.js
+++ b/test/parallel/test-worker-resource-limits.js
@@ -1,8 +1,8 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const v8 = require('v8');
 const { Worker, resourceLimits, isMainThread } = require('worker_threads');
+const { allocateUntilCrash } = require('../common/allocate-and-check-limits');
 
 if (isMainThread) {
   assert.deepStrictEqual(resourceLimits, {});
@@ -33,29 +33,4 @@ if (!process.env.HAS_STARTED_WORKER) {
 }
 
 assert.deepStrictEqual(resourceLimits, testResourceLimits);
-const array = [];
-while (true) {
-  const usedMB = v8.getHeapStatistics().used_heap_size / 1024 / 1024;
-  const maxReservedSize = resourceLimits.maxOldGenerationSizeMb +
-                          resourceLimits.maxYoungGenerationSizeMb;
-  assert(usedMB < maxReservedSize);
-
-  let seenSpaces = 0;
-  for (const { space_name, space_size } of v8.getHeapSpaceStatistics()) {
-    if (space_name === 'new_space') {
-      seenSpaces++;
-      assert(
-        space_size / 1024 / 1024 < resourceLimits.maxYoungGenerationSizeMb * 2);
-    } else if (space_name === 'old_space') {
-      seenSpaces++;
-      assert(space_size / 1024 / 1024 < resourceLimits.maxOldGenerationSizeMb);
-    } else if (space_name === 'code_space') {
-      seenSpaces++;
-      assert(space_size / 1024 / 1024 < resourceLimits.codeRangeSizeMb);
-    }
-  }
-  assert.strictEqual(seenSpaces, 3);
-
-  for (let i = 0; i < 100; i++)
-    array.push([array]);
-}
+allocateUntilCrash(resourceLimits);