Move GlobalSafepoint to IsolateGroup [v8/v8 : main]
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 5 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
5 comments
Move GlobalSafepoint to IsolateGroupNit: [execution, heap]
if (Isolate* shared_isolate = shared_space_isolate()) {Nit: There should always be a shared space isolate, so I would drop the if here.
global_safepoint_ = std::make_unique<GlobalSafepoint>(this);I think you can move this into the else branch of `has_shared_space_isolate()`. When we set up the shared space isolate, we can also allocate GlobalSafepoint.
shared_space_isolate_ = nullptr;Nit: We could drop GlobalSafepoint here when dropping the shared space isolate. This mirrors AddIsolate() which sets up GlobalSafepoint when the shared space isolate is added. That said, this shouldn't affect Chrome where the main/shared-space isolate never goes away. There may be a few tests that trigger this code path though.
Related details
- Jakob Kummerow
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
2 comments
LGTM
Isolate* shared_space_isolate() const;To fix the linker errors, this needs `V8_EXPORT_PRIVATE` (like on line 218, making this symbol visible on shared-library builds so that tests can call this).
Related details
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lively (Gerrit)
Thomas Lively added 5 comments![]( "Open in Gerrit")
Move GlobalSafepoint to IsolateGroupThomas LivelyNit: [execution, heap]
Done
if (Isolate* shared_isolate = shared_space_isolate()) {Nit: There should always be a shared space isolate, so I would drop the if here.
I've replaced the if with a DCHECK_NOT_NULL. The follow-up CL that will add a flag for enabling global safepoints without the shared heap might need the if again, but we can cross that bridge when we get to it.
To fix the linker errors, this needs `V8_EXPORT_PRIVATE` (like on line 218, making this symbol visible on shared-library builds so that tests can call this).
Done
global_safepoint_ = std::make_unique<GlobalSafepoint>(this);I think you can move this into the else branch of `has_shared_space_isolate()`. When we set up the shared space isolate, we can also allocate GlobalSafepoint.
The follow-on CL will have to move this out of the `v8_flags.shared_heap` check entirely, so I think keeping it somewhat separated here is useful.
shared_space_isolate_ = nullptr;Nit: We could drop GlobalSafepoint here when dropping the shared space isolate. This mirrors AddIsolate() which sets up GlobalSafepoint when the shared space isolate is added. That said, this shouldn't affect Chrome where the main/shared-space isolate never goes away. There may be a few tests that trigger this code path though.
Tying the lifetime of the GlobalSafepoint to the the lifetime of the shared space isolate is contrary to the goal of making it possible to have a GlobalSafepoint without a shared heap, i.e. without a shared space isolate.
Related details
- Dominik Inführ
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lively (Gerrit)
Thomas Lively voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Dominik Inführ
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lively (Gerrit)
Thomas Lively added 1 comment![]( "Open in Gerrit")
@jkummerow, @dinfuehr, Please take another look because the votes were reset when the last patch set was uploaded.
Jakob Kummerow (Gerrit)
Jakob Kummerow voted![]( "Open in Gerrit")
- Dominik Inführ
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 1 comment![]( "Open in Gerrit")
V8_EXPORT_PRIVATE Isolate* shared_space_isolate() const;It's `GlobalSafepoint::shared_space_isolate` that needs this annotation, not `IsolateSafepoint::…`.
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
1 comment
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lively (Gerrit)
Thomas Lively voted and added 2 comments![]( "Open in Gerrit")
Votes added by Thomas Lively
| Auto-Submit | +1 |
| Commit-Queue | +1 |
2 comments
I'll need the +1 votes again. Sorry for the churn.
V8_EXPORT_PRIVATE Isolate* shared_space_isolate() const;It's `GlobalSafepoint::shared_space_isolate` that needs this annotation, not `IsolateSafepoint::…`.
Thanks, fixed. What is the build config that would let me reproduce this problem locally? I took a look at the config on the failing bots, but it wasn't obvious to me.
Related details
- Dominik Inführ
- Jakob Kummerow
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
1 comment
Still LGTM
- Jakob Kummerow
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
V8_EXPORT_PRIVATE Isolate* shared_space_isolate() const;Thomas LivelyIt's `GlobalSafepoint::shared_space_isolate` that needs this annotation, not `IsolateSafepoint::…`.
Thanks, fixed. What is the build config that would let me reproduce this problem locally? I took a look at the config on the failing bots, but it wasn't obvious to me.
When you expand step `7. build` and then nested in it step `5. lookup GN args`, and then click the `gn_args` link, you'll see the GN args. Usually that's enough to repro.
Related details
- Thomas Lively
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Commit-Queue | +2 |
2 comments
global_safepoint_ = std::make_unique<GlobalSafepoint>(this);Thomas LivelyI think you can move this into the else branch of `has_shared_space_isolate()`. When we set up the shared space isolate, we can also allocate GlobalSafepoint.
The follow-on CL will have to move this out of the `v8_flags.shared_heap` check entirely, so I think keeping it somewhat separated here is useful.
Marking as resolved so we can land this.
shared_space_isolate_ = nullptr;Thomas LivelyNit: We could drop GlobalSafepoint here when dropping the shared space isolate. This mirrors AddIsolate() which sets up GlobalSafepoint when the shared space isolate is added. That said, this shouldn't affect Chrome where the main/shared-space isolate never goes away. There may be a few tests that trigger this code path though.
Tying the lifetime of the GlobalSafepoint to the the lifetime of the shared space isolate is contrary to the goal of making it possible to have a GlobalSafepoint without a shared heap, i.e. without a shared space isolate.
Marking as resolved so we can land this.
Related details
- Thomas Lively
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[execution, heap] Move GlobalSafepoint to IsolateGroup
In preparation for an experiment in which we will enable global
safepoints without the shared heap, refactor the global safepoint
infrastructure to be associated with an IsolateGroup rather than the
IsolateGroup's shared heap isolate. A follow-on change will introduce a
new feature flag for enabling global safepoints without the shared heap.
- M src/execution/isolate.cc
- M src/execution/isolate.h
- M src/heap/safepoint.cc
- M src/heap/safepoint.h
- M src/init/isolate-group.cc
- M src/init/isolate-group.h
Code-Review: +1 by Dominik Inführ, +1 by Jakob Kummerow
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |