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

[package.json] Enable max_old_space_size Override for Desktop #38576

Merged
merged 1 commit into from
Jan 2, 2020

Conversation

nsakaimbo
Copy link
Contributor

@nsakaimbo nsakaimbo commented Dec 23, 2019

The behavior of heap limits and garbage collection in Node 12 has changed such that attempts to build wp-desktop with updated Calypso (which is now using Node 12) fail due to memory pressure. As-is, Node will attempt to use up to the set heap allocation (max_old_space_size) of 8GB but runs into the hard 4GB limit of Circle CI's RAM at which point Node kills the build process.

// ...
lerna success - photon
lerna success - @automattic/tree-select
Rendering Complete, saving .css file...
Wrote CSS to /home/circleci/wp-desktop/calypso/public/directly.css
Rendering Complete, saving .css file...
Wrote CSS to /home/circleci/wp-desktop/calypso/public/editor.css
Killed

We need to tell Node to allocate to the heap less than the Docker container's 4GB RAM limit. However, it seems there's a fundamental flaw in the way NODE_ARGS is set such that this parameter cannot be overriden at build time.

Simplified Example

A simple example to illustrate:

"scripts": {
  "run-my-script": "ENV_VAR=default npm run my-script",
  "my-script": "echo $ENV_VAR"
}
> ENV_VAR=custom npm run run-my-script
> echo $ENV_VAR
> default

// Want "custom", but get "default"
// Not what we intend or expect

In a nutshell, the above example illustrates the problem with the way NODE_ARGS is currently set. We need to be able to override this value when building wp-desktop, but are unable to.

(Side note: For some reason, this hasn't been an issue until Node 12 updated the way memory is managed. In addition, this only seems to affect Calypso as it is built for wp-desktop. However the issue with the way NODE_ARGS is set is a problem regardless.)

Changes proposed in this Pull Request

The values of NODE_ARGS and max_old_space_size for wp-desktop should be able to be overriden per the constraints of the hardware Caypso is being built on. Using the syntax NODE_ARGS=${NODE_ARGS:---max_old_space_size=8192} ensures that the default value of 8192MB will be used unless overridden by the caller, so we can maintain the existing behavior and customize this parameter where necessary.

Testing instructions

By pointing wp-desktop at this branch, we can verify that enabling the override of the hard-coded max_old_space_size value fixes the build failure on CI for wp-desktop.

  • Before: out-of-memory - process killed (build job]
  • After: max_old_space_size override is respected, and Calypso build is successful (build job)

@nsakaimbo nsakaimbo added [Type] Bug [Pri] BLOCKER Requires immediate attention. [Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. labels Dec 23, 2019
@nsakaimbo nsakaimbo requested a review from a team as a code owner December 23, 2019 18:03
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@nsakaimbo nsakaimbo force-pushed the do-not-hard-code-max-old-space-size branch 2 times, most recently from 9ff16fc to d6fb6ab Compare December 23, 2019 19:24
Behavior of heap limits in Node 12 has changed, such that the hard-coded value of max_old_space_size breaks the wp-desktop build. In addition, the way the default value has been coded to0-date makes it so this parameter could not in fact be overriden when invoked via the command line.
@nsakaimbo nsakaimbo force-pushed the do-not-hard-code-max-old-space-size branch from d6fb6ab to 4dfec76 Compare December 23, 2019 19:40
@nsakaimbo nsakaimbo requested a review from sirreal December 23, 2019 20:52
@nsakaimbo nsakaimbo changed the title [package.json] Remove Hard-Coded max_old_space_size for Desktop [package.json] Enable max_old_space_size Override for Desktop Dec 23, 2019
@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 23, 2019
@nsakaimbo
Copy link
Contributor Author

Seems like some additional CI jobs were triggered by wp-desktop and lines were crossed somewhere - not sure why those jobs are showing up here. Please ignore ci/wp-desktop, ci/wp-e2e-tests-full-desktop and ci/wp-e2e-tests-full-desktop as those have no bearing on this PR.

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 23, 2019
@nsakaimbo nsakaimbo merged commit ea170c6 into master Jan 2, 2020
@nsakaimbo nsakaimbo deleted the do-not-hard-code-max-old-space-size branch January 2, 2020 15:28
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WordPress Desktop App Features and improvements related to the WordPress Desktop App. [Pri] BLOCKER Requires immediate attention. [Type] Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants