Set max old space size on coverage and test

[arr00]

Fixes #5363

Hardhat has a known issue which results in JavaScript heap out of memory error unless the max old space size is set. This PR will automatically set it on required scripts (test and coverage).

Note: this fix only sets the NODE_OPTIONS max-old-space-size flag if it was not set already. Additional options in NODE_OPTIONS are maintained. The new flag is not maintained after the call is completed.

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: d44125c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[ernestognw]

From the docs:

--max-old-space-size=SIZE (in MiB)
Sets the max memory size of V8's old memory section. As memory consumption approaches the limit, V8 will spend more time on garbage collection in an effort to free unused memory.

On a machine with 2 GiB of memory, consider setting this to 1536 (1.5 GiB) to leave some memory for other uses and avoid swapping.

node --max-old-space-size=1536 index.js

I'm not sure what swapping refers to, but I'm worried setting it to 8192 makes the "old space size" constant and perhaps too much in low-capability devices. It doesn't say whether the default is calculated depending on the host, but I assume so based on this comment.

[Amxx]

• why 4096 and not more ? In the past (before the hardhat migration to EDR 4096 was not enough)
• why only on coverage and not on test ?

[arr00]

• why 4096 and not more ? In the past (before the hardhat migration to EDR 4096 was not enough)

  • why only on coverage and not on test ?

I'm testing on multiple machines (including CI) to see what we need. Ideally it would be below 8192 so that 8gb machines could run coverage. The test run requires significantly less memory to the point that the default is generally sufficient. We could set it to something like 4096 to just be safe though.

[Amxx]

I'm really not a big fan of this change:

• It fixes a value that may be to-much to to little depending on the evolution of the tests and tooling. Hardhat evolution did lower the needed value in the past. Same is true of coverage. Also, as we add more tests this could become outdated
• Different machines (hardware, os) will have different things available.

If our estimation is too tight, there is a risk we have to update it to frequently. If the estimation is not tight enough, it will some "small" machines

IMO this value should be controlled by the host of the machine. Not by us. And an issue with what this PR proposes is that it doesn't allow the machine's host to override the value we fix without editing the package.json.

[Amxx]

Can we remove that from the tests?

My understanding is that it was only really needed for the coverage ... and we now have an elegant integration in coverage.sh.

[arr00]

What are your thoughts on the current state?

[Amxx]

I'd keep this as the CI config. If this is ever modified, the script would not override it.

[Amxx]

do we need an eval here ?

Suggested change

	eval "scripts/set-max-old-space-size.sh"
	scripts/set-max-old-space-size.sh

[arr00]

This was already updated. The line is now

. ./scripts/set-max-old-space-size.sh

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
pypi/[email protected]	environment, eval, filesystem, network, shell	0	48.6 MB	certora, shellyg
pypi/[email protected]	eval, filesystem, shell, unsafe	0	1.01 MB	daejunpark, justin_gerard

View full report↗︎