-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
src: initial enablement of IsolateGroups #60254
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
6e01998
to
426bbea
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
bf8d6c9
to
4c346fa
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This is ready for review! I'm not yet convinced that it's (pointer compression in general) is safe to enable by default as it may be too breaking for some workloads but with isolate groups at least we have the option now of using multiple worker threads and don't have the process-wide 4 GB limit. I'm going to experiment next the v8 option to allow for an 8 GB heap limit. There should be no real noticeable difference in performance but overall apps should be using significantly less memory with pointer compression enabled. To give a rough idea, here's two comparisons of the memory just starting the node.js repl.. first is existing node 24, the second with main with pointer compression enabled:
Note that |
This comment was marked as outdated.
This comment was marked as outdated.
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.
I've tested and it works pretty well.
Marking this author ready as it has one sign off and has passed CI... however, before landing I'd like to see if we can get signoffs from @targos and or @joyeecheung and or @addaleax . |
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.
LGTM % nits
9580be9
to
ca0b57f
Compare
7290cde
to
1a5f855
Compare
This comment was marked as outdated.
This comment was marked as outdated.
I've tested this with an "hello world" SSR Next.js application, which reduces Total Heap by roughly 50% (400MB -> 200MB). It's amazing. There is also some possible throughput improvement, but I'm seeing inconsistent numbers, and I need to nail it down. |
cc @nodejs/delivery-channels @nodejs/embedders - this changes the build configuration of whoever using |
There's also another V8 flag that is supposed to enable an 8gb cage but initial testing is showing some compile errors in v8 that I need to figure out. |
FWIW I noticed that |
Thanks for the call out, yes we run with V8 sandbox enabled which requires shared pointer compression cage so isolate groups would not be an option in the default mode, I see there is work to enable sandbox per isolate group https://issues.chromium.org/issues/342905186 which is interesting. Looking at the current changes, any reason |
We are running sandbox with multi-cages in workers and it works great. Might need some tweaks
We certainly can. I think maybe we'd make it as an opt out rather than an opt in since I think our default for node should really be to have multiple cages. I can add that before landing this. |
1a5f855
to
14723a7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Added an additional compile flag to switch on shared cage. @deepak1556 ... once this lands, you'll just need to add |
|
Thank you for starting the additional ci jobs! Saved me a step! :-) |
This comment was marked as outdated.
This comment was marked as outdated.
14723a7
to
ec710da
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Mostly LGTM, but with a safety question.
Perfect, thank you! |
This lays the initial groundwork for enabling the use of IsolateGroups. Every isolate in V8 is created within a group. When pointer compression is enabled, all isolates within a single group are limited to a 4 GB shared pointer cage. By default, all isolates in the process share the same group, which means that when running with pointer compression, the entire process would be limited to a single 4 GB shared pointer cage. But, we can create as many IsolateGroups as we want, limited only by the amount of virtual memory available on the machine.
ec710da
to
c135312
Compare
Enable IsolateGroups!
Every isolate in V8 is created within a group. When pointer compression is enabled, all isolates within a single group are limited to a 4 GB shared pointer cage. By default, all isolates in the process share the same group, which means that when running with pointer compression, the entire process would be limited to a single 4 GB shared pointer cage. But, we can create as many IsolateGroups as we want, limited only by the amount of virtual memory available on the machine.
In the default node.js build, this is disabled. When only pointer compression is turned on, isolate groups are used.
/cc @erikcorry @nodejs/v8