chore: replace vm.Script with vm.compileFunction
#12205
Conversation
|
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 cla@fb.com. Thanks! |
rthreei
force-pushed
the
fix/memory-leak
branch
from
9869863
to
8d3e19c
Compare
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.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
rthreei
force-pushed
the
fix/memory-leak
branch
4 times, most recently
from
2d84b73
to
951de91
Compare
Memory leak is alleviated with `vm.compileFunction`. However, implementing `importModuleDynamically` causes memory to leak again, so that has been skipped. Fixes jestjs#11956
rthreei
force-pushed
the
fix/memory-leak
branch
from
951de91
to
790a026
Compare
|
What needs to happen to get this over the finish line? |
I guess @SimenB has some doubts about this? |
|
Main reason is that this will make it much worse for node 12 and 14, and it's really a bug in node |
|
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! |
| @@ -72,7 +72,7 @@ test('import cjs', async () => { | |||
| expect(half(4)).toBe(2); | |||
| }); | |||
|
|
|||
| 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
|
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 |
|
@phawxby nothing new since the last activity, feel free to fix: #12205 (comment) |
As you noted in the thread above, the |
|
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. |
|
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 = class CompileFunctionRuntime extends JestRuntime {
_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). |
|
In NodeJS v18.17.1 the heap increases from 118Mb to 5815MB and total runtime runInBand + logHeapUsage is 187sec :/ |
|
Node landed a fix for |
|
NodeJs v20.8.0 running: Bumps heap size from 125Mb to 5601 MB heap size (execution time 166 sec ) Removing runtime: '@side/jest-runtime', -setting: Bumps heap size to 5778 MB ( execution time 178sec ) removing the runInBand and logHeapUsage However not having --runInBand the memory usage is even more expensive and in dev ops cheap machines the computer is starwing... |
|
I'm starting to feel that there is some other memory leak issue as well, it feels like Jest keeps the test data in memory until the whole run is finished |
|
@Havunen this fix (nodejs/node#49950 (comment)) isn't in node 20.8.0. next week 20.9.0 should get released. maybe this fixes some of the memory problems |
|
@ximex is the fix available in any nightly / preview release? I would like to test if it solves this issue |
|
I'm not the expert for this. but i think so if you try the todays nightly version |
|
I tried it but it does not seem to change anything? |
|
If you use ESM the fix in Node won't help you, unfortunately - that is for |
|
For ESM you probably need to wait for nodejs/node#33439 and/or an upstream v8 proper fix for the code caching |
|
NodeJs 20.8.0 CommonJs - no changes -- 86sec 10GB RAM NodeJs 20.8.0 CommonJs - +patch for runtimeSupportsVmModules to be undefined - 86sec 10GB RAM It seems its not yet fixed |
|
The fix is not released yet, you'll need to install the nightly. But I'm not sure if the commit made it in time for the nightly or if you'll have to wait until tomorrow |
Ok, I will try again tomorrow |
Because node js is not GCing dynamic modules, would it make sense to cache every created dynamic module to ensure only one gets created? |
|
That would leak module state between tests |
|
The current situation in Node.js is:
|
|
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 |
|
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 |
|
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 |
|
I created an issue to nodejs repo: nodejs/node#50113 |
|
Fantastic, thanks @Havunen! I also noticed I could get a pure nodejs thing to go OOM only when passing |
|
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. |
|
can we enable this by an option? a feature flag ? to be an opt in ? |
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):