-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pummel/test-heapdump-shadow-realm.js is flaky due to OOM #49572
Comments
I'm not aware that weak callbacks for ShadowRealm are deferred to prevent it from being collected. Since this is related to the heapdump specifically (there is no reliability report on |
Normally heapdump should not have more JS heap overhead than the size of a semi-space (because of promotion) + a little bit of margin for cached source positions (can't be that much for ShadowRealms that only have one tiny script). It could be though that because the weak callbacks are deferred, they are still recognized as reachable when the heap snapshot is taken and thus still in the heap snapshot which means the heap snapshot can get bloated a lot (and a huge heap snapshot in itself can eat a lot of memory because we serialize it into JS on the same thread for test verification). We can see if the |
Looks like this disappeared from the CI. Closing for now. We can reopen if it reappears. |
I think it's back: https://ci.nodejs.org/job/node-test-pull-request/54396/ |
Oh, my, this failed18 PRs across the last 100 CI runs. nodejs/reliability#681 looks like it started from 09-29 |
I am guessing something landed around that time introduced a memory leak in the realms? |
Actually it seems this time around it's a different bug. It's STATUS_STACK_BUFFER_OVERRUN on Windows (or it could be other assertion failures, https://devblogs.microsoft.com/oldnewthing/20190108-00/?p=100655) |
First STATUS_STACK_BUFFER_OVERRUN failure goes back to https://ci.nodejs.org/job/node-test-pull-request/54325/ which is rebasing onto a4fdb1a |
Stress test on ubuntu1804-64 https://ci.nodejs.org/job/node-stress-single-test/455/ however the failures in the CI mostly come from win2016_vs2017-x64 and the stress tests do not run there.. |
Stress test passed, but it looks like it ran other tests, not |
https://ci.nodejs.org/job/node-stress-single-test/456/parameters/ Another run of stress test on pummel/test-heapdump-shadow-realm. Edit: well, I do remember I checked win2016-vs2017 as one of the run labels, but it turned out that only ubuntu1804-64 was checked. |
No failures with ubuntu1804-64 |
With a tight loop the GC may not have enough time to kick in. Try setImmediate() instead. PR-URL: nodejs#49573 Refs: nodejs#49572 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
ShadowRealm garbage-collection is covered in another test. Reduce the number of repetition in test-heapdump-shadowrealm.js trying to fix the flakiness of the test. PR-URL: nodejs#50104 Refs: nodejs#49572 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
There have been no new failures since December 2023, closing. |
Test
pummel/test-heapdump-shadow-realm.js
Platform
Linux x64
Console output
Build links
nodejs/reliability#660
First CI: https://ci.nodejs.org/job/node-test-pull-request/53749/
Last CI: https://ci.nodejs.org/job/node-test-pull-request/53778/
Additional information
cc @legendecas At first glance I think we could lower the number of shadow realms created in the test. But then the realms are supposed to be GC-able anyway...perhaps we should switch to
setImmediate()
instead to give GC some time to kick in?The text was updated successfully, but these errors were encountered: