[heap] Fix marking in per-context mode

Per-context mode marking segregates worklists per context. Upon doing
so, Worklist::Local's move ctor was invoked which cleared the back
pointer to worklist. This break switching to that context which
happens in rare secnarios.

Rework Local marking worklists avoiding the move ctor which is also
removed.

Bug: chromium:1355545
Change-Id: If0e8c7f08df564b2a1e27e4a3fc5a6a40e46ee46
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3845630
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82636}

src/heap/base/worklist.h

@@ -307,8 +307,10 @@ class Worklist<EntryType, MinSegmentSize>::Local final {
   explicit Local(Worklist<EntryType, MinSegmentSize>* worklist);
   ~Local();
 
-  Local(Local&&) V8_NOEXCEPT;
-  Local& operator=(Local&&) V8_NOEXCEPT;
+  // Moving needs to specify whether the `worklist_` pointer is preserved or
+  // not.
+  Local(Local&&) V8_NOEXCEPT = delete;
+  Local& operator=(Local&&) V8_NOEXCEPT = delete;
 
   // Having multiple copies of the same local view may be unsafe.
   Local(const Local&) = delete;
@@ -382,35 +384,6 @@ Worklist<EntryType, MinSegmentSize>::Local::~Local() {
   DeleteSegment(pop_segment_);
 }
 
-template <typename EntryType, uint16_t MinSegmentSize>
-Worklist<EntryType, MinSegmentSize>::Local::Local(
-    Worklist<EntryType, MinSegmentSize>::Local&& other) V8_NOEXCEPT {
-  worklist_ = other.worklist_;
-  push_segment_ = other.push_segment_;
-  pop_segment_ = other.pop_segment_;
-  other.worklist_ = nullptr;
-  other.push_segment_ = nullptr;
-  other.pop_segment_ = nullptr;
-}
-
-template <typename EntryType, uint16_t MinSegmentSize>
-typename Worklist<EntryType, MinSegmentSize>::Local&
-Worklist<EntryType, MinSegmentSize>::Local::operator=(
-    Worklist<EntryType, MinSegmentSize>::Local&& other) V8_NOEXCEPT {
-  if (this != &other) {
-    DCHECK_NULL(worklist_);
-    DCHECK_NULL(push_segment_);
-    DCHECK_NULL(pop_segment_);
-    worklist_ = other.worklist_;
-    push_segment_ = other.push_segment_;
-    pop_segment_ = other.pop_segment_;
-    other.worklist_ = nullptr;
-    other.push_segment_ = nullptr;
-    other.pop_segment_ = nullptr;
-  }
-  return *this;
-}
-
 template <typename EntryType, uint16_t MinSegmentSize>
 void Worklist<EntryType, MinSegmentSize>::Local::Push(EntryType entry) {
   if (V8_UNLIKELY(push_segment_->IsFull())) {

src/heap/concurrent-marking.cc

@@ -779,6 +779,7 @@ void ConcurrentMarking::Run(JobDelegate* delegate,
 
 size_t ConcurrentMarking::GetMaxConcurrency(size_t worker_count) {
   size_t marking_items = marking_worklists_->shared()->Size();
+  marking_items += marking_worklists_->other()->Size();
   for (auto& worklist : marking_worklists_->context_worklists())
     marking_items += worklist.worklist->Size();
   return std::min<size_t>(

src/heap/marking-worklist-inl.h

@@ -20,19 +20,15 @@ void MarkingWorklists::Update(Callback callback) {
   on_hold_.Update(callback);
   wrapper_.Update(callback);
   other_.Update(callback);
-  for (auto cw : context_worklists_) {
-    if (cw.context == kSharedContext || cw.context == kOtherContext) {
-      // These contexts were updated above.
-      continue;
-    }
+  for (auto& cw : context_worklists_) {
     cw.worklist->Update(callback);
   }
 }
 
-void MarkingWorklists::Local::Push(HeapObject object) { active_.Push(object); }
+void MarkingWorklists::Local::Push(HeapObject object) { active_->Push(object); }
 
 bool MarkingWorklists::Local::Pop(HeapObject* object) {
-  if (active_.Pop(object)) return true;
+  if (active_->Pop(object)) return true;
   if (!is_per_context_mode_) return false;
   // The active worklist is empty. Find any other non-empty worklist and
   // switch the active worklist to it.
@@ -78,17 +74,9 @@ Address MarkingWorklists::Local::SwitchToContext(Address context) {
   return SwitchToContextSlow(context);
 }
 
-Address MarkingWorklists::Local::SwitchToShared() {
-  return SwitchToContext(kSharedContext);
-}
-
-void MarkingWorklists::Local::SwitchToContext(
+void MarkingWorklists::Local::SwitchToContextImpl(
     Address context, MarkingWorklist::Local* worklist) {
-  // Save the current worklist.
-  *active_owner_ = std::move(active_);
-  // Switch to the new worklist.
-  active_owner_ = worklist;
-  active_ = std::move(*worklist);
+  active_ = worklist;
   active_context_ = context;
 }
 

src/heap/marking-worklist.cc

@@ -26,11 +26,7 @@ void MarkingWorklists::Clear() {
   on_hold_.Clear();
   wrapper_.Clear();
   other_.Clear();
-  for (auto cw : context_worklists_) {
-    if (cw.context == kSharedContext || cw.context == kOtherContext) {
-      // These contexts were cleared above.
-      continue;
-    }
+  for (auto& cw : context_worklists_) {
     cw.worklist->Clear();
   }
   ReleaseContextWorklists();
@@ -43,24 +39,17 @@ void MarkingWorklists::Print() {
 
 void MarkingWorklists::CreateContextWorklists(
     const std::vector<Address>& contexts) {
-  DCHECK(worklists_.empty());
   DCHECK(context_worklists_.empty());
   if (contexts.empty()) return;
-  worklists_.reserve(contexts.size());
-  context_worklists_.reserve(contexts.size() + 2);
-  context_worklists_.push_back({kSharedContext, &shared_});
-  context_worklists_.push_back({kOtherContext, &other_});
+
+  context_worklists_.reserve(contexts.size());
   for (Address context : contexts) {
-    MarkingWorklist* worklist = new MarkingWorklist();
-    worklists_.push_back(std::unique_ptr<MarkingWorklist>(worklist));
-    context_worklists_.push_back({context, worklist});
+    context_worklists_.push_back(
+        {context, std::make_unique<MarkingWorklist>()});
   }
 }
 
-void MarkingWorklists::ReleaseContextWorklists() {
-  context_worklists_.clear();
-  worklists_.clear();
-}
+void MarkingWorklists::ReleaseContextWorklists() { context_worklists_.clear(); }
 
 void MarkingWorklists::PrintWorklist(const char* worklist_name,
                                      MarkingWorklist* worklist) {
@@ -93,34 +82,44 @@ constexpr Address MarkingWorklists::Local::kSharedContext;
 constexpr Address MarkingWorklists::Local::kOtherContext;
 constexpr std::nullptr_t MarkingWorklists::Local::kNoCppMarkingState;
 
+namespace {
+
+inline std::unordered_map<Address, std::unique_ptr<MarkingWorklist::Local>>
+GetLocalPerContextMarkingWorklists(bool is_per_context_mode,
+                                   MarkingWorklists* global) {
+  if (!is_per_context_mode) return {};
+
+  std::unordered_map<Address, std::unique_ptr<MarkingWorklist::Local>>
+      worklist_by_context;
+  worklist_by_context.reserve(global->context_worklists().size());
+  for (auto& cw : global->context_worklists()) {
+    worklist_by_context[cw.context] =
+        std::make_unique<MarkingWorklist::Local>(cw.worklist.get());
+  }
+  return worklist_by_context;
+}
+
+}  // namespace
+
 MarkingWorklists::Local::Local(
     MarkingWorklists* global,
     std::unique_ptr<CppMarkingState> cpp_marking_state)
-    : on_hold_(global->on_hold()),
+    : active_(&shared_),
+      shared_(global->shared()),
+      on_hold_(global->on_hold()),
       wrapper_(global->wrapper()),
-      cpp_marking_state_(std::move(cpp_marking_state)) {
-  if (global->context_worklists().empty()) {
-    MarkingWorklist::Local shared(global->shared());
-    active_ = std::move(shared);
-    active_context_ = kSharedContext;
-    active_owner_ = nullptr;
-  } else {
-    is_per_context_mode_ = true;
-    worklist_by_context_.reserve(global->context_worklists().size());
-    for (auto& cw : global->context_worklists()) {
-      worklist_by_context_[cw.context] =
-          std::make_unique<MarkingWorklist::Local>(cw.worklist);
-    }
-    active_owner_ = worklist_by_context_[kSharedContext].get();
-    active_ = std::move(*active_owner_);
-    active_context_ = kSharedContext;
-  }
-}
+      active_context_(kSharedContext),
+      is_per_context_mode_(!global->context_worklists().empty()),
+      worklist_by_context_(
+          GetLocalPerContextMarkingWorklists(is_per_context_mode_, global)),
+      other_(global->other()),
+      cpp_marking_state_(std::move(cpp_marking_state)) {}
 
 void MarkingWorklists::Local::Publish() {
-  active_.Publish();
+  shared_.Publish();
   on_hold_.Publish();
   wrapper_.Publish();
+  other_.Publish();
   if (is_per_context_mode_) {
     for (auto& cw : worklist_by_context_) {
       if (cw.first != active_context_) {
@@ -134,17 +133,21 @@ void MarkingWorklists::Local::Publish() {
 bool MarkingWorklists::Local::IsEmpty() {
   // This function checks the on_hold_ worklist, so it works only for the main
   // thread.
-  if (!active_.IsLocalEmpty() || !on_hold_.IsLocalEmpty() ||
-      !active_.IsGlobalEmpty() || !on_hold_.IsGlobalEmpty()) {
+  if (!active_->IsLocalEmpty() || !on_hold_.IsLocalEmpty() ||
+      !active_->IsGlobalEmpty() || !on_hold_.IsGlobalEmpty()) {
     return false;
   }
   if (!is_per_context_mode_) {
     return true;
   }
+  if (!shared_.IsLocalEmpty() || !other_.IsLocalEmpty() ||
+      !shared_.IsGlobalEmpty() || !other_.IsGlobalEmpty()) {
+    return false;
+  }
   for (auto& cw : worklist_by_context_) {
     if (cw.first != active_context_ &&
         !(cw.second->IsLocalEmpty() && cw.second->IsGlobalEmpty())) {
-      SwitchToContext(cw.first, cw.second.get());
+      SwitchToContextImpl(cw.first, cw.second.get());
       return false;
     }
   }
@@ -160,38 +163,31 @@ bool MarkingWorklists::Local::IsWrapperEmpty() const {
 }
 
 void MarkingWorklists::Local::ShareWork() {
-  if (!active_.IsLocalEmpty() && active_.IsGlobalEmpty()) {
-    active_.Publish();
+  if (!active_->IsLocalEmpty() && active_->IsGlobalEmpty()) {
+    active_->Publish();
   }
   if (is_per_context_mode_ && active_context_ != kSharedContext) {
-    MarkingWorklist::Local* shared = worklist_by_context_[kSharedContext].get();
-    if (!shared->IsLocalEmpty() && shared->IsGlobalEmpty()) {
-      shared->Publish();
+    if (!shared_.IsLocalEmpty() && shared_.IsGlobalEmpty()) {
+      shared_.Publish();
     }
   }
 }
 
-void MarkingWorklists::Local::MergeOnHold() {
-  MarkingWorklist::Local* shared =
-      active_context_ == kSharedContext
-          ? &active_
-          : worklist_by_context_[kSharedContext].get();
-  shared->Merge(&on_hold_);
-}
+void MarkingWorklists::Local::MergeOnHold() { shared_.Merge(&on_hold_); }
 
 bool MarkingWorklists::Local::PopContext(HeapObject* object) {
   DCHECK(is_per_context_mode_);
   // As an optimization we first check only the local segments to avoid locks.
   for (auto& cw : worklist_by_context_) {
     if (cw.first != active_context_ && !cw.second->IsLocalEmpty()) {
-      SwitchToContext(cw.first, cw.second.get());
-      return active_.Pop(object);
+      SwitchToContextImpl(cw.first, cw.second.get());
+      return active_->Pop(object);
     }
   }
   // All local segments are empty. Check global segments.
   for (auto& cw : worklist_by_context_) {
     if (cw.first != active_context_ && cw.second->Pop(object)) {
-      SwitchToContext(cw.first, cw.second.get());
+      SwitchToContextImpl(cw.first, cw.second.get());
       return true;
     }
   }
@@ -203,14 +199,24 @@ bool MarkingWorklists::Local::PopContext(HeapObject* object) {
 Address MarkingWorklists::Local::SwitchToContextSlow(Address context) {
   const auto& it = worklist_by_context_.find(context);
   if (V8_UNLIKELY(it == worklist_by_context_.end())) {
-    // This context was created during marking or is not being measured,
-    // so we don't have a specific worklist for it.
-    SwitchToContext(kOtherContext, worklist_by_context_[kOtherContext].get());
+    // The context passed is not an actual context:
+    // - Shared context that should use the explicit worklist.
+    // - This context was created during marking and should use the other
+    // bucket.
+    if (context == kSharedContext) {
+      SwitchToContextImpl(kSharedContext, &shared_);
+    } else {
+      SwitchToContextImpl(kOtherContext, &other_);
+    }
   } else {
-    SwitchToContext(it->first, it->second.get());
+    SwitchToContextImpl(it->first, it->second.get());
   }
   return active_context_;
 }
 
+Address MarkingWorklists::Local::SwitchToSharedForTesting() {
+  return SwitchToContext(kSharedContext);
+}
+
 }  // namespace internal
 }  // namespace v8

src/heap/marking-worklist.h

@@ -62,7 +62,7 @@ using WrapperTracingWorklist = ::heap::base::Worklist<HeapObject, 16>;
 // a stable across Scavenges and stay valid throughout the marking phase.
 struct ContextWorklistPair {
   Address context;
-  MarkingWorklist* worklist;
+  std::unique_ptr<MarkingWorklist> worklist;
 };
 
 // A helper class that owns all global marking worklists.
@@ -73,8 +73,8 @@ class V8_EXPORT_PRIVATE MarkingWorklists final {
   // - kSharedContext is for objects that are not attributed to any context.
   // - kOtherContext is for objects that are attributed to contexts that are
   //   not being measured.
-  static const Address kSharedContext = 0;
-  static const Address kOtherContext = 8;
+  static constexpr Address kSharedContext = 0;
+  static constexpr Address kOtherContext = 8;
 
   MarkingWorklists() = default;
 
@@ -90,6 +90,7 @@ class V8_EXPORT_PRIVATE MarkingWorklists final {
 
   MarkingWorklist* shared() { return &shared_; }
   MarkingWorklist* on_hold() { return &on_hold_; }
+  MarkingWorklist* other() { return &other_; }
   WrapperTracingWorklist* wrapper() { return &wrapper_; }
 
   // A list of (context, worklist) pairs that was set up at the start of
@@ -112,6 +113,7 @@ class V8_EXPORT_PRIVATE MarkingWorklists final {
   void PrintWorklist(const char* worklist_name, MarkingWorklist* worklist);
 
   // Worklist used for most objects.
+  // TODO(mlippautz): Rename to "default".
   MarkingWorklist shared_;
 
   // Concurrent marking uses this worklist to bail out of marking objects
@@ -125,11 +127,8 @@ class V8_EXPORT_PRIVATE MarkingWorklists final {
   // transitive closure.
   WrapperTracingWorklist wrapper_;
 
-  // Per-context worklists.
+  // Per-context worklists. Objects are in the `shared_` worklist by default.
   std::vector<ContextWorklistPair> context_worklists_;
-  // This is used only for lifetime management of the per-context worklists.
-  std::vector<std::unique_ptr<MarkingWorklist>> worklists_;
-
   // Worklist used for objects that are attributed to contexts that are
   // not being measured.
   MarkingWorklist other_;
@@ -187,27 +186,31 @@ class V8_EXPORT_PRIVATE MarkingWorklists::Local final {
   // Returns the context of the active worklist.
   Address Context() const { return active_context_; }
   inline Address SwitchToContext(Address context);
-  inline Address SwitchToShared();
   bool IsPerContextMode() const { return is_per_context_mode_; }
 
   CppMarkingState* cpp_marking_state() const {
     return cpp_marking_state_.get();
   }
 
+  Address SwitchToSharedForTesting();
+
  private:
+  inline void SwitchToContextImpl(Address context,
+                                  MarkingWorklist::Local* worklist);
+
   bool PopContext(HeapObject* object);
   Address SwitchToContextSlow(Address context);
-  inline void SwitchToContext(Address context,
-                              MarkingWorklist::Local* worklist);
+
+  // Points to either `shared_`, `other_` or to a per-context worklist.
+  MarkingWorklist::Local* active_;
+  MarkingWorklist::Local shared_;
   MarkingWorklist::Local on_hold_;
   WrapperTracingWorklist::Local wrapper_;
-  MarkingWorklist::Local active_;
   Address active_context_;
-  MarkingWorklist::Local* active_owner_;
-  bool is_per_context_mode_ = false;
-  std::unordered_map<Address, std::unique_ptr<MarkingWorklist::Local>>
+  const bool is_per_context_mode_;
+  const std::unordered_map<Address, std::unique_ptr<MarkingWorklist::Local>>
       worklist_by_context_;
-
+  MarkingWorklist::Local other_;
   std::unique_ptr<CppMarkingState> cpp_marking_state_;
 };
 

test/unittests/heap/marking-worklist-unittest.cc

@@ -87,7 +87,7 @@ TEST_F(MarkingWorklistTest, ContextWorklistsPushPop) {
       ReadOnlyRoots(i_isolate()->heap()).undefined_value();
   worklists.SwitchToContext(context);
   worklists.Push(pushed_object);
-  worklists.SwitchToShared();
+  worklists.SwitchToSharedForTesting();
   HeapObject popped_object;
   EXPECT_TRUE(worklists.Pop(&popped_object));
   EXPECT_EQ(popped_object, pushed_object);
@@ -104,7 +104,7 @@ TEST_F(MarkingWorklistTest, ContextWorklistsEmpty) {
   worklists.SwitchToContext(context);
   worklists.Push(pushed_object);
   EXPECT_FALSE(worklists.IsEmpty());
-  worklists.SwitchToShared();
+  worklists.SwitchToSharedForTesting();
   EXPECT_FALSE(worklists.IsEmpty());
   HeapObject popped_object;
   EXPECT_TRUE(worklists.Pop(&popped_object));