Add AtomicIfConcurrentGC for array lengths

Summary:
In Hades, any variable-sized object that can change its length after construction
must make sure to do so in a way that the GC doesn't become corrupted.
There are two cases:
* The length is being shortened. The conservative thing to do here is have the GC keep marking the
now-unreachable objects as if they were live. Semantically this is equivalent to writing "empty" into
each slot between `[newLength, oldLength)` (with a corresponding write barrier)
before shortening the length
* The length is being increased. In this case the memory must be initialized before increasing
the length, so that the GC doesn't read garbage memory. After an initial empty init, then the length increase,
real elements can be placed and a normal write barrier must be done.

There are some minor consequences for perf in `ArrayStorage` and `SegmentedArray`, but I
tried to limit it to only when Hades is enabled.

Reviewed By: davedets

Differential Revision: D21485757

fbshipit-source-id: f0a39cb8ef001bf428858b7241eb3b9bd0597604

include/hermes/VM/ArrayStorage.h

@@ -115,22 +115,22 @@ class ArrayStorage final
   /// \return a reference to the element at index \p index
   template <Inline inl = Inline::No>
   GCHermesValue &at(size_type index) {
-    assert(index < size_ && "index out of range");
+    assert(index < size() && "index out of range");
     return data()[index];
   }
 
   size_type capacity() const {
     return capacity_;
   }
   size_type size() const {
-    return size_;
+    return size_.load(std::memory_order_relaxed);
   }
 
   iterator begin() {
     return data();
   }
   iterator end() {
-    return data() + size_;
+    return data() + size();
   }
 
   /// Append the given element to the end (increasing size by 1).
@@ -139,20 +139,22 @@ class ArrayStorage final
       Runtime *runtime,
       Handle<> value) {
     auto *self = selfHandle.get();
-    if (LLVM_LIKELY(self->size_ < self->capacity_)) {
+    const auto currSz = self->size();
+    if (LLVM_LIKELY(currSz < self->capacity_)) {
       // Use the constructor of GCHermesValue to use the correct write barrier
       // for uninitialized memory.
-      new (&self->data()[self->size_++])
+      new (&self->data()[currSz])
           GCHermesValue(value.get(), &runtime->getHeap());
+      self->size_.store(currSz + 1, std::memory_order_release);
       return ExecutionStatus::RETURNED;
     }
     return pushBackSlowPath(selfHandle, runtime, value);
   }
 
   /// Pop the last element off the array and return it.
   HermesValue pop_back() {
-    assert(size_ > 0 && "Can't pop from empty ArrayStorage");
-    return data()[--size_];
+    assert(size() > 0 && "Can't pop from empty ArrayStorage");
+    return data()[size_.fetch_sub(1, std::memory_order_relaxed) - 1];
   }
 
   /// Ensure that the capacity of the array is at least \p capacity,
@@ -182,7 +184,7 @@ class ArrayStorage final
       MutableHandle<ArrayStorage> &selfHandle,
       Runtime *runtime,
       size_type newSize) {
-    return shift(selfHandle, runtime, 0, newSize - selfHandle->size_, newSize);
+    return shift(selfHandle, runtime, 0, newSize - selfHandle->size(), newSize);
   }
 
   /// Set the size to a value <= the capacity. This is a special
@@ -197,7 +199,7 @@ class ArrayStorage final
   /// shrinking during a GC compaction. In order to increase the capacity, a new
   /// ArrayStorage must be created.
   size_type capacity_;
-  size_type size_{0};
+  AtomicIfConcurrentGC<size_type> size_{0};
 
   ArrayStorage() = delete;
   ArrayStorage(const ArrayStorage &) = delete;

include/hermes/VM/Callable.h

@@ -29,7 +29,7 @@ class Environment final
   GCPointer<Environment> parentEnvironment_{};
 
   /// Number of entries in the environment.
-  const uint32_t size_;
+  AtomicIfConcurrentGC<uint32_t> size_;
 
  public:
 #ifdef HERMESVM_SERIALIZE
@@ -61,11 +61,11 @@ class Environment final
 
   /// \return the number of HermesValue slots in this environment.
   uint32_t getSize() const {
-    return size_;
+    return size_.load(std::memory_order_relaxed);
   }
 
   GCHermesValue &slot(uint32_t index) {
-    assert(index < size_ && "invalid Environment slot index");
+    assert(index < getSize() && "invalid Environment slot index");
     return getSlots()[index];
   }
 

include/hermes/VM/DictPropertyMap.h

@@ -287,7 +287,7 @@ class DictPropertyMap final : public VariableSizeRuntimeCell,
 
   /// How many entries have been added to the descriptor array (including
   /// deleted).
-  size_type numDescriptors_{0};
+  AtomicIfConcurrentGC<size_type> numDescriptors_{0};
 
   /// Number of valid properties in the map.
   size_type numProperties_{0};
@@ -496,7 +496,11 @@ void DictPropertyMap::forEachProperty(
     Runtime *runtime,
     const CallbackFunction &callback) {
   GCScopeMarkerRAII gcMarker{runtime};
-  for (size_type i = 0, e = selfHandle->numDescriptors_; i != e; ++i) {
+  for (size_type
+           i = 0,
+           e = selfHandle->numDescriptors_.load(std::memory_order_relaxed);
+       i != e;
+       ++i) {
     auto const *descPair = selfHandle->getDescriptorPairs() + i;
     if (descPair->first.isValid()) {
       callback(descPair->first, descPair->second);
@@ -509,7 +513,10 @@ template <typename CallbackFunction>
 void DictPropertyMap::forEachPropertyNoAlloc(
     DictPropertyMap *self,
     const CallbackFunction &callback) {
-  for (size_type i = 0, e = self->numDescriptors_; i != e; ++i) {
+  for (size_type i = 0,
+                 e = self->numDescriptors_.load(std::memory_order_relaxed);
+       i != e;
+       ++i) {
     auto const *descPair = self->getDescriptorPairs() + i;
     if (descPair->first.isValid()) {
       callback(descPair->first, descPair->second);
@@ -523,7 +530,11 @@ bool DictPropertyMap::forEachPropertyWhile(
     Runtime *runtime,
     const CallbackFunction &callback) {
   GCScopeMarkerRAII gcMarker{runtime};
-  for (size_type i = 0, e = selfHandle->numDescriptors_; i != e; ++i) {
+  for (size_type
+           i = 0,
+           e = selfHandle->numDescriptors_.load(std::memory_order_relaxed);
+       i != e;
+       ++i) {
     auto const *descPair = selfHandle->getDescriptorPairs() + i;
     if (descPair->first.isValid()) {
       if (!callback(runtime, descPair->first, descPair->second))
@@ -539,7 +550,11 @@ void DictPropertyMap::forEachMutablePropertyDescriptor(
     Handle<DictPropertyMap> selfHandle,
     Runtime *runtime,
     const CallbackFunction &callback) {
-  for (size_type i = 0, e = selfHandle->numDescriptors_; i != e; ++i) {
+  for (size_type
+           i = 0,
+           e = selfHandle->numDescriptors_.load(std::memory_order_relaxed);
+       i != e;
+       ++i) {
     auto *descPair = selfHandle->getDescriptorPairs() + i;
     if (descPair->first.isValid()) {
       callback(descPair->second);
@@ -555,7 +570,9 @@ inline DictPropertyMap::DescriptorPair *DictPropertyMap::getDescriptorPair(
 
   auto *hashPair = self->getHashPairs() + pos.hashPairIndex;
   auto descIndex = hashPair->getDescIndex();
-  assert(descIndex < self->numDescriptors_ && "descriptor index out of range");
+  assert(
+      descIndex < self->numDescriptors_.load(std::memory_order_relaxed) &&
+      "descriptor index out of range");
 
   auto *res = self->getDescriptorPairs() + descIndex;
   assert(hashPair->mayBe(res->first) && "accessing incorrect descriptor pair");

include/hermes/VM/GCDecl.h

@@ -8,6 +8,8 @@
 #ifndef HERMES_VM_GCDECL_H
 #define HERMES_VM_GCDECL_H
 
+#include <atomic>
+
 #ifdef HERMESVM_GC_HADES
 #include <mutex>
 #include <thread>
@@ -60,6 +62,9 @@ using WeakRefLock = std::lock_guard<DebugMutex>;
 using WeakRefMutex = std::mutex;
 using WeakRefLock = std::lock_guard<std::mutex>;
 #endif
+/// HadesGC requires some fields to be atomic, such as array lengths.
+template <typename T>
+using AtomicIfConcurrentGC = std::atomic<T>;
 #else
 /// Non-concurrent GCs don't need any locks in order to have correct
 /// semantics for WeakRef, so use a cheap type to pass around instead.
@@ -84,6 +89,58 @@ static_assert(
     "FakeLockGuard must not be trivially destructible to avoid warnings about "
     "unused variables");
 
+/// FakeAtomic has the same API as std::atomic, but ignores the memory order
+/// argument and always accesses data non-atomically.
+/// Used when the GC doesn't require atomicity.
+/// In the JS VM, there is currently only one mutator thread and at most one GC
+/// thread. The GC thread will not do any modifications to these atomics, and
+/// will only read them. Therefore it is typically safe for the mutator to use
+/// relaxed reads. Writes will typically require std::memory_order_release or
+/// stricter to make sure the GC sees the writes which occur before the atomic
+/// write.
+/// NOTE: This differs from std::atomic where it doesn't have default memory
+/// orders, since we want all atomic operations to be very explicit with their
+/// requirements. Also don't define operator T for the same reason.
+template <typename T>
+class FakeAtomic final {
+ public:
+  constexpr explicit FakeAtomic() : data_{} {}
+  constexpr explicit FakeAtomic(T desired) : data_{desired} {}
+
+  T load(std::memory_order order) const {
+    (void)order;
+    return data_;
+  }
+
+  void store(T desired, std::memory_order order) {
+    (void)order;
+    data_ = desired;
+  }
+
+  T fetch_add(T arg, std::memory_order order) {
+    (void)order;
+    const T oldData = data_;
+    data_ += arg;
+    return oldData;
+  }
+
+  T fetch_sub(T arg, std::memory_order order) {
+    (void)order;
+    const T oldData = data_;
+    data_ -= arg;
+    return oldData;
+  }
+
+  /// Use store explicitly instead.
+  FakeAtomic &operator=(const FakeAtomic &) = delete;
+
+ private:
+  T data_;
+};
+
+/// No need for atomics with non-Hades GCs.
+template <typename T>
+using AtomicIfConcurrentGC = FakeAtomic<T>;
 #endif
 
 #if defined(HERMESVM_GC_MALLOC)

include/hermes/VM/Metadata.h

@@ -112,17 +112,17 @@ struct Metadata final {
     /// \p lengthLocation The location of the size of the array.
     /// The \c ArrayType parameter denotes which type of elements are in the
     /// array.
-    template <ArrayData::ArrayType type, typename SizeType>
+    template <ArrayData::ArrayType type>
     inline void addArray(
         const void *startLocation,
-        const SizeType *lengthLocation,
+        const AtomicIfConcurrentGC<uint32_t> *lengthLocation,
         std::size_t stride);
 
-    template <ArrayData::ArrayType type, typename SizeType>
+    template <ArrayData::ArrayType type>
     inline void addArray(
         const char *name,
         const void *startLocation,
-        const SizeType *lengthLocation,
+        const AtomicIfConcurrentGC<uint32_t> *lengthLocation,
         std::size_t stride);
 
     /// Should be called first when building metadata for a JSObject subclass.
@@ -203,23 +203,20 @@ llvm::raw_ostream &operator<<(
 /// @name Inline implementations
 /// @{
 
-template <Metadata::ArrayData::ArrayType type, typename SizeType>
+template <Metadata::ArrayData::ArrayType type>
 inline void Metadata::Builder::addArray(
     const void *startLocation,
-    const SizeType *lengthLocation,
+    const AtomicIfConcurrentGC<uint32_t> *lengthLocation,
     std::size_t stride) {
-  addArray<type, SizeType>(nullptr, startLocation, lengthLocation, stride);
+  addArray<type>(nullptr, startLocation, lengthLocation, stride);
 }
 
-template <Metadata::ArrayData::ArrayType type, typename SizeType>
+template <Metadata::ArrayData::ArrayType type>
 inline void Metadata::Builder::addArray(
     const char *name,
     const void *startLocation,
-    const SizeType *lengthLocation,
+    const AtomicIfConcurrentGC<uint32_t> *lengthLocation,
     std::size_t stride) {
-  static_assert(
-      (sizeof(SizeType) == sizeof(std::int32_t)),
-      "The size parameter of an array should only be a 32 bit value");
   array_ = ArrayData(
       type,
       reinterpret_cast<const char *>(startLocation) - base_,

include/hermes/VM/SegmentedArray.h

@@ -59,21 +59,21 @@ class SegmentedArray final
 
     GCHermesValue &at(uint32_t index) {
       assert(
-          index < length_ &&
+          index < length() &&
           "Cannot get an index outside of the length of a segment");
       return data_[index];
     }
 
     /// \p const version of \p at.
     const GCHermesValue &at(uint32_t index) const {
       assert(
-          index < length_ &&
+          index < length() &&
           "Cannot get an index outside of the length of a segment");
       return data_[index];
     }
 
     uint32_t length() const {
-      return length_;
+      return length_.load(std::memory_order_relaxed);
     }
 
     /// Increases or decreases the length of the segment, up to a max of
@@ -87,7 +87,7 @@ class SegmentedArray final
     /// by the GC).
     void setLengthWithoutFilling(uint32_t newLength) {
       assert(newLength <= kMaxLength && "Cannot set length to more than size");
-      length_ = newLength;
+      length_.store(newLength, std::memory_order_release);
     }
 
    private:
@@ -101,7 +101,7 @@ class SegmentedArray final
     friend void SegmentBuildMeta(const GCCell *cell, Metadata::Builder &mb);
     static VTable vt;
 
-    uint32_t length_;
+    AtomicIfConcurrentGC<uint32_t> length_;
     GCHermesValue data_[kMaxLength];
 
     explicit Segment(Runtime *runtime)
@@ -146,7 +146,7 @@ class SegmentedArray final
   size_type slotCapacity_;
   /// The number of slots that are currently valid. The \c size() is a derived
   /// field from this value.
-  size_type numSlotsUsed_;
+  AtomicIfConcurrentGC<size_type> numSlotsUsed_;
 
   struct iterator {
     using iterator_category = std::bidirectional_iterator_tag;
@@ -281,11 +281,11 @@ class SegmentedArray final
   /// Gets the size of the SegmentedArray. The size is the number of elements
   /// currently active in the array.
   size_type size() const {
-    if (LLVM_LIKELY(numSlotsUsed_ <= kValueToSegmentThreshold)) {
-      return numSlotsUsed_;
+    const auto numSlotsUsed = numSlotsUsed_.load(std::memory_order_relaxed);
+    if (LLVM_LIKELY(numSlotsUsed <= kValueToSegmentThreshold)) {
+      return numSlotsUsed;
     } else {
-      const SegmentNumber numSegments =
-          numSlotsUsed_ - kValueToSegmentThreshold;
+      const SegmentNumber numSegments = numSlotsUsed - kValueToSegmentThreshold;
       const size_type numBeforeLastSegment =
           kValueToSegmentThreshold + (numSegments - 1) * Segment::kMaxLength;
       const uint32_t numInLastSegment = segmentAt(numSegments - 1)->length();
@@ -526,9 +526,10 @@ class SegmentedArray final
 
   /// \return the number of segments in active use by this SegmentedArray.
   SegmentNumber numUsedSegments() const {
-    return numSlotsUsed_ <= kValueToSegmentThreshold
+    const auto numSlotsUsed = numSlotsUsed_.load(std::memory_order_relaxed);
+    return numSlotsUsed <= kValueToSegmentThreshold
         ? 0
-        : numSlotsUsed_ - kValueToSegmentThreshold;
+        : numSlotsUsed - kValueToSegmentThreshold;
   }
 
   /// @name Resize helper functions

include/hermes/VM/SlotVisitor.h

@@ -9,6 +9,7 @@
 #define HERMES_VM_SLOTVISITOR_H
 
 #include "hermes/VM/GCCell.h"
+#include "hermes/VM/GCDecl.h"
 #include "hermes/VM/GCPointer.h"
 #include "hermes/VM/HermesValue.h"
 #include "hermes/VM/Metadata.h"
@@ -25,8 +26,11 @@ class BaseVisitor {
   visitArray(Acceptor &acceptor, char *base, const Metadata::ArrayData &array) {
     using ArrayType = Metadata::ArrayData::ArrayType;
     char *start = base + array.startOffset;
-    const auto length =
-        *reinterpret_cast<std::uint32_t *>(base + array.lengthOffset);
+    // Load the length, making sure all dependent writes have completed with
+    // memory_order_acquire.
+    const auto length = reinterpret_cast<AtomicIfConcurrentGC<std::uint32_t> *>(
+                            base + array.lengthOffset)
+                            ->load(std::memory_order_acquire);
     const auto stride = array.stride;
     switch (array.type) {
       case ArrayType::Pointer: