test: add and use checkIfCollectable in vm leak tests #49671

joyeecheung · 2023-09-16T13:14:42Z

test: add checkIfCollectable to test/common/gc.js

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.

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.

joyeecheung · 2023-09-16T13:16:44Z

I checked locally that without #48510 these tests all fail and with that PR these tests all pass, so at least the leak should not get way with it.

nodejs-github-bot · 2023-09-16T13:32:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53979/

joyeecheung · 2023-09-16T15:05:10Z

So locally they can still crash with #49639 however the crash can go away if we use the hacky heap snapshot trick to enforce a very thorough GC. I will think about how to make it less prone to false positives without, um, having to rely on a heap snapshot to do GC…

joyeecheung · 2023-09-17T11:35:17Z

Added a special option to resort to heap snapshots. I was trying to avoid it if possible but welp, looks like it is necessary for the source text module test at least. Locally that works with the v8 11.8 upgrade.

nodejs-github-bot · 2023-09-17T11:38:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54001/

nodejs-github-bot · 2023-09-18T13:22:38Z

Commit Queue failed

- Loading data for nodejs/node/pull/49671 ✔ Done loading data for nodejs/node/pull/49671 ----------------------------------- PR info ------------------------------------ Title test: add and use checkIfCollectable in vm leak tests (#49671) Author Joyee Cheung (@joyeecheung) Branch joyeecheung:fix-compile-2 -> nodejs:main Labels test, esm, author ready, needs-ci Commits 4 - test: add checkIfCollectable to test/common/gc.js - test: use checkIfCollectable in vm leak tests - fixup! test: add checkIfCollectable to test/common/gc.js - fixup! test: use checkIfCollectable in vm leak tests Committers 1 - Joyee Cheung PR-URL: https://github.com/nodejs/node/pull/49671 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/49671 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Michaël Zasso -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 16 Sep 2023 13:14:42 GMT ✔ Approvals: 4 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/49671#pullrequestreview-1629986325 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/49671#pullrequestreview-1630074148 ✔ - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49671#pullrequestreview-1630084856 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/49671#pullrequestreview-1630584633 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-09-17T11:38:02Z: https://ci.nodejs.org/job/node-test-pull-request/54001/ - Querying data for job/node-test-pull-request/54001/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 49671 From https://github.com/nodejs/node * branch refs/pull/49671/merge -> FETCH_HEAD ✔ Fetched commits as b03a757619db..4561ac3cd401 -------------------------------------------------------------------------------- [main aaecad0b19] test: add checkIfCollectable to test/common/gc.js Author: Joyee Cheung Date: Sat Sep 16 15:07:30 2023 +0200 1 file changed, 49 insertions(+) create mode 100644 test/common/gc.js [main db84c26040] test: use checkIfCollectable in vm leak tests Author: Joyee Cheung Date: Sat Sep 16 15:08:18 2023 +0200 4 files changed, 20 insertions(+), 33 deletions(-) [main 1a7168b5c7] fixup! test: add checkIfCollectable to test/common/gc.js Author: Joyee Cheung Date: Sun Sep 17 10:59:28 2023 +0200 1 file changed, 26 insertions(+), 5 deletions(-) [main 495ed973f5] fixup! test: use checkIfCollectable in vm leak tests Author: Joyee Cheung Date: Sun Sep 17 10:59:36 2023 +0200 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 4 commits in the PR. Attempting autorebase. Rebasing (2/6) Rebasing (3/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: add checkIfCollectable to test/common/gc.js

PR-URL: #49671
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Michaël Zasso targos@protonmail.com

[detached HEAD 5bab3a35c7] test: add checkIfCollectable to test/common/gc.js
Author: Joyee Cheung joyeec9h3@gmail.com
Date: Sat Sep 16 15:07:30 2023 +0200
1 file changed, 70 insertions(+)
create mode 100644 test/common/gc.js
Rebasing (4/6)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
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.

PR-URL: #49671
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Yagiz Nizipli yagiz@nizipli.com
Reviewed-By: Michaël Zasso targos@protonmail.com

[detached HEAD 6d5c3154f1] test: use checkIfCollectable in vm leak tests
Author: Joyee Cheung joyeec9h3@gmail.com
Date: Sat Sep 16 15:08:18 2023 +0200
4 files changed, 20 insertions(+), 33 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/6223134509

PR-URL: #49671 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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. PR-URL: #49671 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>