Add write barriers to all places where an array length is shortened

Riley Dulin · facebook-github-bot · commit e0cd6e06edbb · 2020-09-08T12:08:18.000-07:00
Summary:
In Hades, a write barrier is needed when a pointer becomes unreachable. This
primarily happens when the GC has a different set of things that might be marked.
Currently, the only way that can happen is if an array's length is reduced.

Whenever that can occur, call a new API on GCHermesValue: `unreachableWriteBarrier`,
to alert the GC of the change.

Reviewed By: davedets

Differential Revision: D23362654

fbshipit-source-id: 4125f899774336e60aafd7282310aa7d25388a2b
diff --git a/include/hermes/VM/ArrayStorage.h b/include/hermes/VM/ArrayStorage.h
@@ -156,14 +156,10 @@ class ArrayStorage final
     const size_type sz = size();
     assert(sz > 0 && "Can't pop from empty ArrayStorage");
     HermesValue val = data()[sz - 1];
-#ifdef HERMESVM_GC_HADES
     // In Hades, a snapshot write barrier must be executed on the value that is
     // conceptually being changed to null. The write doesn't need to occur, but
     // it is the only correct way to use the write barrier.
-    data()[sz - 1].set(HermesValue::encodeEmptyValue(), &runtime->getHeap());
-#else
-    (void)runtime;
-#endif
+    data()[sz - 1].unreachableWriteBarrier(&runtime->getHeap());
     // The background thread can't mutate size, so we don't need fetch_sub here.
     // Relaxed is fine, because the GC doesn't care about the order of seeing
     // the length and the individual elements, as long as illegal HermesValues
diff --git a/include/hermes/VM/HadesGC.h b/include/hermes/VM/HadesGC.h
@@ -138,6 +138,9 @@ class HadesGC final : public GCBase {
   void constructorWriteBarrier(void *loc, HermesValue value);
   void constructorWriteBarrier(void *loc, void *value);
 
+  void snapshotWriteBarrier(GCHermesValue *loc);
+  void snapshotWriteBarrierRange(GCHermesValue *start, uint32_t numHVs);
+
   void weakRefReadBarrier(void *value);
   void weakRefReadBarrier(HermesValue value);
 
@@ -552,11 +555,12 @@ class HadesGC final : public GCBase {
   void scanDirtyCards(EvacAcceptor &acceptor);
 
   /// Common logic for doing the Snapshot At The Beginning (SATB) write barrier.
-  void snapshotWriteBarrier(GCCell *oldValue);
+  void snapshotWriteBarrierInternal(GCCell *oldValue);
 
   /// Common logic for doing the Snapshot At The Beginning (SATB) write barrier.
-  /// Forwards to \c snapshotWriteBarrier(GCCell *) if oldValue is a pointer.
-  void snapshotWriteBarrier(HermesValue oldValue);
+  /// Forwards to \c snapshotWriteBarrierInternal(GCCell *) if oldValue is a
+  /// pointer.
+  void snapshotWriteBarrierInternal(HermesValue oldValue);
 
   /// Common logic for doing the generational write barrier for detecting
   /// pointers into YG.
diff --git a/include/hermes/VM/HermesValue-inline.h b/include/hermes/VM/HermesValue-inline.h
@@ -71,6 +71,13 @@ void GCHermesValue::setNonPtr(HermesValue hv, GC *gc) {
   setNoBarrier(hv);
 }
 
+void GCHermesValue::unreachableWriteBarrier(GC *gc) {
+#ifdef HERMESVM_GC_HADES
+  // Hades needs a snapshot barrier executed when something becomes unreachable.
+  gc->snapshotWriteBarrier(this);
+#endif
+}
+
 /*static*/
 template <typename InputIt>
 inline void
@@ -183,6 +190,15 @@ inline OutputIt GCHermesValue::copy_backward(
   return result;
 }
 
+inline void GCHermesValue::rangeUnreachableWriteBarrier(
+    GCHermesValue *first,
+    GCHermesValue *last,
+    GC *gc) {
+#ifdef HERMESVM_GC_HADES
+  gc->snapshotWriteBarrierRange(first, last - first);
+#endif
+}
+
 inline void GCHermesValue::copyToPinned(
     const GCHermesValue *first,
     const GCHermesValue *last,
diff --git a/include/hermes/VM/HermesValue.h b/include/hermes/VM/HermesValue.h
@@ -541,6 +541,13 @@ class GCHermesValue : public HermesValue {
   /// Some GCs still need to do a write barrier though, so pass a GC parameter.
   inline void setNonPtr(HermesValue hv, GC *gc);
 
+  /// Force a write barrier to occur on this value, as if the value was being
+  /// set to null. This should be used when a value is becoming unreachable by
+  /// the GC, without having anything written to its memory.
+  /// NOTE: This barrier is typically used when a variable-sized object's length
+  /// decreases.
+  inline void unreachableWriteBarrier(GC *gc);
+
   /// Fills a region of GCHermesValues defined by [\p first, \p last) with the
   /// value \p fill.  If the fill value is an object pointer, must
   /// provide a non-null \p gc argument, to perform write barriers.
@@ -572,6 +579,13 @@ class GCHermesValue : public HermesValue {
   static inline OutputIt
   copy_backward(InputIt first, InputIt last, OutputIt result, GC *gc);
 
+  /// Same as \c unreachableWriteBarrier, but for a range of values all becoming
+  /// unreachable.
+  static inline void rangeUnreachableWriteBarrier(
+      GCHermesValue *first,
+      GCHermesValue *last,
+      GC *gc);
+
   /// Copies a range of values to a non-heap location, e.g., the JS stack.
   static inline void copyToPinned(
       const GCHermesValue *first,
diff --git a/include/hermes/VM/SegmentedArray.h b/include/hermes/VM/SegmentedArray.h
@@ -85,7 +85,10 @@ class SegmentedArray final
     /// It is the caller's responsibility to ensure that the newly used portion
     /// will contain valid values before they are accessed (including accesses
     /// by the GC).
+    /// \pre This cannot be called if kConcurrentGC is true, since the GC might
+    ///   read uninitialized memory even if the mutator wouldn't.
     void setLengthWithoutFilling(uint32_t newLength) {
+      assert(!kConcurrentGC && "Cannot avoid filling for a concurrent GC");
       assert(newLength <= kMaxLength && "Cannot set length to more than size");
       length_.store(newLength, std::memory_order_release);
     }
diff --git a/lib/VM/ArrayStorage.cpp b/lib/VM/ArrayStorage.cpp
@@ -169,17 +169,12 @@ void ArrayStorage::resizeWithinCapacity(
         HermesValue::encodeEmptyValue(),
         &runtime->getHeap());
   } else if (newSize < sz) {
-#ifdef HERMESVM_GC_HADES
     // Execute write barriers on elements about to be conceptually changed to
     // null.
     // This also means if an array is refilled, it can treat the memory here
     // as uninitialized safely.
-    GCHermesValue::fill(
-        self->data() + newSize,
-        self->data() + sz,
-        HermesValue::encodeEmptyValue(),
-        &runtime->getHeap());
-#endif
+    GCHermesValue::rangeUnreachableWriteBarrier(
+        self->data() + newSize, self->data() + sz, &runtime->getHeap());
   }
   self->size_.store(newSize, std::memory_order_release);
 }
@@ -191,6 +186,8 @@ ExecutionStatus ArrayStorage::shift(
     size_type toFirst,
     size_type toLast) {
   assert(toLast <= maxElements() && "size overflows 32-bit storage");
+  assert(toFirst <= toLast && "First must be before last");
+  assert(fromFirst <= selfHandle->size() && "fromFirst must be before size");
 
   // If we don't need to expand the capacity.
   if (toLast <= selfHandle->capacity_) {
@@ -229,6 +226,13 @@ ExecutionStatus ArrayStorage::shift(
           HermesValue::encodeEmptyValue(),
           &runtime->getHeap());
     }
+    if (toLast < self->size()) {
+      // Some elements are becoming unreachable, let the GC know.
+      GCHermesValue::rangeUnreachableWriteBarrier(
+          self->data() + toLast,
+          self->data() + self->size(),
+          &runtime->getHeap());
+    }
     self->size_.store(toLast, std::memory_order_release);
     return ExecutionStatus::RETURNED;
   }
diff --git a/lib/VM/SegmentedArray.cpp b/lib/VM/SegmentedArray.cpp
@@ -81,9 +81,13 @@ void SegmentedArray::Segment::setLength(Runtime *runtime, uint32_t newLength) {
         data_ + newLength,
         HermesValue::encodeEmptyValue(),
         &runtime->getHeap());
+    length_.store(newLength, std::memory_order_release);
+  } else if (newLength < len) {
+    // If length is decreasing a write barrier needs to be done.
+    GCHermesValue::rangeUnreachableWriteBarrier(
+        data_ + newLength, data_ + len, &runtime->getHeap());
+    length_.store(newLength, std::memory_order_release);
   }
-  // If length is decreasing nothing special needs to be done.
-  setLengthWithoutFilling(newLength);
 }
 
 VTable SegmentedArray::vt(
@@ -332,8 +336,9 @@ ExecutionStatus SegmentedArray::growLeft(
     return ExecutionStatus::EXCEPTION;
   }
   PseudoHandle<SegmentedArray> newSegmentedArray = std::move(*arrRes);
-  // Don't fill with empty values, most will be copied in.
-  newSegmentedArray = increaseSize</*Fill*/ false>(
+  // If it's not a concurrent GC, don't fill with empty values, most will be
+  // copied in.
+  newSegmentedArray = increaseSize</*Fill*/ kConcurrentGC>(
       runtime, std::move(newSegmentedArray), newSize);
   // Fill the beginning of the new array with empty values.
   GCHermesValue::uninitialized_fill(
@@ -389,6 +394,9 @@ void SegmentedArray::increaseSizeWithinCapacity(
     Runtime *runtime,
     size_type amount,
     bool fill) {
+  assert(
+      !kConcurrentGC ||
+      fill && "If kConcurrentGC is true, fill must also be true");
   // This function has the same logic as increaseSize, but removes some
   // complexity from avoiding dealing with alllocations.
   const auto empty = HermesValue::encodeEmptyValue();
@@ -435,6 +443,9 @@ PseudoHandle<SegmentedArray> SegmentedArray::increaseSize(
     Runtime *runtime,
     PseudoHandle<SegmentedArray> self,
     size_type amount) {
+  assert(
+      !kConcurrentGC ||
+      Fill && "If kConcurrentGC is true, fill must also be true");
   const auto empty = HermesValue::encodeEmptyValue();
   const auto currSize = self->size();
   const auto finalSize = currSize + amount;
@@ -530,18 +541,26 @@ PseudoHandle<SegmentedArray> SegmentedArray::increaseSize(
 }
 
 void SegmentedArray::decreaseSize(Runtime *runtime, size_type amount) {
-  assert(amount <= size() && "Cannot decrease size past zero");
-  const auto finalSize = size() - amount;
-  if (finalSize <= kValueToSegmentThreshold) {
-    // Just adjust the field and exit, no segments to compress.
-    numSlotsUsed_.store(finalSize, std::memory_order_release);
-    return;
+  const auto initialSize = size();
+  const auto initialNumSlots = numSlotsUsed_.load(std::memory_order_relaxed);
+  assert(amount <= initialSize && "Cannot decrease size past zero");
+  const auto finalSize = initialSize - amount;
+  const auto finalNumSlots = numSlotsForCapacity(finalSize);
+  assert(
+      finalNumSlots <= initialNumSlots &&
+      "Should not be increasing the number of slots");
+  if (finalSize > kValueToSegmentThreshold) {
+    // Set the new last used segment's length to be the leftover.
+    segmentAt(toSegment(finalSize - 1))
+        ->setLength(runtime, toInterior(finalSize - 1) + 1);
   }
-  // Set the new last used segment's length to be the leftover.
-  segmentAt(toSegment(finalSize - 1))
-      ->setLength(runtime, toInterior(finalSize - 1) + 1);
-  numSlotsUsed_.store(
-      numSlotsForCapacity(finalSize), std::memory_order_release);
+  // Before shrinking, do a snapshot write barrier for the elements being
+  // removed.
+  GCHermesValue::rangeUnreachableWriteBarrier(
+      inlineStorage() + finalNumSlots,
+      inlineStorage() + initialNumSlots,
+      &runtime->getHeap());
+  numSlotsUsed_.store(finalNumSlots, std::memory_order_release);
 }
 
 gcheapsize_t SegmentedArray::_trimSizeCallback(const GCCell *cell) {
diff --git a/lib/VM/gcs/HadesGC.cpp b/lib/VM/gcs/HadesGC.cpp
@@ -1240,7 +1240,7 @@ void HadesGC::writeBarrier(void *loc, HermesValue value) {
     return;
   }
   if (concurrentPhase_.load(std::memory_order_acquire) == Phase::Mark) {
-    snapshotWriteBarrier(*static_cast<HermesValue *>(loc));
+    snapshotWriteBarrierInternal(*static_cast<HermesValue *>(loc));
   }
   if (!value.isPointer()) {
     return;
@@ -1264,7 +1264,7 @@ void HadesGC::writeBarrier(void *loc, void *value) {
 #else
     GCCell *const oldValue = static_cast<GCCell *>(oldValueStorage);
 #endif
-    snapshotWriteBarrier(oldValue);
+    snapshotWriteBarrierInternal(oldValue);
   }
   // Always do the non-snapshot write barrier in order for YG to be able to
   // scan cards.
@@ -1294,7 +1294,30 @@ void HadesGC::constructorWriteBarrier(void *loc, void *value) {
   generationalWriteBarrier(loc, value);
 }
 
-void HadesGC::snapshotWriteBarrier(GCCell *oldValue) {
+void HadesGC::snapshotWriteBarrier(GCHermesValue *loc) {
+  if (inYoungGen(loc)) {
+    // A pointer that lives in YG never needs any write barriers.
+    return;
+  }
+  if (concurrentPhase_.load(std::memory_order_acquire) == Phase::Mark) {
+    snapshotWriteBarrierInternal(*loc);
+  }
+}
+
+void HadesGC::snapshotWriteBarrierRange(GCHermesValue *start, uint32_t numHVs) {
+  if (inYoungGen(start)) {
+    // A pointer that lives in YG never needs any write barriers.
+    return;
+  }
+  if (concurrentPhase_.load(std::memory_order_acquire) != Phase::Mark) {
+    return;
+  }
+  for (uint32_t i = 0; i < numHVs; ++i) {
+    snapshotWriteBarrierInternal(start[i]);
+  }
+}
+
+void HadesGC::snapshotWriteBarrierInternal(GCCell *oldValue) {
   assert(
       (!oldValue || oldValue->isValid()) &&
       "Invalid cell encountered in snapshotWriteBarrier");
@@ -1306,9 +1329,9 @@ void HadesGC::snapshotWriteBarrier(GCCell *oldValue) {
   }
 }
 
-void HadesGC::snapshotWriteBarrier(HermesValue oldValue) {
+void HadesGC::snapshotWriteBarrierInternal(HermesValue oldValue) {
   if (oldValue.isPointer()) {
-    snapshotWriteBarrier(static_cast<GCCell *>(oldValue.getPointer()));
+    snapshotWriteBarrierInternal(static_cast<GCCell *>(oldValue.getPointer()));
   }
 }
 
@@ -1339,7 +1362,7 @@ void HadesGC::weakRefReadBarrier(void *value) {
       return;
     case Phase::Mark:
       // Treat the value read from the weak reference as live.
-      snapshotWriteBarrier(static_cast<GCCell *>(value));
+      snapshotWriteBarrierInternal(static_cast<GCCell *>(value));
       return;
   }
   llvm_unreachable("All phases should be handled");
