chore: replace vm.Script with vm.compileFunction#12205
Uh oh!
There was an error while loading. Please reload this page.
Conversation
facebook-github-bot commented Dec 31, 2021
Hi @rthreei! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
rthreeiforce-pushed the fix/memory-leak branch from 9869863 to 8d3e19cCompare
Codecov Report
@@ Coverage Diff @@## main #12205 +/- ## ========================================== + Coverage 67.51% 67.54% +0.02% ========================================== Files 328 328 Lines 17246 17228 -18 Branches 5071 5067 -4 ========================================== - Hits 11644 11637 -7 + Misses 5569 5556 -13 - Partials 33 35 +2
Continue to review full report at Codecov.
|
facebook-github-bot commented Dec 31, 2021
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
rthreeiforce-pushed the fix/memory-leak branch 4 times, most recently from 2d84b73 to 951de91Compare
Memory leak is alleviated with `vm.compileFunction`. However, implementing `importModuleDynamically` causes memory to leak again, so that has been skipped. Fixesjestjs#11956
vanstinator commented Feb 24, 2022
What needs to happen to get this over the finish line? |
Smrtnyk commented Feb 26, 2022
I guess @SimenB has some doubts about this? |
SimenB commented Feb 26, 2022
Main reason is that this will make it much worse for node 12 and 14, and it's really a bug in node |
vanstinator commented Feb 26, 2022
Can we make it an opt-in change with a cli flag? I'm happy to do the work and update the tests. With node 14 going EOL this fall more people are going to be looking to move to 16 (including us at Plex). A cli flag would make the move much less painful. When node fixes their bug we can remove the option from jest again when it makes sense |
I don't think we should add a config flag - if we do this we should just detect the version of node inline so people don't have to change anything. Maybe you could change this to use |
Sure, that approach works too. I'll open a fresh PR based on this one when I'm ready. Thanks for the quick brainstorm! |
| }); | ||
| test('import esm from cjs',async()=>{ | ||
| test.skip('import esm from cjs',async()=>{ |
There was a problem hiding this comment.
note that this makes the change unacceptable - this needs to work. Since nodejs/node#31860 has been fixed, I assume you can just add the missing importModuleDynamically to the compileFunction call
There was a problem hiding this comment.
fyi - when I had left importModuleDynamically implemented, the memory leak persisted. Another mystery...
There was a problem hiding this comment.
interesting! that might be a separate bug in node 😅
There was a problem hiding this comment.
I've replicated this myself locally. I'll see if I can figure out why.
There was a problem hiding this comment.
I've replicated this myself locally. I'll see if I can figure out why.
Any luck @vanstinator ?
There was a problem hiding this comment.
If this is the only problem holding y'all back to merge this PR, I'd love to hear an update from you @vanstinator
There was a problem hiding this comment.
@rene-leanix Furthermore, it appears progression on this issue may be dependent upon this V8 PR via this comment
phawxby commented Apr 14, 2022
Is there anything I can do to get this PR moving along again? We generally try to avoid falling too far behind even minor versions |
SimenB commented Apr 17, 2022
@phawxby nothing new since the last activity, feel free to fix: #12205 (comment) |
phawxby commented Apr 19, 2022
As you noted in the thread above, the |
rthreei commented Apr 19, 2022
|
phawxby commented Apr 19, 2022
Ah, I misunderstood your comment as the issue only occurs when importing esm from cjs, not in all scenarios. I'm going to have a dig around this afternoon and see if I can find anything but I'm not holding out much hope. |
phawxby commented Apr 19, 2022
A bit more digging and I've found this, which suggests this isn't going to be especially easy to fix. Which then ties in with this. |
Instead of using a patch, one thing that can be done is to extend the default While const{default: JestRuntime}=require('jest-runtime');module.exports=classCompileFunctionRuntimeextendsJestRuntime{_execModule(){// do something}};This module could then be published as a workaround. Somewhat brittle as |
@a88zach has published https://github.com/reside-eng/jest-runtime which does what @SimenB suggested in the last comment. This unblocked us from upgrading to Node > 16.10. See #11956 (comment). |
Havunen commented Oct 5, 2023
Ok, I will try again tomorrow |
Havunen commented Oct 5, 2023
Because node js is not GCing dynamic modules, would it make sense to cache every created dynamic module to ensure only one gets created? |
SimenB commented Oct 5, 2023
That would leak module state between tests |
The current situation in Node.js is:
|
SimenB commented Oct 5, 2023
Oooh, number 3 is perfect! We'll have to try to come up with a minimal reproduction after these fixes are out to avoid conflating issues 👍 |
edit: nvm its not related to that. It seems to be related to a json file and we have a lot of those in our app |
Havunen commented Oct 5, 2023
I created a new issue here: #14605 |
FYI you can use --heapsnapshot-near-heap-limit=3 (or a higher value) to generate a bunch of heap snapshots and figure out what is leaking - at least what is leaking on the JS side. --heap-prof might be useful too if heap snapshots take too long to generate. (On how to use the heap snapshots, check out https://developer.chrome.com/docs/devtools/memory-problems/heap-snapshots/ though the UI screenshots are a bit outdated but the general idea stays the same). There are probably also some more up-to-date tutorials on how to use the Chrome DevTools to visualize these things if you search for them. |
The reproduction linked above with ESM doesn't leak for me if I remove the Maybe Not sure as we're of course back to the same upstream v8 issue the moment it's enabled. But this would allow lots of people who don't need Lots of |
Havunen commented Oct 9, 2023
I have reproduced the issue using nodejs and javascript only, and also found a work around. Having a shared context between the modules seems to leak the memory. However setting the shared context null manually after function execution it fixes the memory leak. Setting the variable null in JS is non-sense (ref: https://github.com/Havunen/nodejs-memory-leak/blob/main/test.js#L45-L46 ) because it goes out of scope and should be GC'd but it does not seem to happen. So its definetly a nodejs / v8 bug |
Havunen commented Oct 10, 2023
I created an issue to nodejs repo: nodejs/node#50113 |
SimenB commented Oct 10, 2023
Fantastic, thanks @Havunen! I also noticed I could get a pure nodejs thing to go OOM only when passing |
AlecksJohannes commented Jan 5, 2024
Hey there amazing folks ! I know this PR is here been quite a long time, I just gave this PR a tried on Node 20.10, unfortunately it does not fix the issue. The heap memory usage still keeps piling up for each test files. |
sibelius commented Oct 1, 2024
can we enable this by an option? a feature flag ? to be an opt in ? |
ahnpnl commented Jan 16, 2025
Would be nice if we can ship this to v30 👍 |
Yeah, we can probably do that since the leak in |
sibelius commented Jan 16, 2025
Release behind a feature flag? |
SimenB commented Jan 16, 2025
Let's see what CI says: #15461 |
joyeecheung commented Jan 16, 2025
The V8 patch https://chromium-review.googlesource.com/c/v8/v8/+/4962094 is not on v20 by the way, though I happened to be looking into back porting it to make back porting compile cache easier (which would make back porting require(esm) easier). |
SimenB commented Jan 16, 2025
@joyeecheung oh, interesting! Do you think we should hold off on migrating off of Even if we do, we'd still not have it for Node 18, so if it's impactful, we might wanna hold off regardless of 20.x backport 🤔 |
ahnpnl commented Jan 16, 2025
I think this can be also a solution to move forward @SimenB |
csvan commented May 27, 2025
omg omg omg been following this for the last 4 years and so happy it is finally closed <3 Amazing work! |
maschwenk commented May 27, 2025
@csvan 😆 I've kind of lost of the thread of what's going on here. How will this affect those of us on Node 20.x +? Lower memory usage (and/or leaks) on the new Jest 30 beta? |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
20 participants
Summary
Memory leak is alleviated with
vm.compileFunction.Related to #11956
Inspired by earlier work done earlier #10586
Test plan
Change is covered by existing tests. Below is a before and after of heap usage against a trivial reproduction.
Before (against main branch):
After (against this branch):