[objects] Introduce CppGCManaged<T> to replace legacy Managed<T> [v8/v8 : main]
Andreas Haas (Gerrit)
Andreas Haas added 1 comment![]( "Open in Gerrit")
v8::Isolate* isolate_ = nullptr;Andreas HaasHow do we guarantee that `Isolate` doesn't go away? (WebWorker case)
Andreas HaasIsn't this an Oilpan object, don't all Oilpan objects get sweeped when the Oilpan heap teared down, which happens during isolate tear down?
I checked it again, and you are right, there is a small issue. Not in the production case, where my argument is correct, but for blink tests that reuse the CppHeap, we would run into an issue. I fixed it by adding a flag that the isolate is only used when the isolate still exists.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 9 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
9 comments
lgtm
I think we can improve some of this incrementally on top
class CppHeapObjectWrapper {Andreas HaasDo we really need to add `CppGCManagedBase` to this machinery? We have a field `CppHeapPointerMember` now that should make this unnecessary?
(This wrapping type was created before we had the field type and can only work with a single cppgc field)
Michael LippautzAs far as I know, this is only needed for the write barrier (called in managed-inl.h, `WriteBarrier::ForCppHeapPointer(...)`), and as far as I can tell, it is only needed for Oilpan young-gen.
Oilpan doesn't have a supported young gen and I will remove than "soon". Writing a doc now.
As for the write barrier (for marking): This is a bit backwards: The barrier should be emitted by CppHeapPointerMember which knows the offset (of `this`). We can address this in a follow up.
raw->WriteLazilyInitializedCppHeapPointerField(
offsetof(CppGCManagedBase, cpp_gc_wrapper_), isolate,
reinterpret_cast<Address>(destructor),
CppHeapPointerTag::kCppGCManagedTag);
WriteBarrier::ForCppHeapPointer(
raw,
raw->RawCppHeapPointerField(offsetof(CppGCManagedBase, cpp_gc_wrapper_)),
destructor);This should really be a cpp_gc_wrapper_.Store(...); that does the store and performs the write barrier.
We can address this in a followup.
Handle<CppGCManaged<CppType>> CppGCManaged<CppType>::From(I would rename this `Create()` as it actually creates a JS object (among other things).
: public cppgc::GarbageCollected<CppGCManagedWrapper> {More naming thoughts: Managed<> will eventually be fully replaced, right?
In that case we can drop the CppGC prefix from the new classes.
V8_OBJECT class CppGCManaged : public CppGCManagedBase {This is the goto version of the JS world: Lets give it a nice comment. Also, how about we move it to the top of the file?
class CppGCManagedWrapper finalI find the naming a bit weird here still, because for bindings purposes we generally call
- the JS objects wrappers
- the C++ objects wrappables
class Managed : public Foreign {Please leave behind a deprecation message with some strong wording that new types must use the cppgc version
const char* name = "system / CppGCManaged";Let's preserve the debuggability here with the %s tag_name.
Related details
- Andreas Haas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +0 |
1 comment
saelo: fyi
Related details
- Andreas Haas
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreas Haas (Gerrit)
Andreas Haas added 7 comments![]( "Open in Gerrit")
class CppHeapObjectWrapper {Andreas HaasDo we really need to add `CppGCManagedBase` to this machinery? We have a field `CppHeapPointerMember` now that should make this unnecessary?
(This wrapping type was created before we had the field type and can only work with a single cppgc field)
Michael LippautzAs far as I know, this is only needed for the write barrier (called in managed-inl.h, `WriteBarrier::ForCppHeapPointer(...)`), and as far as I can tell, it is only needed for Oilpan young-gen.
Oilpan doesn't have a supported young gen and I will remove than "soon". Writing a doc now.
As for the write barrier (for marking): This is a bit backwards: The barrier should be emitted by CppHeapPointerMember which knows the offset (of `this`). We can address this in a follow up.
Ack
raw->WriteLazilyInitializedCppHeapPointerField(
offsetof(CppGCManagedBase, cpp_gc_wrapper_), isolate,
reinterpret_cast<Address>(destructor),
CppHeapPointerTag::kCppGCManagedTag);
WriteBarrier::ForCppHeapPointer(
raw,
raw->RawCppHeapPointerField(offsetof(CppGCManagedBase, cpp_gc_wrapper_)),
destructor);This should really be a cpp_gc_wrapper_.Store(...); that does the store and performs the write barrier.
We can address this in a followup.
Ack.
Handle<CppGCManaged<CppType>> CppGCManaged<CppType>::From(I would rename this `Create()` as it actually creates a JS object (among other things).
Done
: public cppgc::GarbageCollected<CppGCManagedWrapper> {More naming thoughts: Managed<> will eventually be fully replaced, right?
In that case we can drop the CppGC prefix from the new classes.
I agree. I plan to rename CppGCManaged to Managed once I'm done with the porting.
V8_OBJECT class CppGCManaged : public CppGCManagedBase {This is the goto version of the JS world: Lets give it a nice comment. Also, how about we move it to the top of the file?
Done
class Managed : public Foreign {Please leave behind a deprecation message with some strong wording that new types must use the cppgc version
Done
Let's preserve the debuggability here with the %s tag_name.
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreas Haas (Gerrit)
Andreas Haas voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreas Haas (Gerrit)
Andreas Haas added 1 comment![]( "Open in Gerrit")
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
stamp, assuming michi's +1 was dropped accidentally.
Related details
- Andreas Haas
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andreas Haas (Gerrit)
Andreas Haas voted Commit-Queue+2![]( "Open in Gerrit")
- Michael Lippautz
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
[objects] Introduce CppGCManaged<T> to replace legacy Managed<T>
This CL introduces CppGCManaged<T> and its base class CppGCManagedBase,
a sandbox-aware replacement for legacy Managed<T> that leverages V8's
CppHeapPointerTable and Oilpan (cppgc) for automatic lifetime
management.
Unlike legacy Managed<T> (which stores pointers in the
ExternalPointerTable and requires manual destructor registration with
the Isolate), CppGCManaged<T> allocates a cppgc::GarbageCollected
destructor in the CppHeap. This allows Oilpan to automatically invoke
destructors during CppHeap teardown without requiring manual
registration or tracking on the Isolate.
Key changes:
- Introduce CppGCManagedBase (Torque class extending HeapObject)
containing a single cpp_heap_destructor_ field pointing to a
GarbageCollected CppGCManagedPtrDestructor object.
- Add CppGCManagedBase to the CppHeapPointerWrapperObjectT write barrier
host union, IsCppHeapPointerWrapperObject predicate, map visitor IDs,
and BodyDescriptor to support write barriers and GC
marking/compacting.
- Add CppGCManaged<T>::Ptr move-only smart pointer view with
V8_LIFETIME_BOUND annotations for Clang lifetime safety.
- Migrate WasmMemoryObject::managed_backing_store to
CppGCManaged<BackingStore> as an initial use case and end-to-end
integration test.
- Add unit test ManagedTest.CppGCManagedGCCausesDestruction.
NO_IFTTT=Non-JSObject heap objects (e.g. CppGCManagedBase) are not handled JSObject::GetHeaderSize.
- M BUILD.bazel
- M BUILD.gn
- M include/v8-sandbox.h
- M src/compiler/turbofan-types.cc
- M src/diagnostics/objects-debug.cc
- M src/diagnostics/objects-printer.cc
- M src/execution/isolate.cc
- M src/execution/isolate.h
- M src/heap/cppgc-js/cpp-heap.cc
- M src/heap/cppgc-js/cpp-heap.h
- M src/heap/factory.cc
- M src/heap/factory.h
- M src/heap/heap-visitor.h
- M src/heap/setup-heap-internal.cc
- M src/objects/backing-store.h
- M src/objects/cpp-heap-object-wrapper-inl.h
- M src/objects/cpp-heap-object-wrapper.h
- M src/objects/instance-type-inl.h
- M src/objects/js-objects.cc
- M src/objects/managed-inl.h
- A src/objects/managed-type-id.h
- M src/objects/managed.cc
- M src/objects/managed.h
- A src/objects/managed.tq
- M src/objects/map.cc
- M src/objects/map.h
- M src/objects/object-list-macros.h
- M src/objects/objects-body-descriptors-inl.h
- M src/objects/objects-inl.h
- M src/profiler/heap-snapshot-generator.cc
- M src/roots/roots.h
- M src/roots/static-roots-intl-nowasm.h
- M src/roots/static-roots-intl-wasm.h
- M src/roots/static-roots-nointl-nowasm.h
- M src/roots/static-roots-nointl-wasm.h
- M src/runtime/runtime-wasm.cc
- M src/sandbox/isolate.h
- M src/wasm/module-instantiate.cc
- M src/wasm/wasm-js.cc
- M src/wasm/wasm-memory-map-descriptor.cc
- M src/wasm/wasm-objects-inl.h
- M src/wasm/wasm-objects.cc
- M src/wasm/wasm-objects.h
- M src/wasm/wasm-objects.tq
- M test/cctest/wasm/test-run-wasm-module.cc
- M test/common/wasm/fuzzer-common.cc
- A test/mjsunit/sandbox/regress-535892918.js
- M test/unittests/objects/managed-unittest.cc
Code-Review: +1 by Leszek Swirski
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |