diff --git a/include/v8-cppgc.h b/include/v8-cppgc.h index 09d7f89549..8860ea8a78 100644 --- a/include/v8-cppgc.h +++ b/include/v8-cppgc.h @@ -35,6 +35,9 @@ struct V8_EXPORT CppHeapCreateParams { */ class V8_EXPORT CppHeap { public: + static std::unique_ptr Create(v8::Platform* platform, + const CppHeapCreateParams& params); + virtual ~CppHeap() = default; /** diff --git a/include/v8.h b/include/v8.h index 9e1260ea22..6433784fdf 100644 --- a/include/v8.h +++ b/include/v8.h @@ -93,7 +93,6 @@ class Utils; class Value; class WasmMemoryObject; class WasmModuleObject; -struct CppHeapCreateParams; template class GlobalValueMap; template @@ -8469,16 +8468,6 @@ class V8_EXPORT Isolate { int embedder_wrapper_type_index = -1; int embedder_wrapper_object_index = -1; - /** - * If parameters are set, V8 creates a managed C++ heap as extension to its - * JavaScript heap. - * - * See v8::Isolate::GetCppHeap() for working with the heap. - * - * This is an experimental feature and may still change significantly. - */ - std::shared_ptr cpp_heap_params; - V8_DEPRECATED( "Setting this has no effect. Embedders should ignore import assertions " "that they do not use.") @@ -9100,8 +9089,26 @@ class V8_EXPORT Isolate { EmbedderHeapTracer* GetEmbedderHeapTracer(); /** - * \returns the C++ heap managed by V8. Only available if the Isolate was - * created with proper CreatePrams::cpp_heap_params option. + * Attaches a managed C++ heap as an extension to the JavaScript heap. The + * embedder maintains ownership of the CppHeap. At most one C++ heap can be + * attached to V8. + * + * This is an experimental feature and may still change significantly. + */ + void AttachCppHeap(CppHeap*); + + /** + * Detaches a managed C++ heap if one was attached using `AttachCppHeap()`. + * + * This is an experimental feature and may still change significantly. + */ + void DetachCppHeap(); + + /** + * This is an experimental feature and may still change significantly. + + * \returns the C++ heap managed by V8. Only available if such a heap has been + * attached using `AttachCppHeap()`. */ CppHeap* GetCppHeap() const; diff --git a/src/api/api.cc b/src/api/api.cc index 8f537aef3b..390ff6ccd7 100644 --- a/src/api/api.cc +++ b/src/api/api.cc @@ -8223,6 +8223,16 @@ EmbedderHeapTracer* Isolate::GetEmbedderHeapTracer() { return isolate->heap()->GetEmbedderHeapTracer(); } +void Isolate::AttachCppHeap(CppHeap* cpp_heap) { + i::Isolate* isolate = reinterpret_cast(this); + isolate->heap()->AttachCppHeap(cpp_heap); +} + +void Isolate::DetachCppHeap() { + i::Isolate* isolate = reinterpret_cast(this); + isolate->heap()->DetachCppHeap(); +} + CppHeap* Isolate::GetCppHeap() const { const i::Isolate* isolate = reinterpret_cast(this); return isolate->heap()->cpp_heap(); @@ -8335,9 +8345,6 @@ void Isolate::Initialize(Isolate* isolate, i_isolate->set_allow_atomics_wait(params.allow_atomics_wait); i_isolate->heap()->ConfigureHeap(params.constraints); - if (params.cpp_heap_params) { - i_isolate->heap()->ConfigureCppHeap(params.cpp_heap_params); - } if (params.constraints.stack_limit() != nullptr) { uintptr_t limit = reinterpret_cast(params.constraints.stack_limit()); diff --git a/src/heap/cppgc-js/cpp-heap.cc b/src/heap/cppgc-js/cpp-heap.cc index b3f652c8c9..e71f5d3fe2 100644 --- a/src/heap/cppgc-js/cpp-heap.cc +++ b/src/heap/cppgc-js/cpp-heap.cc @@ -35,6 +35,12 @@ namespace v8 { +// static +std::unique_ptr CppHeap::Create(v8::Platform* platform, + const CppHeapCreateParams& params) { + return std::make_unique(platform, params.custom_spaces); +} + cppgc::AllocationHandle& CppHeap::GetAllocationHandle() { return internal::CppHeap::From(this)->object_allocator(); } @@ -63,8 +69,7 @@ namespace { class CppgcPlatformAdapter final : public cppgc::Platform { public: - explicit CppgcPlatformAdapter(v8::Isolate* isolate) - : platform_(V8::GetCurrentPlatform()), isolate_(isolate) {} + explicit CppgcPlatformAdapter(v8::Platform* platform) : platform_(platform) {} CppgcPlatformAdapter(const CppgcPlatformAdapter&) = delete; CppgcPlatformAdapter& operator=(const CppgcPlatformAdapter&) = delete; @@ -90,9 +95,11 @@ class CppgcPlatformAdapter final : public cppgc::Platform { return platform_->GetTracingController(); } + void SetIsolate(v8::Isolate* isolate) { isolate_ = isolate; } + private: v8::Platform* platform_; - v8::Isolate* isolate_; + v8::Isolate* isolate_ = nullptr; }; class UnifiedHeapConcurrentMarker @@ -169,38 +176,67 @@ void UnifiedHeapMarker::AddObject(void* object) { } // namespace CppHeap::CppHeap( - v8::Isolate* isolate, + v8::Platform* platform, const std::vector>& custom_spaces, std::unique_ptr metric_recorder) - : cppgc::internal::HeapBase(std::make_shared(isolate), - custom_spaces, - cppgc::internal::HeapBase::StackSupport:: - kSupportsConservativeStackScan, - std::move(metric_recorder)), - isolate_(*reinterpret_cast(isolate)) { - if (isolate_.heap_profiler()) { - isolate_.heap_profiler()->AddBuildEmbedderGraphCallback( - &CppGraphBuilder::Run, this); - } + : cppgc::internal::HeapBase( + std::make_shared(platform), custom_spaces, + cppgc::internal::HeapBase::StackSupport:: + kSupportsConservativeStackScan, + std::move(metric_recorder)) { + // Enter no GC scope. `AttachIsolate()` removes this and allows triggering + // garbage collections. + no_gc_scope_++; stats_collector()->RegisterObserver(this); } CppHeap::~CppHeap() { - if (isolate_.heap_profiler()) { - isolate_.heap_profiler()->RemoveBuildEmbedderGraphCallback( - &CppGraphBuilder::Run, this); + if (isolate_) { + isolate_->heap()->DetachCppHeap(); } } void CppHeap::Terminate() { - FinalizeIncrementalGarbageCollectionIfNeeded( - cppgc::Heap::StackState::kNoHeapPointers); - // Any future garbage collections will ignore the V8->C++ references. - isolate()->SetEmbedderHeapTracer(nullptr); + // Must not be attached to a heap when invoking termination GCs. + CHECK(!isolate_); // Gracefully terminate the C++ heap invoking destructors. HeapBase::Terminate(); } +void CppHeap::AttachIsolate(Isolate* isolate) { + CHECK_NULL(isolate_); + isolate_ = isolate; + static_cast(platform()) + ->SetIsolate(reinterpret_cast(isolate_)); + if (isolate_->heap_profiler()) { + isolate_->heap_profiler()->AddBuildEmbedderGraphCallback( + &CppGraphBuilder::Run, this); + } + isolate_->heap()->SetEmbedderHeapTracer(this); + no_gc_scope_--; +} + +void CppHeap::DetachIsolate() { + // TODO(chromium:1056170): Investigate whether this can be enforced with a + // CHECK across all relevant embedders and setups. + if (!isolate_) return; + + // Delegate to existing EmbedderHeapTracer API to finish any ongoing garbage + // collection. + FinalizeTracing(); + sweeper_.FinishIfRunning(); + + if (isolate_->heap_profiler()) { + isolate_->heap_profiler()->RemoveBuildEmbedderGraphCallback( + &CppGraphBuilder::Run, this); + } + isolate_ = nullptr; + // Any future garbage collections will ignore the V8->C++ references. + isolate()->SetEmbedderHeapTracer(nullptr); + // Enter no GC scope. + no_gc_scope_++; +} + void CppHeap::RegisterV8References( const std::vector >& embedder_fields) { DCHECK(marker_); @@ -231,7 +267,7 @@ void CppHeap::TracePrologue(TraceFlags flags) { } marker_ = cppgc::internal::MarkerFactory::CreateAndStartMarking( - *isolate_.heap(), AsBase(), platform_.get(), marking_config); + *isolate_->heap(), AsBase(), platform_.get(), marking_config); marking_done_ = false; } diff --git a/src/heap/cppgc-js/cpp-heap.h b/src/heap/cppgc-js/cpp-heap.h index 074ac600f1..db085d553e 100644 --- a/src/heap/cppgc-js/cpp-heap.h +++ b/src/heap/cppgc-js/cpp-heap.h @@ -32,7 +32,7 @@ class V8_EXPORT_PRIVATE CppHeap final } CppHeap( - v8::Isolate* isolate, + v8::Platform* platform, const std::vector>& custom_spaces, std::unique_ptr metric_recorder = nullptr); @@ -44,6 +44,9 @@ class V8_EXPORT_PRIVATE CppHeap final HeapBase& AsBase() { return *this; } const HeapBase& AsBase() const { return *this; } + void AttachIsolate(Isolate* isolate); + void DetachIsolate(); + void Terminate(); // v8::EmbedderHeapTracer interface. @@ -69,7 +72,7 @@ class V8_EXPORT_PRIVATE CppHeap final void ReportBufferedAllocationSizeIfPossible(); - Isolate& isolate_; + Isolate* isolate_ = nullptr; bool marking_done_ = false; // Buffered allocated bytes. Reporting allocated bytes to V8 can trigger a GC diff --git a/src/heap/cppgc/heap-base.cc b/src/heap/cppgc/heap-base.cc index 9a83246718..6c25d5eaeb 100644 --- a/src/heap/cppgc/heap-base.cc +++ b/src/heap/cppgc/heap-base.cc @@ -105,7 +105,6 @@ void HeapBase::AdvanceIncrementalGarbageCollectionOnAllocationIfNeeded() { void HeapBase::Terminate() { DCHECK(!IsMarking()); - DCHECK(!in_no_gc_scope()); CHECK(!in_disallow_gc_scope()); sweeper().FinishIfRunning(); diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 80c0aacd19..23b84296be 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -4732,12 +4732,6 @@ void Heap::ConfigureHeap(const v8::ResourceConstraints& constraints) { configured_ = true; } -void Heap::ConfigureCppHeap(std::shared_ptr params) { - cpp_heap_ = std::make_unique( - reinterpret_cast(isolate()), params->custom_spaces); - SetEmbedderHeapTracer(CppHeap::From(cpp_heap_.get())); -} - void Heap::AddToRingBuffer(const char* string) { size_t first_part = std::min(strlen(string), kTraceRingBufferSize - ring_buffer_end_); @@ -5379,6 +5373,8 @@ void Heap::NotifyOldGenerationExpansion(AllocationSpace space, void Heap::SetEmbedderHeapTracer(EmbedderHeapTracer* tracer) { DCHECK_EQ(gc_state(), HeapState::NOT_IN_GC); + // Setting a tracer is only supported when CppHeap is not used. + DCHECK_IMPLIES(tracer, !cpp_heap_); local_embedder_heap_tracer()->SetRemoteTracer(tracer); } @@ -5386,6 +5382,16 @@ EmbedderHeapTracer* Heap::GetEmbedderHeapTracer() const { return local_embedder_heap_tracer()->remote_tracer(); } +void Heap::AttachCppHeap(v8::CppHeap* cpp_heap) { + CppHeap::From(cpp_heap)->AttachIsolate(isolate()); + cpp_heap_ = cpp_heap; +} + +void Heap::DetachCppHeap() { + CppHeap::From(cpp_heap_)->DetachIsolate(); + cpp_heap_ = nullptr; +} + EmbedderHeapTracer::TraceFlags Heap::flags_for_embedder_tracer() const { if (is_current_gc_forced()) { return EmbedderHeapTracer::TraceFlags::kForced; @@ -5512,7 +5518,10 @@ void Heap::TearDown() { dead_object_stats_.reset(); local_embedder_heap_tracer_.reset(); - cpp_heap_.reset(); + if (cpp_heap_) { + CppHeap::From(cpp_heap_)->DetachIsolate(); + cpp_heap_ = nullptr; + } external_string_table_.TearDown(); diff --git a/src/heap/heap.h b/src/heap/heap.h index a76f816aac..2c14dcc9df 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -1137,10 +1137,10 @@ class Heap { // Unified heap (C++) support. =============================================== // =========================================================================== - V8_EXPORT_PRIVATE void ConfigureCppHeap( - std::shared_ptr params); + V8_EXPORT_PRIVATE void AttachCppHeap(v8::CppHeap* cpp_heap); + V8_EXPORT_PRIVATE void DetachCppHeap(); - v8::CppHeap* cpp_heap() const { return cpp_heap_.get(); } + v8::CppHeap* cpp_heap() const { return cpp_heap_; } // =========================================================================== // External string table API. ================================================ @@ -2236,7 +2236,9 @@ class Heap { std::unique_ptr stress_concurrent_allocation_observer_; std::unique_ptr local_embedder_heap_tracer_; std::unique_ptr marking_barrier_; - std::unique_ptr cpp_heap_; + + // The embedder owns the C++ heap. + v8::CppHeap* cpp_heap_ = nullptr; StrongRootsEntry* strong_roots_head_ = nullptr; base::Mutex strong_roots_mutex_; diff --git a/test/unittests/heap/unified-heap-unittest.cc b/test/unittests/heap/unified-heap-unittest.cc index 73d7279015..bab246287d 100644 --- a/test/unittests/heap/unified-heap-unittest.cc +++ b/test/unittests/heap/unified-heap-unittest.cc @@ -10,6 +10,7 @@ #include "include/v8.h" #include "src/api/api-inl.h" #include "src/heap/cppgc-js/cpp-heap.h" +#include "src/heap/cppgc/sweeper.h" #include "src/objects/objects-inl.h" #include "test/unittests/heap/heap-utils.h" #include "test/unittests/heap/unified-heap-utils.h" @@ -39,6 +40,8 @@ class Wrappable final : public cppgc::GarbageCollected { size_t Wrappable::destructor_callcount = 0; +using UnifiedHeapDetachedTest = TestWithHeapInternals; + } // namespace TEST_F(UnifiedHeapTest, OnlyGC) { CollectGarbageWithEmbedderStack(); } @@ -119,5 +122,29 @@ TEST_F(UnifiedHeapTest, WriteBarrierCppToV8Reference) { wrappable->wrapper()->GetAlignedPointerFromInternalField(1)); } +TEST_F(UnifiedHeapDetachedTest, AllocationBeforeConfigureHeap) { + auto heap = + v8::CppHeap::Create(V8::GetCurrentPlatform(), CppHeapCreateParams{}); + auto* object = + cppgc::MakeGarbageCollected(heap->GetAllocationHandle()); + cppgc::WeakPersistent weak_holder{object}; + + auto& js_heap = *isolate()->heap(); + js_heap.AttachCppHeap(heap.get()); + auto& cpp_heap = *CppHeap::From(isolate()->heap()->cpp_heap()); + { + CollectGarbage(OLD_SPACE); + cpp_heap.AsBase().sweeper().FinishIfRunning(); + EXPECT_TRUE(weak_holder); + } + { + js_heap.SetEmbedderStackStateForNextFinalization( + EmbedderHeapTracer::EmbedderStackState::kNoHeapPointers); + CollectGarbage(OLD_SPACE); + cpp_heap.AsBase().sweeper().FinishIfRunning(); + EXPECT_FALSE(weak_holder); + } +} + } // namespace internal } // namespace v8 diff --git a/test/unittests/heap/unified-heap-utils.cc b/test/unittests/heap/unified-heap-utils.cc index 0730e1e813..bb3ede38f4 100644 --- a/test/unittests/heap/unified-heap-utils.cc +++ b/test/unittests/heap/unified-heap-utils.cc @@ -14,8 +14,10 @@ namespace v8 { namespace internal { -UnifiedHeapTest::UnifiedHeapTest() { - isolate()->heap()->ConfigureCppHeap(std::make_unique()); +UnifiedHeapTest::UnifiedHeapTest() + : cpp_heap_(v8::CppHeap::Create(V8::GetCurrentPlatform(), + CppHeapCreateParams{})) { + isolate()->heap()->AttachCppHeap(cpp_heap_.get()); } void UnifiedHeapTest::CollectGarbageWithEmbedderStack( diff --git a/test/unittests/heap/unified-heap-utils.h b/test/unittests/heap/unified-heap-utils.h index 3a38185d52..0fa0796e10 100644 --- a/test/unittests/heap/unified-heap-utils.h +++ b/test/unittests/heap/unified-heap-utils.h @@ -10,6 +10,9 @@ #include "test/unittests/heap/heap-utils.h" namespace v8 { + +class CppHeap; + namespace internal { class CppHeap; @@ -27,6 +30,9 @@ class UnifiedHeapTest : public TestWithHeapInternals { CppHeap& cpp_heap() const; cppgc::AllocationHandle& allocation_handle(); + + private: + std::unique_ptr cpp_heap_; }; class WrapperHelper {