Clarify roots iteration

Change Heap::IterateStrongRoots to never iterate the read-only roots. In
doing so remove VISIT_ALL_BUT_READ_ONLY and
VISIT_ONLY_STRONG_FOR_SERIALIZATION. All such uses should now use
VISIT_ALL and VISIT_ONLY_STRONG. Where ReadOnlyRoots iteration is
required, this adds ReadOnlyRoots(isolate)->Iterate() at the call site.

Add new begin, end, strong_mutable_roots_begin and
strong_mutable_roots_end methods to RootsTable and try and make the
existing uses a little more consistent.

Bug: v8:8191
Change-Id: Ie9d0f9e5186db418428e2fafd38432b0bd879daa
Reviewed-on: https://chromium-review.googlesource.com/c/1278500
Commit-Queue: Dan Elphick <delphick@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56672}

src/address-map.cc

@@ -14,8 +14,8 @@ RootIndexMap::RootIndexMap(Isolate* isolate) {
   map_ = isolate->root_index_map();
   if (map_ != nullptr) return;
   map_ = new HeapObjectToIndexHashMap();
-  for (RootIndex root_index = RootIndex::kFirstStrongRoot;
-       root_index <= RootIndex::kLastStrongRoot; ++root_index) {
+  for (RootIndex root_index = RootIndex::kFirstStrongOrReadOnlyRoot;
+       root_index <= RootIndex::kLastStrongOrReadOnlyRoot; ++root_index) {
     Object* root = isolate->root(root_index);
     if (!root->IsHeapObject()) continue;
     // Omit root entries that can be written after initialization. They must

src/globals.h

@@ -623,13 +623,11 @@ enum Movability { kMovable, kImmovable };
 
 enum VisitMode {
   VISIT_ALL,
-  VISIT_ALL_BUT_READ_ONLY,
   VISIT_ALL_IN_MINOR_MC_MARK,
   VISIT_ALL_IN_MINOR_MC_UPDATE,
   VISIT_ALL_IN_SCAVENGE,
   VISIT_ALL_IN_SWEEP_NEWSPACE,
   VISIT_ONLY_STRONG,
-  VISIT_ONLY_STRONG_FOR_SERIALIZATION,
   VISIT_FOR_SERIALIZATION,
 };
 

src/heap/heap.cc

@@ -2568,6 +2568,7 @@ FixedArrayBase* Heap::LeftTrimFixedArray(FixedArrayBase* object,
     // Make sure the stack or other roots (e.g., Handles) don't contain pointers
     // to the original FixedArray (which is now the filler object).
     LeftTrimmerVerifierRootVisitor root_visitor(object);
+    ReadOnlyRoots(this).Iterate(&root_visitor);
     IterateRoots(&root_visitor, VISIT_ALL);
   }
 #endif  // ENABLE_SLOW_DCHECKS
@@ -3741,12 +3742,8 @@ void Heap::IterateStrongRoots(RootVisitor* v, VisitMode mode) {
   const bool isMinorGC = mode == VISIT_ALL_IN_SCAVENGE ||
                          mode == VISIT_ALL_IN_MINOR_MC_MARK ||
                          mode == VISIT_ALL_IN_MINOR_MC_UPDATE;
-  // Garbage collection can skip over the read-only roots.
-  const bool isGC = mode != VISIT_ALL && mode != VISIT_FOR_SERIALIZATION &&
-                    mode != VISIT_ONLY_STRONG_FOR_SERIALIZATION;
-  Object** start = isGC ? roots_table().read_only_roots_end()
-                        : roots_table().strong_roots_begin();
-  v->VisitRootPointers(Root::kStrongRootList, nullptr, start,
+  v->VisitRootPointers(Root::kStrongRootList, nullptr,
+                       roots_table().strong_roots_begin(),
                        roots_table().strong_roots_end());
   v->Synchronize(VisitorSynchronization::kStrongRootList);
 
@@ -3786,7 +3783,6 @@ void Heap::IterateStrongRoots(RootVisitor* v, VisitMode mode) {
       // global handles need to be added manually.
       break;
     case VISIT_ONLY_STRONG:
-    case VISIT_ONLY_STRONG_FOR_SERIALIZATION:
       isolate_->global_handles()->IterateStrongRoots(v);
       break;
     case VISIT_ALL_IN_SCAVENGE:
@@ -3798,7 +3794,6 @@ void Heap::IterateStrongRoots(RootVisitor* v, VisitMode mode) {
     case VISIT_ALL_IN_MINOR_MC_UPDATE:
       // Global handles are processed manually by the minor MC.
       break;
-    case VISIT_ALL_BUT_READ_ONLY:
     case VISIT_ALL_IN_SWEEP_NEWSPACE:
     case VISIT_ALL:
       isolate_->global_handles()->IterateAllRoots(v);
@@ -5025,7 +5020,7 @@ class UnreachableObjectsFilter : public HeapObjectsFilter {
 
   void MarkReachableObjects() {
     MarkingVisitor visitor(this);
-    heap_->IterateRoots(&visitor, VISIT_ALL_BUT_READ_ONLY);
+    heap_->IterateRoots(&visitor, VISIT_ALL);
     visitor.TransitiveClosure();
   }
 

src/heap/heap.h

@@ -744,7 +744,14 @@ class Heap {
   // Iterators. ================================================================
   // ===========================================================================
 
+  // None of these methods iterate over the read-only roots. To do this use
+  // ReadOnlyRoots::Iterate. Read-only root iteration is not necessary for
+  // garbage collection and is usually only performed as part of
+  // (de)serialization or heap verification.
+
+  // Iterates over the strong roots and the weak roots.
   void IterateRoots(RootVisitor* v, VisitMode mode);
+  // Iterates over the strong roots.
   void IterateStrongRoots(RootVisitor* v, VisitMode mode);
   // Iterates over entries in the smi roots list.  Only interesting to the
   // serializer/deserializer, since GC does not care about smis.

src/objects-debug.cc

@@ -2231,8 +2231,8 @@ bool CanLeak(Object* obj, Isolate* isolate) {
   if (obj->IsContext()) return true;
   if (obj->IsMap()) {
     Map* map = Map::cast(obj);
-    for (RootIndex root_index = RootIndex::kFirstStrongRoot;
-         root_index <= RootIndex::kLastStrongRoot; ++root_index) {
+    for (RootIndex root_index = RootIndex::kFirstStrongOrReadOnlyRoot;
+         root_index <= RootIndex::kLastStrongOrReadOnlyRoot; ++root_index) {
       if (map == isolate->root(root_index)) return false;
     }
     return true;

src/profiler/heap-snapshot-generator.cc

@@ -1427,7 +1427,8 @@ bool V8HeapExplorer::IterateAndExtractReferences(
   // first. Otherwise a particular JSFunction object could set
   // its custom name to a generic builtin.
   RootsReferencesExtractor extractor(this);
-  heap_->IterateRoots(&extractor, VISIT_ONLY_STRONG_FOR_SERIALIZATION);
+  ReadOnlyRoots(heap_).Iterate(&extractor);
+  heap_->IterateRoots(&extractor, VISIT_ONLY_STRONG);
   extractor.SetVisitingWeakRoots();
   heap_->IterateWeakGlobalHandles(&extractor);
 
@@ -1676,8 +1677,8 @@ void V8HeapExplorer::SetGcSubrootReference(Root root, const char* description,
 const char* V8HeapExplorer::GetStrongGcSubrootName(Object* object) {
   if (strong_gc_subroot_names_.empty()) {
     Isolate* isolate = heap_->isolate();
-    for (RootIndex root_index = RootIndex::kFirstStrongRoot;
-         root_index <= RootIndex::kLastStrongRoot; ++root_index) {
+    for (RootIndex root_index = RootIndex::kFirstStrongOrReadOnlyRoot;
+         root_index <= RootIndex::kLastStrongOrReadOnlyRoot; ++root_index) {
       const char* name = RootsTable::name(root_index);
       strong_gc_subroot_names_.emplace(isolate->root(root_index), name);
     }

src/roots.cc

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "src/roots.h"
+
 #include "src/elements-kind.h"
+#include "src/visitors.h"
 
 namespace v8 {
 namespace internal {
@@ -56,5 +58,12 @@ RootIndex RootsTable::RootIndexForEmptyFixedTypedArray(
   }
 }
 
+void ReadOnlyRoots::Iterate(RootVisitor* visitor) {
+  visitor->VisitRootPointers(Root::kReadOnlyRootList, nullptr,
+                             roots_table_.read_only_roots_begin(),
+                             roots_table_.read_only_roots_end());
+  visitor->Synchronize(VisitorSynchronization::kReadOnlyRootList);
+}
+
 }  // namespace internal
 }  // namespace v8

src/roots.h

@@ -23,6 +23,7 @@ class Isolate;
 class Map;
 class String;
 class Symbol;
+class RootVisitor;
 
 // Defines all the read-only roots in Heap.
 #define STRONG_READ_ONLY_ROOT_LIST(V)                                          \
@@ -356,10 +357,6 @@ enum class RootIndex : uint16_t {
   kFirstRoot = 0,
   kLastRoot = kRootListLength - 1,
 
-  // kStringTable is not a strong root.
-  kFirstStrongRoot = kFirstRoot,
-  kLastStrongRoot = kStringTable - 1,
-
 #define ROOT(...) +1
   kReadOnlyRootsCount = 0 READ_ONLY_ROOT_LIST(ROOT),
   kImmortalImmovableRootsCount =
@@ -368,6 +365,16 @@ enum class RootIndex : uint16_t {
   kFirstReadOnlyRoot = kFirstRoot,
   kLastReadOnlyRoot = kFirstReadOnlyRoot + kReadOnlyRootsCount - 1,
 
+  // The strong roots visited by the garbage collector (not including read-only
+  // roots).
+  kFirstStrongRoot = kLastReadOnlyRoot + 1,
+  // (kStringTable is not a strong root).
+  kLastStrongRoot = kStringTable - 1,
+
+  // All of the strong roots plus the read-only roots.
+  kFirstStrongOrReadOnlyRoot = kFirstRoot,
+  kLastStrongOrReadOnlyRoot = kLastStrongRoot,
+
   // All immortal immovable roots including read only ones.
   kFirstImmortalImmovableRoot = kFirstReadOnlyRoot,
   kLastImmortalImmovableRoot =
@@ -426,10 +433,30 @@ class RootsTable {
   }
 
  private:
+  Object** begin() {
+    return &roots_[static_cast<size_t>(RootIndex::kFirstRoot)];
+  }
+  Object** end() {
+    return &roots_[static_cast<size_t>(RootIndex::kLastRoot) + 1];
+  }
+
+  // Used for iterating over all of the read-only and mutable strong roots.
+  Object** strong_or_read_only_roots_begin() {
+    STATIC_ASSERT(static_cast<size_t>(RootIndex::kLastReadOnlyRoot) ==
+                  static_cast<size_t>(RootIndex::kFirstStrongRoot) - 1);
+    return &roots_[static_cast<size_t>(RootIndex::kFirstStrongOrReadOnlyRoot)];
+  }
+  Object** strong_or_read_only_roots_end() {
+    return &roots_[static_cast<size_t>(RootIndex::kLastStrongOrReadOnlyRoot) +
+                   1];
+  }
+
+  // The read-only, strong and Smi roots as defined by these accessors are all
+  // disjoint.
   Object** read_only_roots_begin() {
     return &roots_[static_cast<size_t>(RootIndex::kFirstReadOnlyRoot)];
   }
-  inline Object** read_only_roots_end() {
+  Object** read_only_roots_end() {
     return &roots_[static_cast<size_t>(RootIndex::kLastReadOnlyRoot) + 1];
   }
 
@@ -460,6 +487,7 @@ class RootsTable {
   friend class Heap;
   friend class Factory;
   friend class ReadOnlyRoots;
+  friend class RootsSerializer;
 };
 
 class ReadOnlyRoots {
@@ -478,8 +506,13 @@ class ReadOnlyRoots {
   V8_INLINE Map* MapForFixedTypedArray(ElementsKind elements_kind);
   V8_INLINE FixedTypedArrayBase* EmptyFixedTypedArrayForMap(const Map* map);
 
+  // Iterate over all the read-only roots. This is not necessary for garbage
+  // collection and is usually only performed as part of (de)serialization or
+  // heap verification.
+  void Iterate(RootVisitor* visitor);
+
  private:
-  const RootsTable& roots_table_;
+  RootsTable& roots_table_;
 };
 
 }  // namespace internal

src/snapshot/startup-deserializer.cc

@@ -37,8 +37,8 @@ void StartupDeserializer::DeserializeInto(Isolate* isolate) {
   {
     DisallowHeapAllocation no_gc;
     isolate->heap()->IterateSmiRoots(this);
-    isolate->heap()->IterateStrongRoots(this,
-                                        VISIT_ONLY_STRONG_FOR_SERIALIZATION);
+    ReadOnlyRoots(isolate).Iterate(this);
+    isolate->heap()->IterateStrongRoots(this, VISIT_ONLY_STRONG);
     isolate->heap()->RepairFreeListsAfterDeserialization();
     isolate->heap()->IterateWeakRoots(this, VISIT_FOR_SERIALIZATION);
     DeserializeDeferredObjects();

src/snapshot/startup-serializer.cc

@@ -117,6 +117,7 @@ void StartupSerializer::SerializeStrongReferences() {
   isolate->heap()->IterateSmiRoots(this);
   isolate->heap()->SetStackLimits();
   // First visit immortal immovables to make sure they end up in the first page.
+  ReadOnlyRoots(isolate).Iterate(this);
   isolate->heap()->IterateStrongRoots(this, VISIT_FOR_SERIALIZATION);
 }
 

src/visitors.h

@@ -17,6 +17,7 @@ class Object;
 #define ROOT_ID_LIST(V)                                \
   V(kStringTable, "(Internalized strings)")            \
   V(kExternalStringsTable, "(External strings)")       \
+  V(kReadOnlyRootList, "(Read-only roots)")            \
   V(kStrongRootList, "(Strong roots)")                 \
   V(kSmiRootList, "(Smi roots)")                       \
   V(kBootstrapper, "(Bootstrapper)")                   \