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

Add global.gc() afterAll and increase max-old-space-size #895

Merged
merged 5 commits into from
Aug 12, 2019

Conversation

dennissivia
Copy link
Contributor

While running tests locally I repeatedly ran into memory allocation errors:
FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory.

I enabled logHeapUsage and noticed, that even test failures related to timeouts and missing mock requests were GC related.
There are open issues regarding jest like this, and when running jest in watch mode, that total memory keeps growing. On my system, it consumed more than 4GB after 4-5 full test runs.

In order to address these issues, I added the following changes to the test command invocation:

  • expose-gc to allow us manually running global.gc() afterAll
  • logHeapUsage for manual inspection
  • max-old-space-size=4096 to increase the old-gen size to 4 Gig instead of 1,5 Gig. This should make testmore reliable with regards to 'out of memory' and 'timeout' errors.
  • setting node flags implied adding the path to jest, which is good to make sure, that everybody run tests with the same jest version and does not accidentally use a globally installed version.

If your laptop does not support 4GB for the GC, just add NODE_OPTIONS=--max-old-space-size=1024 before npm test and the value will be passed to v8.
See allowed v8 flags

This should make tests more reliable and limit the memory allocation in watch mode.

Finally, I extracted the custom jestTimeout into a constant instead of having a magic number in many tests.

Copy link
Contributor

@wilhelmklopp wilhelmklopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! 🚀 ✨

If your laptop does not support 4GB for the GC, just add NODE_OPTIONS=--max-old-space-size=1024 before npm test and the value will be passed to v8.
See allowed v8 flags

Should we add a note about this in the CONTRIBUTING.md ?

} = models;
const { SlackUser, GitHubUser, Installation } = models;

const jestTimeout = 10000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the default value usually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 seconds is the jest default. This PR doesn't change the value of our tests.
I just extracted the value from the tests into a constant.

"test": "jest --coverage --runInBand --forceExit",
"test:watch": "jest --runInBand --forceExit --watch --notify",
"test": "node --expose-gc --max-old-space-size=4096 ./node_modules/.bin/jest --coverage --runInBand --logHeapUsage --forceExit",
"test:watch": "node --expose-gc --max-old-space-size=4096 ./node_modules/.bin/jest --runInBand --logHeapUsage --forceExit --watch --notify",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe node_modules/.bin are automatically part of the $PATH thanks to some node/package.json magic, so we might be able to drop that? Buuut I have never done anything this low level with node, so it's very possible that I'm very wrong here 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct about the PATH, but we don't want to invoke jest, because that would use it's 'shebang' line which is running node without any flags. So that is, what we used before.
The purpose of this change is to directly call the node executable and point it to the script it should run. It this case jest. So the PATH does only matter for the first command in the line (which is node). Hope that makes sense.

Jest now logs memory consumption, since we often run into GC pauses
while executing tests. As a result they are unnecessarily slow and more
unreliable.

Tests are now run with `-expose-gc`, which allows us to call
`global.gc()` between runs (beforeAll), which should make the memory
consumption in `watch` mode more stable.
@dennissivia dennissivia force-pushed the improve-gc-insights-for-tests branch from a0d6923 to dc4290f Compare August 9, 2019 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants