Bug 1879918 - Whole cell buffer tracing is no longer order-dependent between strings and non-strings r=jonco

Differential Revision: https://phabricator.services.mozilla.com/D205143

js/src/gc/Nursery.cpp

@@ -1603,9 +1603,6 @@ void js::Nursery::traceRoots(AutoGCSession& session, TenuringTracer& mover) {
     MOZ_ASSERT(gc->storeBuffer().isEnabled());
     MOZ_ASSERT(gc->storeBuffer().isEmpty());
 
-    // Strings in the whole cell buffer must be traced first, in order to handle
-    // tenured dependent strings pointing to movable chars before the base
-    // string holding those chars is tenured.
     startProfile(ProfileKey::TraceWholeCells);
     sb.traceWholeCells(mover);
     endProfile(ProfileKey::TraceWholeCells);

js/src/gc/StoreBuffer.cpp

@@ -31,8 +31,7 @@ void StoreBuffer::checkAccess() const {
 #endif
 
 bool StoreBuffer::WholeCellBuffer::init() {
-  MOZ_ASSERT(!stringHead_);
-  MOZ_ASSERT(!nonStringHead_);
+  MOZ_ASSERT(!head_);
   if (!storage_) {
     storage_ = MakeUnique<LifoAlloc>(LifoAllocBlockSize);
     // This prevents LifoAlloc::Enum from crashing with a release
@@ -76,8 +75,7 @@ StoreBuffer::StoreBuffer(JSRuntime* rt)
       mayHavePointersToDeadCells_(false)
 #ifdef DEBUG
       ,
-      mEntered(false),
-      markingStringWholeCells(false)
+      mEntered(false)
 #endif
 {
 }
@@ -98,13 +96,11 @@ StoreBuffer::StoreBuffer(StoreBuffer&& other)
       mayHavePointersToDeadCells_(other.mayHavePointersToDeadCells_)
 #ifdef DEBUG
       ,
-      mEntered(other.mEntered),
-      markingStringWholeCells(other.markingStringWholeCells)
+      mEntered(other.mEntered)
 #endif
 {
   MOZ_ASSERT(enabled_);
   MOZ_ASSERT(!mEntered);
-  MOZ_ASSERT(!markingStringWholeCells);
   other.disable();
 }
 
@@ -214,21 +210,14 @@ ArenaCellSet* StoreBuffer::WholeCellBuffer::allocateCellSet(Arena* arena) {
     return nullptr;
   }
 
-  // Maintain separate lists for strings and non-strings, so that all buffered
-  // string whole cells will be processed before anything else (to prevent them
-  // from being deduplicated when their chars are used by a tenured string.)
-  bool isString =
-      MapAllocToTraceKind(arena->getAllocKind()) == JS::TraceKind::String;
-
   AutoEnterOOMUnsafeRegion oomUnsafe;
-  ArenaCellSet*& head = isString ? stringHead_ : nonStringHead_;
-  auto* cells = storage_->new_<ArenaCellSet>(arena, head);
+  auto* cells = storage_->new_<ArenaCellSet>(arena, head_);
   if (!cells) {
     oomUnsafe.crash("Failed to allocate ArenaCellSet");
   }
 
   arena->bufferedCells() = cells;
-  head = cells;
+  head_ = cells;
 
   if (isAboutToOverflow()) {
     rt->gc.storeBuffer().setAboutToOverflow(
@@ -244,12 +233,10 @@ void gc::CellHeaderPostWriteBarrier(JSObject** ptr, JSObject* prev,
 }
 
 void StoreBuffer::WholeCellBuffer::clear() {
-  for (auto** headPtr : {&stringHead_, &nonStringHead_}) {
-    for (auto* set = *headPtr; set; set = set->next) {
-      set->arena->bufferedCells() = &ArenaCellSet::Empty;
-    }
-    *headPtr = nullptr;
+  for (auto* set = head_; set; set = set->next) {
+    set->arena->bufferedCells() = &ArenaCellSet::Empty;
   }
+  head_ = nullptr;
 
   if (storage_) {
     storage_->used() ? storage_->releaseAll() : storage_->freeAll();

js/src/gc/StoreBuffer.h

@@ -169,8 +169,7 @@ class StoreBuffer {
 
   struct WholeCellBuffer {
     UniquePtr<LifoAlloc> storage_;
-    ArenaCellSet* stringHead_ = nullptr;
-    ArenaCellSet* nonStringHead_ = nullptr;
+    ArenaCellSet* head_ = nullptr;
     const Cell* last_ = nullptr;
 
     WholeCellBuffer() = default;
@@ -180,11 +179,9 @@ class StoreBuffer {
 
     WholeCellBuffer(WholeCellBuffer&& other)
         : storage_(std::move(other.storage_)),
-          stringHead_(other.stringHead_),
-          nonStringHead_(other.nonStringHead_),
+          head_(other.head_),
           last_(other.last_) {
-      other.stringHead_ = nullptr;
-      other.nonStringHead_ = nullptr;
+      other.head_ = nullptr;
       other.last_ = nullptr;
     }
     WholeCellBuffer& operator=(WholeCellBuffer&& other) {
@@ -214,9 +211,8 @@ class StoreBuffer {
     }
 
     bool isEmpty() const {
-      MOZ_ASSERT_IF(!stringHead_ && !nonStringHead_,
-                    !storage_ || storage_->isEmpty());
-      return !stringHead_ && !nonStringHead_;
+      MOZ_ASSERT_IF(!head_, !storage_ || storage_->isEmpty());
+      return !head_;
     }
 
     const Cell** lastBufferedPtr() { return &last_; }
@@ -224,8 +220,7 @@ class StoreBuffer {
     CellSweepSet releaseCellSweepSet() {
       CellSweepSet set;
       std::swap(storage_, set.storage_);
-      std::swap(stringHead_, set.head_);
-      nonStringHead_ = nullptr;
+      std::swap(head_, set.head_);
       last_ = nullptr;
       return set;
     }
@@ -535,16 +530,6 @@ class StoreBuffer {
 #endif
 
  public:
-#ifdef DEBUG
-  // If a dependent string in the whole cell buffer has an edge to a base
-  // string, then that base must be visited first through that whole cell buffer
-  // entry so that it doesn't get deduplicated and disappear before the
-  // dependent string is processed. Assert that we visit all string entries
-  // first, before there is any possibility that they will be visited through a
-  // more generic mechanism.
-  bool markingStringWholeCells;
-#endif
-
   explicit StoreBuffer(JSRuntime* rt);
 
   StoreBuffer(const StoreBuffer& other) = delete;
@@ -718,8 +703,8 @@ class ArenaCellSet {
     bits.setWord(wordIndex, value);
   }
 
-  void traceStrings(TenuringTracer& mover);
-  void traceNonStrings(TenuringTracer& mover);
+  // Returns the list of ArenaCellSets that need to be swept.
+  ArenaCellSet* trace(TenuringTracer& mover);
 
   // At the end of a minor GC, sweep through all tenured dependent strings that
   // may point to nursery-allocated chars to update their pointers in case the

js/src/gc/Tenuring.cpp

@@ -339,16 +339,11 @@ void js::gc::StoreBuffer::SlotsEdge::trace(TenuringTracer& mover) const {
 }
 
 static inline void TraceWholeCell(TenuringTracer& mover, JSObject* object) {
-  MOZ_ASSERT_IF(object->storeBuffer(),
-                !object->storeBuffer()->markingStringWholeCells);
   mover.traceObject(object);
 }
 
 // Return whether the string needs to be swept.
 static inline bool TraceWholeCell(TenuringTracer& mover, JSString* str) {
-  MOZ_ASSERT_IF(str->storeBuffer(),
-                str->storeBuffer()->markingStringWholeCells);
-
   if (str->hasBase()) {
     // For tenured dependent strings -> nursery string edges, sweep the
     // (tenured) strings at the end of nursery marking to update chars pointers
@@ -378,7 +373,7 @@ static inline void TraceWholeCell(TenuringTracer& mover,
 }
 
 template <typename T>
-void TenuringTracer::traceBufferedCells(Arena* arena, ArenaCellSet* cells) {
+bool TenuringTracer::traceBufferedCells(Arena* arena, ArenaCellSet* cells) {
   for (size_t i = 0; i < MaxArenaCellIndex; i += cells->BitsPerWord) {
     ArenaCellSet::WordT bitset = cells->getWord(i / cells->BitsPerWord);
     while (bitset) {
@@ -397,11 +392,14 @@ void TenuringTracer::traceBufferedCells(Arena* arena, ArenaCellSet* cells) {
       }
     }
   }
+
+  return false;
 }
 
 template <>
-void TenuringTracer::traceBufferedCells<JSString>(Arena* arena,
-                                  ArenaCellSet* cells) {
+bool TenuringTracer::traceBufferedCells<JSString>(Arena* arena,
+                                                  ArenaCellSet* cells) {
+  bool needsSweep = false;
   for (size_t i = 0; i < MaxArenaCellIndex; i += cells->BitsPerWord) {
     ArenaCellSet::WordT bitset = cells->getWord(i / cells->BitsPerWord);
     ArenaCellSet::WordT tosweep = bitset;
@@ -422,67 +420,62 @@ void TenuringTracer::traceBufferedCells<JSString>(Arena* arena,
     }
 
     cells->setWord(i / cells->BitsPerWord, tosweep);
+    if (tosweep) {
+      needsSweep = true;
+    }
   }
+
+  return needsSweep;
 }
 
-void ArenaCellSet::traceNonStrings(TenuringTracer& mover) {
-  for (ArenaCellSet* cells = this; cells; cells = cells->next) {
+ArenaCellSet* ArenaCellSet::trace(TenuringTracer& mover) {
+  ArenaCellSet* head = nullptr;
+
+  ArenaCellSet* cells = this;
+  while (cells) {
     cells->check();
 
     Arena* arena = cells->arena;
     arena->bufferedCells() = &ArenaCellSet::Empty;
 
     JS::TraceKind kind = MapAllocToTraceKind(arena->getAllocKind());
+    bool needsSweep;
     switch (kind) {
       case JS::TraceKind::Object:
-        mover.traceBufferedCells<JSObject>(arena, cells);
+        needsSweep = mover.traceBufferedCells<JSObject>(arena, cells);
+        break;
+      case JS::TraceKind::String:
+        needsSweep = mover.traceBufferedCells<JSString>(arena, cells);
         break;
       case JS::TraceKind::Script:
-        mover.traceBufferedCells<BaseScript>(arena, cells);
+        needsSweep = mover.traceBufferedCells<BaseScript>(arena, cells);
         break;
       case JS::TraceKind::JitCode:
-        mover.traceBufferedCells<jit::JitCode>(arena, cells);
+        needsSweep = mover.traceBufferedCells<jit::JitCode>(arena, cells);
         break;
       default:
         MOZ_CRASH("Unexpected trace kind");
     }
-  }
-}
 
-void ArenaCellSet::traceStrings(TenuringTracer& mover) {
-  for (ArenaCellSet* cells = this; cells; cells = cells->next) {
-    cells->check();
-    Arena* arena = cells->arena;
-    MOZ_ASSERT(MapAllocToTraceKind(arena->getAllocKind()) ==
-               JS::TraceKind::String);
-    mover.traceBufferedCells<JSString>(arena, cells);
-    // The ArenaCellSet will be left holding the cells that require sweeping.
+    ArenaCellSet* next = cells->next;
+    if (needsSweep) {
+      cells->next = head;
+      head = cells;
+    }
+
+    cells = next;
   }
+
+  return head;
 }
 
 void js::gc::StoreBuffer::WholeCellBuffer::trace(TenuringTracer& mover,
                                                  StoreBuffer* owner) {
   MOZ_ASSERT(owner->isEnabled());
 
-#ifdef DEBUG
-  // Verify that all string whole cells are traced first before any other
-  // strings are visited for any reason.
-  MOZ_ASSERT(!owner->markingStringWholeCells);
-  owner->markingStringWholeCells = true;
-#endif
-  // Trace all of the strings to mark the non-deduplicatable bits, then trace
-  // all other whole cells.
-  if (stringHead_) {
-    stringHead_->traceStrings(mover);
+  if (head_) {
+    head_ = head_->trace(mover);
   }
-#ifdef DEBUG
-  owner->markingStringWholeCells = false;
-#endif
-  if (nonStringHead_) {
-    nonStringHead_->traceNonStrings(mover);
-  }
-
-  nonStringHead_ = nullptr;
 }
 
 // Given a tenured dependent string, walk up its chain of nursery bases until

js/src/gc/Tenuring.h

@@ -106,8 +106,9 @@ class TenuringTracer final : public JSTracer {
   JSString* promoteOrForward(JSString* str);
   JS::BigInt* promoteOrForward(JS::BigInt* bip);
 
+  // Returns whether any cells in the arena require sweeping.
   template <typename T>
-  void traceBufferedCells(Arena* arena, ArenaCellSet* cells);
+  bool traceBufferedCells(Arena* arena, ArenaCellSet* cells);
 
   class AutoPromotedAnyToNursery;