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

feat: isolate groups #1695

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: isolate groups #1695

wants to merge 1 commit into from

Conversation

piscisaureus
Copy link
Member

@piscisaureus piscisaureus commented Feb 7, 2025

Currently requires the following gn flags to work:

v8_enable_external_code_space = true
v8_enable_pointer_compression = true
v8_enable_pointer_compression_shared_cage = false
v8_enable_sandbox = false

@piscisaureus piscisaureus force-pushed the isolate-groups branch 2 times, most recently from da21edb to 348b8a5 Compare February 7, 2025 16:53
@piscisaureus piscisaureus changed the title [TRY] feat: isolate groups feat: isolate groups Feb 7, 2025
unsafe impl Send for IsolateGroup {}
unsafe impl Sync for IsolateGroup {}

impl IsolateGroup {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that IsolateGroup currently does not implement Clone.
This is because the C++ class explicitly deletes the copy constructor.

We could probably work around this somehow, although a fairly naive hack that calls internal_isolate_group->Acquire() didn't work when I tried.

@devsnek
Copy link
Member

devsnek commented Feb 7, 2025

wow this looks identical to my local changes to this lol. could you update the module_snapshot test to use a new group? that will prevent it from being flakey when run without nextest.

@piscisaureus
Copy link
Member Author

wow this looks identical to my local changes to this lol. could you update the module_snapshot test to use a new group? that will prevent it from being flakey when run without nextest.

It doesn't currently build with multiple group support in CI (requires local .gn changes).

Currently requires the following gn flags to work:
```
v8_enable_external_code_space = true
v8_enable_pointer_compression = true
v8_enable_pointer_compression_shared_cage = false
v8_enable_sandbox = false
```
@piscisaureus
Copy link
Member Author

piscisaureus commented Feb 7, 2025

Unfortunately, it doesn't seem possible to use isolate groups with pointer compression disabled.
If I neuter the CanCreateNewGroups check in V8, it fails as follows:

#
# Fatal error in ../../../v8/src/objects/js-objects.cc, line 798
# Debug check failed: properties == roots.empty_fixed_array() || properties == roots.empty_property_dictionary() || properties == roots.empty_swiss_property_dictionary().
#
#
#
#FailureMessage Object: 0x16e782698

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants