Add owning cell to parameters of GCHermesValueBase::set()

Differential Revision: D62196480

include/hermes/VM/ArrayStorage.h

@@ -151,7 +151,7 @@ class ArrayStorageBase final
   template <Inline inl = Inline::No>
   void set(size_type index, HVType val, GC &gc) {
     assert(index < size() && "index out of range");
-    data()[index].set(val, gc);
+    data()[index].set(val, gc, this);
   }
 
   /// \return the element at index \p index

include/hermes/VM/HermesValue-inline.h

@@ -49,13 +49,15 @@ GCHermesValueBase<HVType>::GCHermesValueBase(HVType hv, GC &gc, std::nullptr_t)
 
 template <typename HVType>
 template <typename NeedsBarriers>
-inline void GCHermesValueBase<HVType>::set(HVType hv, GC &gc) {
+inline void
+GCHermesValueBase<HVType>::set(HVType hv, GC &gc, const GCCell *owningObj) {
   if (hv.isPointer()) {
     HERMES_SLOW_ASSERT(
         gc.validPointer(hv.getPointer(gc.getPointerBase())) &&
         "Setting an invalid pointer into a GCHermesValue");
   }
   assert(NeedsBarriers::value || !gc.needsWriteBarrier(this, hv));
+  (void)owningObj;
   if (NeedsBarriers::value)
     gc.writeBarrier(this, hv);
   HVType::setNoBarrier(hv);
@@ -81,10 +83,11 @@ inline void GCHermesValueBase<HVType>::fill(
     InputIt start,
     InputIt end,
     HVType fill,
-    GC &gc) {
+    GC &gc,
+    const GCCell *owningObj) {
   if (fill.isPointer()) {
     for (auto cur = start; cur != end; ++cur) {
-      cur->set(fill, gc);
+      cur->set(fill, gc, owningObj);
     }
   } else {
     for (auto cur = start; cur != end; ++cur) {
@@ -121,14 +124,13 @@ inline OutputIt GCHermesValueBase<HVType>::copy(
     InputIt last,
     OutputIt result,
     GC &gc) {
-#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
   static_assert(
       !std::is_same<InputIt, GCHermesValueBase *>::value ||
           !std::is_same<OutputIt, GCHermesValueBase *>::value,
       "Pointer arguments must invoke pointer overload.");
-#endif
   for (; first != last; ++first, (void)++result) {
-    result->set(*first, gc);
+    auto [hv, owningObj] = first.get_cell_value();
+    result->set(*first, gc, owningObj);
   }
   return result;
 }
@@ -150,31 +152,38 @@ inline OutputIt GCHermesValueBase<HVType>::uninitialized_copy(
   return result;
 }
 
-// Specializations using memmove can't be used in Hades, because the concurrent
-// write barrier needs strict control over how assignments are done to HV fields
-// which need to be atomically updated.
-#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
 /// Specialization for raw pointers to do a ranged write barrier.
 template <typename HVType>
 inline GCHermesValueBase<HVType> *GCHermesValueBase<HVType>::copy(
     GCHermesValueBase<HVType> *first,
     GCHermesValueBase<HVType> *last,
     GCHermesValueBase<HVType> *result,
-    GC &gc) {
+    GC &gc,
+    const GCCell *owningObj) {
+// Specializations using memmove can't be used in Hades, because the concurrent
+// write barrier needs strict control over how assignments are done to HV fields
+// which need to be atomically updated.
+#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
+  gc.writeBarrierRange(result, last - first);
   // We must use "raw" function such as memmove here, rather than a
   // function like std::copy (or copy_backward) that respects
   // constructors and operator=.  For HermesValue, those require the
   // contents not to contain pointers.  The range write barrier
   // before the copies ensure that sufficient barriers are
   // performed.
-  gc.writeBarrierRange(result, last - first);
+  (void)owningObj;
   std::memmove(
       reinterpret_cast<void *>(result),
       first,
       (last - first) * sizeof(GCHermesValueBase<HVType>));
   return result + (last - first);
-}
+#else
+  for (; first != last; ++first, (void)++result) {
+    result->set(*first, gc, owningObj);
+  }
+  return result;
 #endif
+}
 
 /// Specialization for raw pointers to do a ranged write barrier.
 template <typename HVType>
@@ -183,7 +192,7 @@ inline GCHermesValueBase<HVType> *GCHermesValueBase<HVType>::uninitialized_copy(
     GCHermesValueBase<HVType> *last,
     GCHermesValueBase<HVType> *result,
     GC &gc,
-    const GCCell *cell) {
+    const GCCell *owningObj) {
 #ifndef NDEBUG
   uintptr_t fromFirst = reinterpret_cast<uintptr_t>(first),
             fromLast = reinterpret_cast<uintptr_t>(last);
@@ -195,7 +204,7 @@ inline GCHermesValueBase<HVType> *GCHermesValueBase<HVType>::uninitialized_copy(
       "Uninitialized range cannot overlap with an initialized one.");
 #endif
 
-  gc.constructorWriteBarrierRange(cell, result, last - first);
+  gc.constructorWriteBarrierRange(owningObj, result, last - first);
   // memcpy is fine for an uninitialized copy.
   std::memcpy(
       reinterpret_cast<void *>(result), first, (last - first) * sizeof(HVType));
@@ -208,9 +217,10 @@ inline OutputIt GCHermesValueBase<HVType>::copy_backward(
     InputIt first,
     InputIt last,
     OutputIt result,
-    GC &gc) {
+    GC &gc,
+    const GCCell *owningObj) {
   while (first != last) {
-    (--result)->set(*--last, gc);
+    (--result)->set(*--last, gc, owningObj);
   }
   return result;
 }

include/hermes/VM/HermesValue.h

@@ -531,10 +531,11 @@ class GCHermesValueBase final : public HVType {
   GCHermesValueBase(HVType hv, GC &gc, std::nullptr_t);
   GCHermesValueBase(const HVType &) = delete;
 
-  /// The HermesValue \p hv may be an object pointer.  Assign the
-  /// value, and perform any necessary write barriers.
+  /// The HermesValue \p hv may be an object pointer. Assign the value, and
+  /// perform any necessary write barriers. \p cell is the object that contains
+  /// this GCHermesValueBase. It's needed by the write barrier.
   template <typename NeedsBarriers = std::true_type>
-  inline void set(HVType hv, GC &gc);
+  inline void set(HVType hv, GC &gc, const GCCell *owningObj);
 
   /// The HermesValue \p hv must not be an object pointer.  Assign the
   /// value.
@@ -552,7 +553,12 @@ class GCHermesValueBase final : public HVType {
   /// value \p fill.  If the fill value is an object pointer, must
   /// provide a non-null \p gc argument, to perform write barriers.
   template <typename InputIt>
-  static inline void fill(InputIt first, InputIt last, HVType fill, GC &gc);
+  static inline void fill(
+      InputIt first,
+      InputIt last,
+      HVType fill,
+      GC &gc,
+      const GCCell *owningObj);
 
   /// Same as \p fill except the range expressed by  [\p first, \p last) has not
   /// been previously initialized. Cannot use this on previously initialized
@@ -573,14 +579,13 @@ class GCHermesValueBase final : public HVType {
   static inline OutputIt
   uninitialized_copy(InputIt first, InputIt last, OutputIt result, GC &gc);
 
-#if !defined(HERMESVM_GC_HADES) && !defined(HERMESVM_GC_RUNTIME)
   /// Same as \p copy, but specialized for raw pointers.
   static inline GCHermesValueBase<HVType> *copy(
       GCHermesValueBase<HVType> *first,
       GCHermesValueBase<HVType> *last,
       GCHermesValueBase<HVType> *result,
-      GC &gc);
-#endif
+      GC &gc,
+      const GCCell *owningObj);
 
   /// Same as \p uninitialized_copy, but specialized for raw pointers. This is
   /// unsafe to use if the memory region being copied into (pointed to by
@@ -591,12 +596,16 @@ class GCHermesValueBase final : public HVType {
       GCHermesValueBase<HVType> *last,
       GCHermesValueBase<HVType> *result,
       GC &gc,
-      const GCCell *cell);
+      const GCCell *owningObj);
 
   /// Copies a range of values and performs a write barrier on each.
   template <typename InputIt, typename OutputIt>
-  static inline OutputIt
-  copy_backward(InputIt first, InputIt last, OutputIt result, GC &gc);
+  static inline OutputIt copy_backward(
+      InputIt first,
+      InputIt last,
+      OutputIt result,
+      GC &gc,
+      const GCCell *owningObj);
 
   /// Same as \c unreachableWriteBarrier, but for a range of values all becoming
   /// unreachable.

include/hermes/VM/JSObject.h

@@ -1690,7 +1690,7 @@ template <SlotIndex index>
 inline void
 JSObject::setDirectSlotValue(JSObject *self, SmallHermesValue value, GC &gc) {
   static_assert(index < DIRECT_PROPERTY_SLOTS, "Must be a direct property");
-  self->directProps()[index].set(value, gc);
+  self->directProps()[index].set(value, gc, self);
 }
 
 inline SmallHermesValue JSObject::getNamedSlotValueUnsafe(
@@ -1793,7 +1793,7 @@ inline void JSObject::setNamedSlotValueDirectUnsafe(
   // to namedSlotRef(), it is a slight performance regression, which is not
   // entirely unexpected.
   return self->directProps()[index].set<NeedsBarriers>(
-      value, runtime.getHeap());
+      value, runtime.getHeap(), self);
 }
 
 inline void JSObject::setNamedSlotValueIndirectUnsafe(

include/hermes/VM/SegmentedArray.h

@@ -161,6 +161,8 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell,
     using difference_type = ptrdiff_t;
     using pointer = GCHVType *;
     using reference = GCHVType &;
+    // Dereference a value
+    using cell_value_pair = std::pair<reference, GCCell *>;
 
     /// The SegmentedArray which owns this iterator. This iterator should never
     /// be compared against an iterator with a different owner. This is used to
@@ -238,6 +240,24 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell,
       }
     }
 
+    /// Return the value and the owning cell of that value (either owner_ or the
+    /// extra Segment allocated for the current index_).
+    /// Note: this is a temporary workaround to pass the correct owning object
+    /// pointer to write barrier. Once we support large allocation, we could
+    /// get rid of this (and probably the entire iterator here).
+    cell_value_pair get_cell_value() {
+      assert(
+          index_ < owner_->size(base_) &&
+          "Trying to read from an index outside the size");
+      // Almost all arrays fit entirely in the inline storage.
+      if (LLVM_LIKELY(index_ < kValueToSegmentThreshold)) {
+        return {owner_->inlineStorage()[index_], owner_};
+      } else {
+        auto *segment = owner_->segmentAt(base_, toSegment(index_));
+        return {segment->at(toInterior(index_)), segment};
+      }
+    }
+
     pointer operator->() {
       return &**this;
     }
@@ -288,7 +308,25 @@ class SegmentedArrayBase final : public VariableSizeRuntimeCell,
   /// Sets the element located at \p index to \p val.
   template <Inline inl = Inline::No>
   void set(Runtime &runtime, TotalIndex index, HVType val) {
-    atRef<inl>(runtime, index).set(val, runtime.getHeap());
+    assert(index < size(runtime) && "Out-of-bound access");
+    if constexpr (inl == Inline::Yes) {
+      assert(
+          index < kValueToSegmentThreshold &&
+          "Using the inline storage accessor when the index is larger than the "
+          "inline storage");
+      inlineStorage()[index].set(val, runtime.getHeap(), this);
+    }
+    // The caller may not set inl to Inline::Yes when the index is actually in
+    // inline storage.
+    if (index < kValueToSegmentThreshold) {
+      inlineStorage()[index].set(val, runtime.getHeap(), this);
+    } else {
+      auto segmentNumber = toSegment(index);
+      auto *segment = segmentAt(runtime, segmentNumber);
+      auto &elm = segment->at(toInterior(index));
+      // elm lives in segment, which is not the same cell as SegmentedArrayBase.
+      elm.set(val, runtime.getHeap(), segment);
+    }
   }
   template <Inline inl = Inline::No>
   void setNonPtr(Runtime &runtime, TotalIndex index, HVType val) {

include/hermes/VM/StringPrimitive.h

@@ -743,7 +743,7 @@ class BufferedStringPrimitive final : public StringPrimitive {
       Handle<ExternalStringPrimitive<T>> concatBuffer)
       : StringPrimitive(length) {
     concatBufferHV_.set(
-        HermesValue::encodeObjectValue(*concatBuffer), runtime.getHeap());
+        HermesValue::encodeObjectValue(*concatBuffer), runtime.getHeap(), this);
     assert(
         concatBuffer->contents_.size() >= length &&
         "length exceeds size of concatenation buffer");

lib/VM/ArrayStorage.cpp

@@ -185,23 +185,26 @@ ExecutionStatus ArrayStorageBase<HVType>::shift(
           self->data() + fromFirst,
           self->data() + fromFirst + copySize,
           self->data() + toFirst,
-          runtime.getHeap());
+          runtime.getHeap(),
+          self);
     } else if (fromFirst < toFirst) {
       // Copying to the right, need to copy backwards to avoid overwriting what
       // is being copied.
       GCHVType::copy_backward(
           self->data() + fromFirst,
           self->data() + fromFirst + copySize,
           self->data() + toFirst + copySize,
-          runtime.getHeap());
+          runtime.getHeap(),
+          self);
     }
 
     // Initialize the elements which were emptied in front.
     GCHVType::fill(
         self->data(),
         self->data() + toFirst,
         HVType::encodeEmptyValue(),
-        runtime.getHeap());
+        runtime.getHeap(),
+        self);
 
     // Initialize the elements between the last copied element and toLast.
     if (toFirst + copySize < toLast) {

lib/VM/Interpreter.cpp

@@ -2123,16 +2123,16 @@ CallResult<HermesValue> Interpreter::interpretFunction(
       }
 
       CASE(StoreToEnvironment) {
-        vmcast<Environment>(O1REG(StoreToEnvironment))
-            ->slot(ip->iStoreToEnvironment.op2)
-            .set(O3REG(StoreToEnvironment), runtime.getHeap());
+        auto *environment = vmcast<Environment>(O1REG(StoreToEnvironment));
+        environment->slot(ip->iStoreToEnvironment.op2)
+            .set(O3REG(StoreToEnvironment), runtime.getHeap(), environment);
         ip = NEXTINST(StoreToEnvironment);
         DISPATCH;
       }
       CASE(StoreToEnvironmentL) {
-        vmcast<Environment>(O1REG(StoreToEnvironmentL))
-            ->slot(ip->iStoreToEnvironmentL.op2)
-            .set(O3REG(StoreToEnvironmentL), runtime.getHeap());
+        auto *environment = vmcast<Environment>(O1REG(StoreToEnvironmentL));
+        environment->slot(ip->iStoreToEnvironmentL.op2)
+            .set(O3REG(StoreToEnvironmentL), runtime.getHeap(), environment);
         ip = NEXTINST(StoreToEnvironmentL);
         DISPATCH;
       }

lib/VM/JSObject.cpp

@@ -237,7 +237,8 @@ void JSObject::allocateNewSlotStorage(
   // If it is a direct property, just store the value and we are done.
   if (LLVM_LIKELY(newSlotIndex < DIRECT_PROPERTY_SLOTS)) {
     auto shv = SmallHermesValue::encodeHermesValue(*valueHandle, runtime);
-    selfHandle->directProps()[newSlotIndex].set(shv, runtime.getHeap());
+    selfHandle->directProps()[newSlotIndex].set(
+        shv, runtime.getHeap(), *selfHandle);
     return;
   }
 

lib/VM/OrderedHashMap.cpp

@@ -246,7 +246,7 @@ ExecutionStatus OrderedHashMapBase<BucketType, Derived>::insert(
         self->lookupInBucket(runtime, bucket, key.getHermesValue());
     if (entry) {
       // Element for the key already exists, update value and return.
-      entry->value.set(shv, runtime.getHeap());
+      entry->value.set(shv, runtime.getHeap(), entry);
       return ExecutionStatus::RETURNED;
     }
   }
@@ -313,11 +313,11 @@ ExecutionStatus OrderedHashMapBase<BucketType, Derived>::doInsert(
   // call it and set to newMapEntry one at a time.
   auto newMapEntry = runtime.makeHandle(std::move(*crtRes));
   auto k = SmallHermesValue::encodeHermesValue(key.getHermesValue(), runtime);
-  newMapEntry->key.set(k, runtime.getHeap());
+  newMapEntry->key.set(k, runtime.getHeap(), *newMapEntry);
   if constexpr (std::is_same_v<BucketType, HashMapEntry>) {
     auto v =
         SmallHermesValue::encodeHermesValue(value.getHermesValue(), runtime);
-    newMapEntry->value.set(v, runtime.getHeap());
+    newMapEntry->value.set(v, runtime.getHeap(), *newMapEntry);
   }
 
   // After here, no allocation