Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Set max-old-space-size in run command of ui #3060

Merged
merged 3 commits into from
Feb 20, 2020

Conversation

ljewalsh
Copy link
Contributor

@ljewalsh ljewalsh commented Jan 30, 2020

Heap size on node 12 defaults to 1.4G. This is causing issues with our webpack build. Setting max-old-space-size to 2Gb solves the issue. Since we are already using 1.4Gb, raising to 2Gb shouldnt be an issue

@ljewalsh ljewalsh force-pushed the add-webpack-memory-error-documentation branch from 94ff795 to 8e9e777 Compare January 30, 2020 23:04
@ljewalsh ljewalsh changed the title Add information about setting NODE_OPTIONS for ui when webpack throws memory error to README Add information about setting NODE_OPTIONS when webpack throws memory error Jan 30, 2020
@balena-ci
Copy link
Collaborator

balena-ci commented Jan 30, 2020

Ship shape and ready to sail!

Most Updated Files

Last Month Last Week
2 lib/sync/integrations/discourse.js 1 lib/sdk/index.spec.js
2 lib/core/kernel.js 1 lib/sdk/index.js
2 lib/core/backend/postgres/streams.js 1 lib/core/backend/postgres/streams.js
2 lib/core/backend/postgres/index.js
2 lib/chat-widget/environment.js

Most Updated Modules

Last Month Last Week
9 lib/chat-widget 1 lib/sync
8 lib/core 1 lib/core
3 lib/sdk
2 lib/sync
1 lib/worker

@joshbwlng
Copy link
Collaborator

From what I can gather, the default value for max-old-space-size is 512 MB and if there's no downside to beefing it up to 2 GB I would rather set this directly as the default in apps/ui/Dockerfile. I don't want to end up with a README full of "if you have problem X, try solution Y" if setting sane defaults fix the problem for everyone.

@LucianBuzzo
Copy link
Contributor

@joshbwlng would we hit issues running it on a device with limited memory, such as a NUC?

@joshbwlng
Copy link
Collaborator

@LucianBuzzo We would hit problems if we set max-old-space-size too high, but we wouldn't be able to run Jellyfish on a machine with limited memory anyway. Perhaps we should try with 1GB instead of 2GB on a machine with 8GB memory and see how that goes. I assume we would advise having at least 8GB memory for Jellyfish.

@ljewalsh
Copy link
Contributor Author

I tried with 1GB and it failed on my macbook. Seems it needs to be 2GB

@ffissore
Copy link
Contributor

TL;DR

Heap size on node 12 defaults to something bigger than 1.4G on machines with more that 2.8G of ram (although I don't know exactly how much)
IMHO setting --max-old-space-size=2048 is fine when building ui, while it rings a bell when needed at runtime (server side)

Long story

I spent some time doing some research.
According to this announcement and this github pr, since node 12, the way heap is sized has slightly changed, mostly in favour of smaller machines
Versioin of V8 included in node sets max_old_generation_size to 1.4G on x64 cpus. The user provided value of max_old_space_size is used to override this default.
Since node 12, node changes this defaults, favouring smaller machines (good for balena users), but ending up with slightly bigger heaps on bigger machines
So, we are already using a heap 1.4G big by default: raising it to 2G when building the UI should not be a problem
Interesting side note: it seems V8 limits the old generation size to a max of 2G

README.md Outdated Show resolved Hide resolved
In order to solve the webpack memory issue

Change-type: patch
Signed-off-by: Lucy-Jane Walsh <[email protected]>
@ljewalsh ljewalsh force-pushed the add-webpack-memory-error-documentation branch from a74dc65 to 1ed0c6a Compare February 13, 2020 02:52
@ljewalsh ljewalsh changed the title Add information about setting NODE_OPTIONS when webpack throws memory error Set max-old-space in run command of ui Feb 13, 2020
@ljewalsh ljewalsh changed the title Set max-old-space in run command of ui Set max-old-space-size in run command of ui Feb 13, 2020
@ljewalsh ljewalsh merged commit ee1309b into master Feb 20, 2020
@ljewalsh ljewalsh deleted the add-webpack-memory-error-documentation branch February 20, 2020 01:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants