From 325e5a690a1ebc93edff64437139852e1d18112a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 16 Sep 2023 15:07:30 +0200 Subject: [PATCH 1/4] test: add checkIfCollectable to test/common/gc.js --- test/common/gc.js | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 test/common/gc.js diff --git a/test/common/gc.js b/test/common/gc.js new file mode 100644 index 00000000000000..8de874b1cb8303 --- /dev/null +++ b/test/common/gc.js @@ -0,0 +1,49 @@ +'use strict'; + +// TODO(joyeecheung): merge ongc.js and gcUntil from common/index.js +// into this. + +// This function can be used to check if an object factor leaks or not, +// but it needs to be used with care: +// 1. The test should be set up with an ideally small +// --max-old-space-size or --max-heap-size, which combined with +// the maxCount parameter can reproduce a leak of the objects +// created by fn(). +// 2. This works under the assumption that if *none* of the objects +// created by fn() can be garbage-collected, the test would crash due +// to OOM. +// 3. If *any* of the objects created by fn() can be garbage-collected, +// it is considered leak-free. The FinalizationRegistry is used to +// terminate the test early once we detect any of the object is +// garbage-collected to make the test less prone to false positives. +// This may be especially important for memory management relying on +// emphemeron GC which can be inefficient to deal with extremely fast +// heap growth. +// Note that this can still produce false positives. When the test using +// this function still crashes due to OOM, inspect the heap to confirm +// if a leak is present (e.g. using heap snapshots). +async function checkIfCollectable(fn, maxCount = 4096, logEvery = 128) { + let anyFinalized = false; + let count = 0; + + const f = new FinalizationRegistry(() => { + anyFinalized = true; + }); + + async function createObject() { + const obj = await fn(); + f.register(obj); + if (count % logEvery === 1) { + console.log('Created', count); + } + if (count++ < maxCount && !anyFinalized) { + setImmediate(createObject); + } + } + + createObject(); +} + +module.exports = { + checkIfCollectable, +}; From 6174c98911470f1422c8009b824971ebf5885234 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 16 Sep 2023 15:08:18 +0200 Subject: [PATCH 2/4] test: use checkIfCollectable in vm leak tests Previously we simply create a lot of the target objects and check if the process crash due to OOM. Due to how we use emphemeron GC to handle memory management, which is inefficient but necessary for correctness, the tests can produce false positives as the GC isn't efficient enough to catch up with a very fast heap growth. This patch uses a new checkIfCollectable() utility to terminate the test early once we detect that any of the target object can actually be garbage collected. This should lower the chance of false positives. As a drive-by this also allows us to use setImmediate() to grow the heap even faster to make the tests run faster. --- .../es-module/test-vm-compile-function-leak.js | 13 +++++-------- .../test-vm-contextified-script-leak.js | 11 ++++------- .../test-vm-source-text-module-leak.js | 18 ++++++++---------- .../es-module/test-vm-synthetic-module-leak.js | 11 +++-------- 4 files changed, 20 insertions(+), 33 deletions(-) diff --git a/test/es-module/test-vm-compile-function-leak.js b/test/es-module/test-vm-compile-function-leak.js index ff061cdaec7a01..f9f04588fdc7c3 100644 --- a/test/es-module/test-vm-compile-function-leak.js +++ b/test/es-module/test-vm-compile-function-leak.js @@ -4,16 +4,13 @@ // This tests that vm.compileFunction with dynamic import callback does not leak. // See https://github.com/nodejs/node/issues/44211 require('../common'); +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -function main() { - // Try to reach the maximum old space size. - vm.compileFunction(`"${Math.random().toString().repeat(512)}"`, [], { +async function createCompiledFunction() { + return vm.compileFunction(`"${Math.random().toString().repeat(512)}"`, [], { async importModuleDynamically() {}, }); - if (count++ < 2048) { - setTimeout(main, 1); - } } -main(); + +checkIfCollectable(createCompiledFunction, 2048); diff --git a/test/es-module/test-vm-contextified-script-leak.js b/test/es-module/test-vm-contextified-script-leak.js index 7498b46ab80cfa..60212dd4bbbf68 100644 --- a/test/es-module/test-vm-contextified-script-leak.js +++ b/test/es-module/test-vm-contextified-script-leak.js @@ -4,16 +4,13 @@ // This tests that vm.Script with dynamic import callback does not leak. // See: https://github.com/nodejs/node/issues/33439 require('../common'); +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -function main() { +async function createContextifyScript() { // Try to reach the maximum old space size. - new vm.Script(`"${Math.random().toString().repeat(512)}";`, { + return new vm.Script(`"${Math.random().toString().repeat(512)}";`, { async importModuleDynamically() {}, }); - if (count++ < 2 * 1024) { - setTimeout(main, 1); - } } -main(); +checkIfCollectable(createContextifyScript, 2048); diff --git a/test/es-module/test-vm-source-text-module-leak.js b/test/es-module/test-vm-source-text-module-leak.js index bf7f70c670e34c..71d6f61a4ad153 100644 --- a/test/es-module/test-vm-source-text-module-leak.js +++ b/test/es-module/test-vm-source-text-module-leak.js @@ -4,20 +4,18 @@ // This tests that vm.SourceTextModule() does not leak. // See: https://github.com/nodejs/node/issues/33439 require('../common'); - +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -async function createModule() { + +async function createSourceTextModule() { // Try to reach the maximum old space size. const m = new vm.SourceTextModule(` - const bar = new Array(512).fill("----"); - export { bar }; -`); + const bar = new Array(512).fill("----"); + export { bar }; + `); await m.link(() => {}); await m.evaluate(); - if (count++ < 4096) { - setTimeout(createModule, 1); - } return m; } -createModule(); + +checkIfCollectable(createSourceTextModule, 4096); diff --git a/test/es-module/test-vm-synthetic-module-leak.js b/test/es-module/test-vm-synthetic-module-leak.js index 9de02cb22f1128..bc0e4689535327 100644 --- a/test/es-module/test-vm-synthetic-module-leak.js +++ b/test/es-module/test-vm-synthetic-module-leak.js @@ -4,20 +4,15 @@ // This tests that vm.SyntheticModule does not leak. // See https://github.com/nodejs/node/issues/44211 require('../common'); +const { checkIfCollectable } = require('../common/gc'); const vm = require('vm'); -let count = 0; -async function createModule() { - // Try to reach the maximum old space size. +async function createSyntheticModule() { const m = new vm.SyntheticModule(['bar'], () => { m.setExport('bar', new Array(512).fill('----')); }); await m.link(() => {}); await m.evaluate(); - if (count++ < 4 * 1024) { - setTimeout(createModule, 1); - } return m; } - -createModule(); +checkIfCollectable(createSyntheticModule, 4096); From 63a6ab6d6edb1c7ef69fab3843e871c71aad67df Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 17 Sep 2023 10:59:28 +0200 Subject: [PATCH 3/4] fixup! test: add checkIfCollectable to test/common/gc.js --- test/common/gc.js | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/test/common/gc.js b/test/common/gc.js index 8de874b1cb8303..af637af7bedcd6 100644 --- a/test/common/gc.js +++ b/test/common/gc.js @@ -22,7 +22,14 @@ // Note that this can still produce false positives. When the test using // this function still crashes due to OOM, inspect the heap to confirm // if a leak is present (e.g. using heap snapshots). -async function checkIfCollectable(fn, maxCount = 4096, logEvery = 128) { +// The generateSnapshotAt parameter can be used to specify a count +// interval to create the heap snapshot which may enforce a more thorough GC. +// This can be tried for code paths that require it for the GC to catch up +// with heap growth. However this type of forced GC can be in conflict with +// other logic in V8 such as bytecode aging, and it can slow down the test +// significantly, so it should be used scarcely and only as a last resort. +async function checkIfCollectable( + fn, maxCount = 4096, generateSnapshotAt = Infinity, logEvery = 128) { let anyFinalized = false; let count = 0; @@ -33,11 +40,25 @@ async function checkIfCollectable(fn, maxCount = 4096, logEvery = 128) { async function createObject() { const obj = await fn(); f.register(obj); - if (count % logEvery === 1) { - console.log('Created', count); - } if (count++ < maxCount && !anyFinalized) { - setImmediate(createObject); + setImmediate(createObject, 1); + } + // This can force a more thorough GC, but can slow the test down + // significantly in a big heap. Use it with care. + if (count % generateSnapshotAt === 0) { + // XXX(joyeecheung): This itself can consume a bit of JS heap memory, + // but the other alternative writeHeapSnapshot can run into disk space + // not enough problems in the CI & be slower depending on file system. + // Just do this for now as long as it works and only invent some + // internal voodoo when we absolutely have no other choice. + require('v8').getHeapSnapshot().pause().read(); + console.log(`Generated heap snapshot at ${count}`); + } + if (count % logEvery === 0) { + console.log(`Created ${count} objects`); + } + if (anyFinalized) { + console.log(`Found finalized object at ${count}, stop testing`); } } From 4561ac3cd4011147b631f58c9b5ea1b7aa5417ca Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sun, 17 Sep 2023 10:59:36 +0200 Subject: [PATCH 4/4] fixup! test: use checkIfCollectable in vm leak tests --- test/es-module/test-vm-source-text-module-leak.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-vm-source-text-module-leak.js b/test/es-module/test-vm-source-text-module-leak.js index 71d6f61a4ad153..d05e812ac32c95 100644 --- a/test/es-module/test-vm-source-text-module-leak.js +++ b/test/es-module/test-vm-source-text-module-leak.js @@ -18,4 +18,4 @@ async function createSourceTextModule() { return m; } -checkIfCollectable(createSourceTextModule, 4096); +checkIfCollectable(createSourceTextModule, 4096, 1024);