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: include default max_old_space_size in binaries [HEAD-1045] #168

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

cat2608
Copy link
Contributor

@cat2608 cat2608 commented Nov 21, 2023

  • Tests written and linted ℹ︎
  • Commits are squashed and tidy and are suitable to become release notes

What this does

This PR introduces a change in the packaging command for our binaries to specify a default maximum old space size for V8's memory. The change was prompted by the need to accommodate larger projects that may require more memory than the default settings allow.

A change in the pkg-fetch repository (see vercel/pkg-fetch@7f3403b) disabled the NODE_OPTIONS environment variable, which previously allowed consumers to set Node.js runtime options, including --max-old-space-size, at runtime. This has impacted users who need to increase the memory limit beyond the default when running the snyk-to-html tool.

Given that we cannot rely on NODE_OPTIONS, this PR takes the approach of setting a conservative default memory limit at build time using the --options max_old_space_size=16384 flag. This will set the old space size to 16GB, which should suffice for the majority of use cases without manual intervention.

Notes for the reviewer

To be able to test this change I added a log like the following:

async function main() {
  console.log(
    'Heap size limit:',
    v8.getHeapStatistics().heap_size_limit / 1024 / 1024,
    'MB',
  );
}

and we can see the output in the following image:

test

More information

HEAD-1045
HEAD-1051

@cat2608 cat2608 requested a review from a team as a code owner November 21, 2023 12:14
.releaserc Outdated
@@ -4,7 +4,7 @@
{
"//": "build the alpine, macos, linux and windows binaries",
"path": "@semantic-release/exec",
"cmd": "npm i -g pkg && pkg . -t node14-alpine-x64,node14-linux-x64,node14-macos-x64,node14-win-x64"
"cmd": "npm i -g pkg && pkg . --options max_old_space_size=16384 -t node14-alpine-x64,node14-linux-x64,node14-macos-x64,node14-win-x64"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I think we should pin the version and not use latest all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which version do you mean? For the pkg tool?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, npm i -g pkg installs the latest version of pkg

Copy link
Contributor Author

@cat2608 cat2608 Nov 21, 2023

Choose a reason for hiding this comment

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

Could you please share more about the benefits you see in pinning, and what issues we're aiming to prevent?

Pinning does bring build stability, but considering that pkg is just a build tool and our pipeline doesn't run often as far as I can see, I'm more concerned about being outdated over time and missing bug fixes or improvements of the tool.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Currently also major upgrades will be included even breaking changes, which is problematic.
  • PKG is not just a build tool, it determines which node version is being bundled in the binaries.
  • Pinning doesn't mean to not consume bugfix releases, but at least not automatically consume major changes.

@cat2608 cat2608 merged commit 6a578dc into master Nov 22, 2023
11 checks passed
@cat2608 cat2608 deleted the fix/HEAD-1045-HEAD-1051-increase-memory-limit-node branch November 22, 2023 09:51
Copy link

🎉 This PR is included in version 2.3.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants