diff --git a/src/heap/base/worklist.h b/src/heap/base/worklist.h index 55faa2b113..9f026f8f8b 100644 --- a/src/heap/base/worklist.h +++ b/src/heap/base/worklist.h @@ -307,8 +307,10 @@ class Worklist::Local final { explicit Local(Worklist* 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::Local::~Local() { DeleteSegment(pop_segment_); } -template -Worklist::Local::Local( - Worklist::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 Worklist::Local& -Worklist::Local::operator=( - Worklist::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 void Worklist::Local::Push(EntryType entry) { if (V8_UNLIKELY(push_segment_->IsFull())) { diff --git a/src/heap/concurrent-marking.cc b/src/heap/concurrent-marking.cc index 6eaaeb3698..257a3d78bf 100644 --- a/src/heap/concurrent-marking.cc +++ b/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( diff --git a/src/heap/marking-worklist-inl.h b/src/heap/marking-worklist-inl.h index 8a1551f1a2..6024a3e46a 100644 --- a/src/heap/marking-worklist-inl.h +++ b/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; } diff --git a/src/heap/marking-worklist.cc b/src/heap/marking-worklist.cc index 60e0b44565..38c974c904 100644 --- a/src/heap/marking-worklist.cc +++ b/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
& 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(worklist)); - context_worklists_.push_back({context, worklist}); + context_worklists_.push_back( + {context, std::make_unique()}); } } -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> +GetLocalPerContextMarkingWorklists(bool is_per_context_mode, + MarkingWorklists* global) { + if (!is_per_context_mode) return {}; + + std::unordered_map> + 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(cw.worklist.get()); + } + return worklist_by_context; +} + +} // namespace + MarkingWorklists::Local::Local( MarkingWorklists* global, std::unique_ptr 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(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 diff --git a/src/heap/marking-worklist.h b/src/heap/marking-worklist.h index 95f5aad478..b2c6fc297e 100644 --- a/src/heap/marking-worklist.h +++ b/src/heap/marking-worklist.h @@ -62,7 +62,7 @@ using WrapperTracingWorklist = ::heap::base::Worklist; // a stable across Scavenges and stay valid throughout the marking phase. struct ContextWorklistPair { Address context; - MarkingWorklist* worklist; + std::unique_ptr 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 context_worklists_; - // This is used only for lifetime management of the per-context worklists. - std::vector> 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> + const bool is_per_context_mode_; + const std::unordered_map> worklist_by_context_; - + MarkingWorklist::Local other_; std::unique_ptr cpp_marking_state_; }; diff --git a/test/unittests/heap/marking-worklist-unittest.cc b/test/unittests/heap/marking-worklist-unittest.cc index a48cc6a446..0bd53c2893 100644 --- a/test/unittests/heap/marking-worklist-unittest.cc +++ b/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));