diff --git a/src/execution/isolate.cc b/src/execution/isolate.cc index fd2833a732..61a45b439d 100644 --- a/src/execution/isolate.cc +++ b/src/execution/isolate.cc @@ -4218,7 +4218,7 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data, // Lock clients_mutex_ in order to prevent shared GCs from other clients // during deserialization. - base::Optional clients_guard; + base::Optional clients_guard; if (Isolate* isolate = shared_isolate_ ? shared_isolate_ : attach_to_shared_space_isolate) { diff --git a/src/heap/parked-scope.h b/src/heap/parked-scope.h index c3520e742c..de92c32bc9 100644 --- a/src/heap/parked-scope.h +++ b/src/heap/parked-scope.h @@ -70,6 +70,32 @@ class V8_NODISCARD ParkedMutexGuard { base::Mutex* mutex_; }; +// Scope that automatically parks the thread while blocking on the given +// base::RecursiveMutex. +class V8_NODISCARD ParkedRecursiveMutexGuard { + public: + explicit ParkedRecursiveMutexGuard(LocalIsolate* local_isolate, + base::RecursiveMutex* mutex) + : ParkedRecursiveMutexGuard(local_isolate->heap(), mutex) {} + explicit ParkedRecursiveMutexGuard(LocalHeap* local_heap, + base::RecursiveMutex* mutex) + : mutex_(mutex) { + DCHECK(AllowGarbageCollection::IsAllowed()); + if (!mutex_->TryLock()) { + ParkedScope scope(local_heap); + mutex_->Lock(); + } + } + + ParkedRecursiveMutexGuard(const ParkedMutexGuard&) = delete; + ParkedRecursiveMutexGuard& operator=(const ParkedMutexGuard&) = delete; + + ~ParkedRecursiveMutexGuard() { mutex_->Unlock(); } + + private: + base::RecursiveMutex* mutex_; +}; + template class V8_NODISCARD ParkedSharedMutexGuardIf final { diff --git a/src/heap/safepoint.cc b/src/heap/safepoint.cc index 70fba727c1..615666df3c 100644 --- a/src/heap/safepoint.cc +++ b/src/heap/safepoint.cc @@ -26,8 +26,7 @@ namespace v8 { namespace internal { -IsolateSafepoint::IsolateSafepoint(Heap* heap) - : heap_(heap), local_heaps_head_(nullptr), active_safepoint_scopes_(0) {} +IsolateSafepoint::IsolateSafepoint(Heap* heap) : heap_(heap) {} void IsolateSafepoint::EnterLocalSafepointScope() { // Safepoints need to be initiated on some main thread. @@ -317,7 +316,8 @@ void GlobalSafepoint::RemoveClient(Isolate* client) { // A shared heap may have already acquired the client mutex to perform a // shared GC. We need to park the Isolate here to allow for a shared GC. IgnoreLocalGCRequests ignore_gc_requests(client->heap()); - ParkedMutexGuard guard(client->main_thread_local_heap(), &clients_mutex_); + ParkedRecursiveMutexGuard guard(client->main_thread_local_heap(), + &clients_mutex_); if (client->global_safepoint_next_client_isolate_) { client->global_safepoint_next_client_isolate_ @@ -356,6 +356,8 @@ void GlobalSafepoint::EnterGlobalSafepointScope(Isolate* initiator) { clients_mutex_.Lock(); } + if (++active_safepoint_scopes_ > 1) return; + TimedHistogramScope timer( initiator->counters()->gc_time_to_global_safepoint()); TRACE_GC(initiator->heap()->tracer(), @@ -404,14 +406,19 @@ void GlobalSafepoint::EnterGlobalSafepointScope(Isolate* initiator) { } void GlobalSafepoint::LeaveGlobalSafepointScope(Isolate* initiator) { - if (shared_heap_isolate_->is_shared()) { - shared_heap_isolate_->heap()->safepoint()->local_heaps_mutex_.Unlock(); - } + clients_mutex_.AssertHeld(); + DCHECK_GT(active_safepoint_scopes_, 0); - IterateClientIsolates([initiator](Isolate* client) { - Heap* client_heap = client->heap(); - client_heap->safepoint()->LeaveGlobalSafepointScope(initiator); - }); + if (--active_safepoint_scopes_ == 0) { + if (shared_heap_isolate_->is_shared()) { + shared_heap_isolate_->heap()->safepoint()->local_heaps_mutex_.Unlock(); + } + + IterateClientIsolates([initiator](Isolate* client) { + Heap* client_heap = client->heap(); + client_heap->safepoint()->LeaveGlobalSafepointScope(initiator); + }); + } clients_mutex_.Unlock(); } diff --git a/src/heap/safepoint.h b/src/heap/safepoint.h index 7e935889b7..23bc905580 100644 --- a/src/heap/safepoint.h +++ b/src/heap/safepoint.h @@ -141,9 +141,9 @@ class IsolateSafepoint final { // Mutex is used both for safepointing and adding/removing threads. A // RecursiveMutex is needed since we need to support nested SafepointScopes. base::RecursiveMutex local_heaps_mutex_; - LocalHeap* local_heaps_head_; + LocalHeap* local_heaps_head_ = nullptr; - int active_safepoint_scopes_; + int active_safepoint_scopes_ = 0; friend class GlobalSafepoint; friend class GlobalSafepointScope; @@ -187,8 +187,11 @@ class GlobalSafepoint final { void LeaveGlobalSafepointScope(Isolate* initiator); Isolate* const shared_heap_isolate_; - base::Mutex clients_mutex_; + // RecursiveMutex is needed since we need to support nested + // GlobalSafepointScopes. + base::RecursiveMutex clients_mutex_; Isolate* clients_head_ = nullptr; + int active_safepoint_scopes_ = 0; friend class GlobalSafepointScope; friend class Isolate;