-
Notifications
You must be signed in to change notification settings - Fork 433
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
Make memory to build UI more configurable #1037
Comments
@artlowel and @atarix83 : I'd appreciate your (very quick) opinions on this. I suspect this might help make it easier for people to build the UI on lower memory machines, but I also realize that will make the build much slower as a result. Another option here could be to change the default to 2-4GB (which should work for everyone), but allow a way for CI & developers to override that value easily. This is low priority right now, but just wanted quick feedback on the idea. |
A 2GB default is what I'm currently using for the UI build, on an AWS ec2 t3.medium instance (4GB RAM... which is the same size of VM I'm using for our API instance). |
From chatting with @hardyoyo on Slack, it sounds like he's tested that Node v12 (and above) manage the memory automatically (by using any available memory as noted in https://nodejs.medium.com/introducing-node-js-12-76c41a1b3f3f). So, it appears that the Therefore, it might be worth considering removing this setting from our |
I've verified that it works without --max_old_space_size, on node 12 and 14. Since, in order to test it, I had to make the changes anyway I created a PR: #1038 Node 10 is EOL in April this year, so I wouldn't worry about supporting it too much |
It turns out that with node 12 the max amount of memory is still only 2GB: nodejs/node#28202 This seems a likely cause for the issues we've been having with the demo server. I've set the max higher using an environment variable for now. We'll see whether that improves stability. If it does, it still doesn't mean we have to add that flag to our yarn script again, but we may have to add it to the docs. |
CST-12498 fix tests errors and lint errors Approved-by: Stefano Maffei
Describe the bug
Currently, we "hardcode" & require 8GB of available memory to build the Angular UI. See this setting:
https://github.com/DSpace/dspace-angular/blob/main/package.json#L26
Ideally, we should support systems which have less than 8GB of available memory, as there's no reason (that I can tell) for this default setting except that it speeds up the build on systems which have more memory available. There's no way to easily override this default setting, unless you manually edit the
package.json
before building the UI.Should we consider removing the
--max_old_space_size=8192
flag in that setting? That would allow users more control over how much memory they wanted to provide by using theNODE_OPTIONS
environment variable, like:NODE_OPTIONS="--max_old_space_size=8192" yarn start
(The downside to this approach appears to be that--max_old_space_size
seems to default to 512MB, which is really low)UPDATE: In Node v12, it appears that Node might automatically just use "available memory". See "Properly configure default heap limits" section here: https://nodejs.medium.com/introducing-node-js-12-76c41a1b3f3f
Obviously, if we remove this default, we may want to update our CI build process to still use 8GB by setting this environment variable in our build.yml: https://github.com/DSpace/dspace-angular/blob/main/.github/workflows/build.yml#L12
The text was updated successfully, but these errors were encountered: