Skip to content
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

Fix module registry memory leak. #8282

Merged
merged 2 commits into from Apr 7, 2019

Conversation

Projects
None yet
9 participants
@scotthovestadt
Copy link
Contributor

commented Apr 7, 2019

Summary

This leak is caused by this line:

return moduleRegistry.get(key) || null;

The moduleRegistry is preserved in the closure and never GC'd, along with all modules within the registry, making it a pretty nasty memory leak. I'm fairly certain it's also a bug fix: when you reset the registry, this reference to the registry continued referencing the old version. That doesn't make sense after a reset.

This code is now a map and easy to clear but until very recently it was a normal Object which explains why it was 'cleared' in this way.

Memory Impact

command: yarn jest --logHeapUsage --w=1 packages/

--expose-gc enabled to trigger gc manually after each test for accurate readings on the leak

without circus:
295 MB -> 262 MB (11% savings)

with circus:
848 MB -> 762 MB (10% savings)

Test plan

  • All tests pass.
  • Reason for leak identified.
  • Memory impact quantified.
  • Hoping CI for Circus doesn't OOM now. :)
@scotthovestadt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2019

Screen Shot 2019-04-07 at 1 50 27 AM

🎉🎉🎉

Circus CI no longer fails with an OOM error, which means this has definitely made an impact.

Just need a sanity check on the behavior change and if it makes sense. In the case where we want to preserve the old behavior (I still think it's a bug), it can be done in a way that doesn't create a large memory leak.

@cpojer take a look if you have a moment, maybe you can confirm it's a bug for me?

@codecov-io

This comment has been minimized.

Copy link

commented Apr 7, 2019

Codecov Report

Merging #8282 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8282   +/-   ##
=======================================
  Coverage   62.13%   62.13%           
=======================================
  Files         266      266           
  Lines       10664    10664           
  Branches     2590     2590           
=======================================
  Hits         6626     6626           
  Misses       3448     3448           
  Partials      590      590
Impacted Files Coverage Δ
packages/jest-runtime/src/index.ts 69.39% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01044da...bebca61. Read the comment docs.

@SimenB

SimenB approved these changes Apr 7, 2019

Copy link
Collaborator

left a comment

This is so awesome!

Changelog? 😀

@SimenB

This comment has been minimized.

Copy link
Collaborator

commented Apr 7, 2019

I'm fairly certain it's also a bug fix: when you reset the registry, this reference to the registry continued referencing the old version. That doesn't make sense after a reset.

I agree. Hopefully nobody (unwittingly) relied on getting old versions of modules after a reset

@thymikee
Copy link
Collaborator

left a comment

nice!

@jeysal

jeysal approved these changes Apr 7, 2019

Copy link
Collaborator

left a comment

Great work identifying this ❤️

@jeysal

This comment has been minimized.

Copy link
Collaborator

commented Apr 7, 2019

Also cc @dubzzz fast-check is not to blame after all ;)

@cpojer

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

This is great! Memory leaks are the worst.

@scotthovestadt scotthovestadt merged commit 650c0b5 into facebook:master Apr 7, 2019

11 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-11 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
facebook.jest #20190407.8 succeeded
Details
@peterdemartini

This comment has been minimized.

Copy link

commented Apr 11, 2019

@scotthovestadt can we get a release with this PR? Our tests are broken in CI due to memory leaks and it would be nice to be able to test this.

@scotthovestadt

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@peterdemartini Yes, soon! There will be a release within a week.

Please report back and let me know if this has solved it for you after the release.

@johannessjoberg

This comment has been minimized.

Copy link

commented Apr 30, 2019

Any updates concerning the release @scotthovestadt ? 🙏

@SimenB SimenB referenced this pull request May 15, 2019

Open

Uglier inline snapshots #7792

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.