From 700dfe36c316eecbe127bb639552a068e0b2131c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Inf=C3=BChr?= Date: Thu, 17 Nov 2022 09:31:04 +0100 Subject: [PATCH] [heap] Rework C++ shared marking barrier checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the checks in the C++ marking barrier that deal with shared objects. The checks we now use here are the same we will be using for the JS barrier in RecordWrite (see https://crrev.com/c/4020176). This CL also reworks WriteWithoutHost, the barrier used for traced handles. It doesn't use MarkValue anymore since the logic is a bit different to the regular marking barrier on objects. Bug: v8:13267 Change-Id: If23b65ce5f06af99a5cea864ce28a68f8d5b37de Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4031028 Reviewed-by: Michael Lippautz Commit-Queue: Dominik Inführ Cr-Commit-Position: refs/heads/main@{#84317} --- src/heap/marking-barrier-inl.h | 76 ++++++++++++++++++---------------- src/heap/marking-barrier.cc | 16 +++++-- src/heap/marking-barrier.h | 8 ++-- 3 files changed, 59 insertions(+), 41 deletions(-) diff --git a/src/heap/marking-barrier-inl.h b/src/heap/marking-barrier-inl.h index 9c96b61044..854a2f3b28 100644 --- a/src/heap/marking-barrier-inl.h +++ b/src/heap/marking-barrier-inl.h @@ -23,19 +23,52 @@ void MarkingBarrier::MarkValue(HeapObject host, HeapObject value) { // In that case host has only one markbit and the second markbit belongs to // another object. We can detect that case by checking if value is a one word // filler map. - DCHECK_IMPLIES( - !host.is_null(), - !marking_state_.IsImpossible(host) || - value == ReadOnlyRoots(heap_->isolate()).one_pointer_filler_map()); + DCHECK(!marking_state_.IsImpossible(host) || + value == ReadOnlyRoots(heap_->isolate()).one_pointer_filler_map()); - if (V8_UNLIKELY(uses_shared_heap_) && value.InSharedWritableHeap()) { - if (ProcessSharedObject(value)) return; + // When shared heap isn't enabled all objects are local, we can just run the + // local marking barrier. Also from the point-of-view of the shared space + // isolate (= main isolate) also shared objects are considered local. + if (V8_UNLIKELY(uses_shared_heap_) && !is_shared_space_isolate_) { + // Check whether incremental marking is enabled for that object's space. + if (!MemoryChunk::FromHeapObject(host)->IsFlagSet( + BasicMemoryChunk::Flag::INCREMENTAL_MARKING)) { + return; + } + + if (host.InSharedWritableHeap()) { + // Invoking shared marking barrier when storing into shared objects. + MarkValueShared(value); + return; + } else if (value.InSharedWritableHeap()) { + // No marking needed when storing shared objects in local objects. + return; + } } - DCHECK_IMPLIES(value.InSharedWritableHeap(), is_shared_heap_isolate_); + DCHECK_IMPLIES(host.InSharedWritableHeap(), is_shared_space_isolate_); + DCHECK_IMPLIES(value.InSharedWritableHeap(), is_shared_space_isolate_); - if (!is_activated_) return; + DCHECK(is_activated_); + MarkValueLocal(value); +} +void MarkingBarrier::MarkValueShared(HeapObject value) { + // Value is either in read-only space or shared heap. + DCHECK(value.InSharedHeap()); + + // We should only reach this on client isolates (= worker isolates). + DCHECK(v8_flags.shared_space); + DCHECK(!is_shared_space_isolate_); + DCHECK(shared_heap_worklist_.has_value()); + + // Mark shared object and push it onto shared heap worklist. + if (marking_state_.WhiteToGrey(value)) { + shared_heap_worklist_->Push(value); + } +} + +void MarkingBarrier::MarkValueLocal(HeapObject value) { if (is_minor()) { // We do not need to insert into RememberedSet here because the // C++ marking barrier already does this for us. @@ -51,33 +84,6 @@ void MarkingBarrier::MarkValue(HeapObject host, HeapObject value) { } } -bool MarkingBarrier::ProcessSharedObject(HeapObject value) { - if (v8_flags.shared_space) { - if (is_shared_heap_isolate_) { - // The main isolate processes objects in the shared heap as regular local - // objects. - return false; - } else { - // Background threads need to mark shared objects when incremental marking - // in the shared heap is active. - if (shared_heap_worklist_.has_value() && - marking_state_.WhiteToGrey(value)) { - DCHECK(v8_flags.shared_space); - DCHECK(!is_shared_heap_isolate_); - shared_heap_worklist_->Push(value); - } - - // Background threads do not process shared objects as regular local - // objects. - return true; - } - - } else { - // All threads ignore shared objects during incremental marking. - return true; - } -} - template inline void MarkingBarrier::MarkRange(HeapObject host, TSlot start, TSlot end) { auto* isolate = heap_->isolate(); diff --git a/src/heap/marking-barrier.cc b/src/heap/marking-barrier.cc index 2dcfd7891b..da595dd9c3 100644 --- a/src/heap/marking-barrier.cc +++ b/src/heap/marking-barrier.cc @@ -32,7 +32,7 @@ MarkingBarrier::MarkingBarrier(LocalHeap* local_heap) marking_state_(isolate()), is_main_thread_barrier_(local_heap->is_main_thread()), uses_shared_heap_(isolate()->has_shared_heap()), - is_shared_heap_isolate_(isolate()->is_shared_heap_isolate()) {} + is_shared_space_isolate_(isolate()->is_shared_space_isolate()) {} MarkingBarrier::~MarkingBarrier() { DCHECK(typed_slots_map_.empty()); } @@ -53,8 +53,18 @@ void MarkingBarrier::Write(HeapObject host, HeapObjectSlot slot, void MarkingBarrier::WriteWithoutHost(HeapObject value) { DCHECK(is_main_thread_barrier_); - DCHECK(is_activated_ || shared_heap_worklist_.has_value()); - MarkValue(HeapObject(), value); + DCHECK(is_activated_); + + // Without a shared heap and on the shared space isolate (= main isolate) all + // objects are considered local. + if (V8_UNLIKELY(uses_shared_heap_) && !is_shared_space_isolate_) { + // On client isolates (= worker isolates) shared values can be ignored. + if (value.InSharedWritableHeap()) { + return; + } + } + + MarkValueLocal(value); } void MarkingBarrier::Write(Code host, RelocInfo* reloc_info, HeapObject value) { diff --git a/src/heap/marking-barrier.h b/src/heap/marking-barrier.h index 6b585d1770..c9d90bb0d9 100644 --- a/src/heap/marking-barrier.h +++ b/src/heap/marking-barrier.h @@ -56,7 +56,9 @@ class MarkingBarrier { Heap* heap() const { return heap_; } private: - inline bool ProcessSharedObject(HeapObject value); + inline void MarkValueShared(HeapObject value); + inline void MarkValueLocal(HeapObject value); + inline bool WhiteToGreyAndPush(HeapObject value); void RecordRelocSlot(Code host, RelocInfo* rinfo, HeapObject target); @@ -86,9 +88,9 @@ class MarkingBarrier { typed_slots_map_; bool is_compacting_ = false; bool is_activated_ = false; - bool is_main_thread_barrier_; + const bool is_main_thread_barrier_; const bool uses_shared_heap_; - const bool is_shared_heap_isolate_; + const bool is_shared_space_isolate_; MarkingBarrierType marking_barrier_type_; };