[cppgc] Delay cross-thread root marking [v8/v8 : main]
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Commit-Queue | +1 |
1 comment
Related details
- Omer Katz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 2 comments![]( "Open in Gerrit")
enum class CrossThreadRootMarking {I'm not sure I see the value in this enum. I think it would the code would be simpler if we just explicitly call `MarkCrossThreadRoots` from the one location that actually uses `kAutomatic`.
CHECK(visited_cross_thread_persistents_in_atomic_pause_);Why check this twice (here and line 405 above)?
Related details
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Commit-Queue | +1 |
2 comments
enum class CrossThreadRootMarking {I'm not sure I see the value in this enum. I think it would the code would be simpler if we just explicitly call `MarkCrossThreadRoots` from the one location that actually uses `kAutomatic`.
It keeps the `cppgc` API simple and contained. I think its's beneficial to have one implementation where we don't tear the calls apart. E.g., every now and then we reshuffle testing calls and I think it's nice that some things there stay simple.
CHECK(visited_cross_thread_persistents_in_atomic_pause_);Why check this twice (here and line 405 above)?
- Omer Katz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
enum class CrossThreadRootMarking {Michael LippautzI'm not sure I see the value in this enum. I think it would the code would be simpler if we just explicitly call `MarkCrossThreadRoots` from the one location that actually uses `kAutomatic`.
It keeps the `cppgc` API simple and contained. I think its's beneficial to have one implementation where we don't tear the calls apart. E.g., every now and then we reshuffle testing calls and I think it's nice that some things there stay simple.
Users of the standalone library (and most tests iirc) would anyway go through `MarkerBase::FinishMarking`. Do you think calling `MarkCrossThreadRoots` explicitly from `FinishMarking` (e.g. after joining concurrent marking, which would also keep `cppgc` and `cppgc-js` aligned) will degrade the API?
Related details
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
enum class CrossThreadRootMarking {Michael LippautzI'm not sure I see the value in this enum. I think it would the code would be simpler if we just explicitly call `MarkCrossThreadRoots` from the one location that actually uses `kAutomatic`.
Omer KatzIt keeps the `cppgc` API simple and contained. I think its's beneficial to have one implementation where we don't tear the calls apart. E.g., every now and then we reshuffle testing calls and I think it's nice that some things there stay simple.
Users of the standalone library (and most tests iirc) would anyway go through `MarkerBase::FinishMarking`. Do you think calling `MarkCrossThreadRoots` explicitly from `FinishMarking` (e.g. after joining concurrent marking, which would also keep `cppgc` and `cppgc-js` aligned) will degrade the API?
It's essentially about putting the call into `FinishMarking()` indeed.
I think the API here for the marker is nicer and better guided than with an extra call.
Related details
- Omer Katz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
2 comments
enum class CrossThreadRootMarking {Michael LippautzI'm not sure I see the value in this enum. I think it would the code would be simpler if we just explicitly call `MarkCrossThreadRoots` from the one location that actually uses `kAutomatic`.
Omer KatzIt keeps the `cppgc` API simple and contained. I think its's beneficial to have one implementation where we don't tear the calls apart. E.g., every now and then we reshuffle testing calls and I think it's nice that some things there stay simple.
Michael LippautzUsers of the standalone library (and most tests iirc) would anyway go through `MarkerBase::FinishMarking`. Do you think calling `MarkCrossThreadRoots` explicitly from `FinishMarking` (e.g. after joining concurrent marking, which would also keep `cppgc` and `cppgc-js` aligned) will degrade the API?
It's essentially about putting the call into `FinishMarking()` indeed.
I think the API here for the marker is nicer and better guided than with an extra call.
I disagree and I think the enum is more noise the value, but it's not worth the time to argue over it.
Related details
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
1 comment
Related details
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted Commit-Queue+2![]( "Open in Gerrit")
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[cppgc] Delay cross-thread root marking
Cross-thread roots (CrossThreadPersistent and
WeakCrossThreadPersistent) marking and weakness clearing requires
obtaining a lock from the GC during the final atomic pause. The same
lock is used for creating these references. This way we ensure that
the GC doesn't reclaim objects that are held by other threads. It also
ensures that converting a weak into a strong persistent is consistent
wrt to GC running on a different thread.
In order to minimize the impact of this lock we already delayed the
acquisition in GC till we were done marking all CppHeap objects.
However, there may be a lot of JS objects left which can cause quite a
large window where this lock is taken.
This CL only acquires the lock at the very end of marking when
everything else is marked. There will usually not be many objects left
to be marked from this root set.
In addition, we clear cross-thread weak roots immediately durng weak
roots processing which allows us to further shrink down the time
window where the GC holds the lock.
- M src/heap/cppgc-js/cpp-heap.cc
- M src/heap/cppgc-js/cpp-heap.h
- M src/heap/cppgc/marker.cc
- M src/heap/cppgc/marker.h
- M src/heap/mark-compact.cc
- M src/heap/minor-mark-sweep.cc
Code-Review: +1 by Omer Katz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |