-
Notifications
You must be signed in to change notification settings - Fork 30k
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
cli: allow --huge-max-old-generation-size in NODE_OPTIONS #32251
cli: allow --huge-max-old-generation-size in NODE_OPTIONS #32251
Conversation
CI: https://ci.nodejs.org/job/node-test-pull-request/29816/ (:white_check_mark:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also add what it increased to?
As you said on twitter: "For machines with a lot of RAM, that behavior is changed to 4GB on x64 if --huge-max-old-generation-size is passed to V8."
Honestly, I doubt that many people would use end up using this – I’m okay with documenting it more verbosely, but it gets tricky to say that this increases the limit of the default limit of the heap (and not the actual limit like --max-old-space-size does). Ultimately, it probably makes more sense to document |
I guess I have one other question: Is there a reason node is not defaulting to |
I think this goes back to: should we be documenting V8 flags? While I agree these flags are important, it's still not something under our control. The behavior can change across V8 versions and we don't have test their behaviors, only for their existence. I'm not against it, especially for important flags like memory-tuning, but maybe there's something else we can do? Could we add a NODE_V8_OPTIONS environment variable instead, so it's explicit those flags are not supported by us? For the memory-related flags, could Node.js expose a "simplified" tuning flag (and those wanting something more fine-tuned can use the V8 flags)? |
@wesleytodd Yeah, I've thought about that too. It would be fairly easy to make that happen, I'm not sure why it's not the default in V8. @mmarchini Yeah, I think it's a bit of a grey are, but I'd prefer to keep them unless we have a reason to disable some of them... |
PR-URL: #32251 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: David Carlier <[email protected]>
Landed in 95e3733 |
PR-URL: #32251 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: David Carlier <[email protected]>
PR-URL: nodejs#32251 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: David Carlier <[email protected]>
PR-URL: #32251 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: David Carlier <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes