From 05c80cac201a0046de7558137ccac5815bc38a27 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 9 Mar 2020 22:11:22 +0100 Subject: [PATCH] LibJS: Make the GC marking phase cycle-proof Don't visit cells that are already marked. This prevents the marking phase from looping forever when two cells refer to each other. Also do the marking directly from the CellVisitor, removing another unnecessary phase of the collector. :^) --- Libraries/LibJS/Cell.h | 7 ++--- Libraries/LibJS/Heap.cpp | 59 ++++++++++++-------------------------- Libraries/LibJS/Heap.h | 1 - Libraries/LibJS/Object.cpp | 6 ++-- Libraries/LibJS/Object.h | 2 +- 5 files changed, 24 insertions(+), 51 deletions(-) diff --git a/Libraries/LibJS/Cell.h b/Libraries/LibJS/Cell.h index 31347f761b..d4e347a1af 100644 --- a/Libraries/LibJS/Cell.h +++ b/Libraries/LibJS/Cell.h @@ -44,13 +44,10 @@ public: class Visitor { public: - virtual void did_visit(Cell*) = 0; + virtual void visit(Cell*) = 0; }; - virtual void visit_graph(Visitor& visitor) - { - visitor.did_visit(this); - } + virtual void visit_children(Visitor&) {} private: bool m_mark { false }; diff --git a/Libraries/LibJS/Heap.cpp b/Libraries/LibJS/Heap.cpp index bd0ec319b8..d6cd2fedb0 100644 --- a/Libraries/LibJS/Heap.cpp +++ b/Libraries/LibJS/Heap.cpp @@ -63,11 +63,7 @@ void Heap::collect_garbage() { HashTable roots; collect_roots(roots); - - HashTable live_cells; - visit_live_cells(roots, live_cells); - - mark_live_cells(live_cells); + mark_live_cells(roots); sweep_dead_cells(); } @@ -83,48 +79,30 @@ void Heap::collect_roots(HashTable& roots) #endif } -class LivenessVisitor final : public Cell::Visitor { +class MarkingVisitor final : public Cell::Visitor { public: - LivenessVisitor(HashTable& live_cells) - : m_live_cells(live_cells) + MarkingVisitor() {} + + virtual void visit(Cell* cell) { - } - - virtual void did_visit(Cell* cell) override - { - m_live_cells.set(cell); - } - -private: - HashTable& m_live_cells; -}; - -void Heap::visit_live_cells(const HashTable& roots, HashTable& live_cells) -{ - LivenessVisitor visitor(live_cells); - for (auto* root : roots) { - root->visit_graph(visitor); - } - -#ifdef HEAP_DEBUG - dbg() << "visit_live_cells:"; - for (auto* cell : live_cells) { - dbg() << " @ " << cell; - } -#endif -} - -void Heap::mark_live_cells(const HashTable& live_cells) -{ -#ifdef HEAP_DEBUG - dbg() << "mark_live_cells:"; -#endif - for (auto& cell : live_cells) { + if (cell->is_marked()) + return; #ifdef HEAP_DEBUG dbg() << " ! " << cell; #endif cell->set_marked(true); + cell->visit_children(*this); } +}; + +void Heap::mark_live_cells(const HashTable& roots) +{ +#ifdef HEAP_DEBUG + dbg() << "mark_live_cells:"; +#endif + MarkingVisitor visitor; + for (auto* root : roots) + visitor.visit(root); } void Heap::sweep_dead_cells() @@ -147,5 +125,4 @@ void Heap::sweep_dead_cells() }); } } - } diff --git a/Libraries/LibJS/Heap.h b/Libraries/LibJS/Heap.h index 887c35ce48..977089a928 100644 --- a/Libraries/LibJS/Heap.h +++ b/Libraries/LibJS/Heap.h @@ -57,7 +57,6 @@ private: Cell* allocate_cell(size_t); void collect_roots(HashTable&); - void visit_live_cells(const HashTable& roots, HashTable& live_cells); void mark_live_cells(const HashTable& live_cells); void sweep_dead_cells(); diff --git a/Libraries/LibJS/Object.cpp b/Libraries/LibJS/Object.cpp index ae96485d00..53f010681d 100644 --- a/Libraries/LibJS/Object.cpp +++ b/Libraries/LibJS/Object.cpp @@ -48,12 +48,12 @@ void Object::put(String property_name, Value value) m_properties.set(property_name, move(value)); } -void Object::visit_graph(Cell::Visitor& visitor) +void Object::visit_children(Cell::Visitor& visitor) { - Cell::visit_graph(visitor); + Cell::visit_children(visitor); for (auto& it : m_properties) { if (it.value.is_object()) - it.value.as_object()->visit_graph(visitor); + visitor.visit(it.value.as_object()); } } diff --git a/Libraries/LibJS/Object.h b/Libraries/LibJS/Object.h index 1f9ce7e018..cb13c4b4ba 100644 --- a/Libraries/LibJS/Object.h +++ b/Libraries/LibJS/Object.h @@ -44,7 +44,7 @@ public: virtual bool is_function() const { return false; } virtual const char* class_name() const override { return "Object"; } - virtual void visit_graph(Cell::Visitor&) override; + virtual void visit_children(Cell::Visitor&) override; private: HashMap m_properties;