Improve max capacity enforcement in DictPropertyMap

Summary:
Previously DictPropertyMap was enforcing its max capacity limitation by
calculating the requested allocation size and comparing it against the
maximum possible allocation size. Unfortunately that was susceptible to
integer overflow.

Secondly, since the maximum capacity was only indirectly expressed as
memory size instead of directly as number of entries, it was impossible
to use in resizing logic. After doubling the capacity, if it happened to
be larger that the maximum, we would simply fail instead of capping it
at the maximum, since we didn't actually know the maximum. For example,
when adding properties sequentially, we would eventually try to double
the capacity from 130,000 to 260,000 properties and fail (this is with
GenGCNC and 4MB segment size). However theoretically we could have
instead increased the capacity to 167,000 (which is the maximum that
fits) and added 37,000 more properties.

Address both these shortcomings by calculating the maximum capacity at
compile time. Having a constant available to compare against makes all
overflow checks and resizing logic simple and efficient.

Calculating the maximum capacity at compile time is not trivial since it
depends on several constants, some of which are only implicitly known
by the compiler (like alignment) and is not a simple equation. So, we
perform a binary search over the solution space (all sizes between 0 and
0xFFFFFFFF >> 1) in a recursive constexpr function.

A new test was added. To avoid excessive runtime and mmeory with
collectors other than GenGCNC, it doesn't do anything if the maximum
capacity is over 500,000.

Reviewed By: amnn

Differential Revision: D14162545

fbshipit-source-id: 60e4049c9dab35cc9e37d3a9fcad849b0e13a96c

include/hermes/VM/DictPropertyMap.h

@@ -94,21 +94,13 @@ class DictPropertyMap final : public VariableSizeRuntimeCell,
     return cell->getKind() == CellKind::DictPropertyMapKind;
   }
 
+  /// Return the maximum possible capacity of DictPropMap.
+  static size_type getMaxCapacity();
+
   /// Create an instance of DictPropertyMap with the specified capacity.
   static CallResult<PseudoHandle<DictPropertyMap>> create(
       Runtime *runtime,
-      size_type capacity = DEFAULT_CAPACITY) {
-    if (LLVM_UNLIKELY(!wouldFit(capacity))) {
-      return runtime->raiseRangeError(
-          TwineChar16("Property storage can't accommodate ") + capacity +
-          " properties");
-    }
-    size_type hashCapacity = calcHashCapacity(capacity);
-    void *mem = runtime->alloc</*fixedSize*/ false>(
-        allocationSize(capacity, hashCapacity));
-    return createPseudoHandle(
-        new (mem) DictPropertyMap(runtime, capacity, hashCapacity));
-  }
+      size_type capacity = DEFAULT_CAPACITY);
 
   /// Return the number of non-deleted properties in the map.
   size_type size() const {
@@ -184,10 +176,6 @@ class DictPropertyMap final : public VariableSizeRuntimeCell,
   /// which is the next slot at the end of the currently allocated storage.
   static SlotIndex allocatePropertySlot(DictPropertyMap *self);
 
-  /// \return true iff a DictPropertyMap of the given capacity would be too big
-  ///   to fit in the GC.
-  static bool wouldFit(size_type capacity);
-
   void dump();
 
  private:
@@ -221,10 +209,26 @@ class DictPropertyMap final : public VariableSizeRuntimeCell,
   /// Number of entries in the deleted list.
   size_type deletedListSize_{0};
 
-  /// Derive the size of the hash table so it can hold \p c elements without
+  /// Derive the size of the hash table so it can hold \p cap elements without
   /// many collisions. The result must also be a power of 2.
-  static size_type calcHashCapacity(size_type c) {
-    return llvm::NextPowerOf2(c * 4 / 3 + 1);
+  static size_type calcHashCapacity(size_type cap) {
+    assert(
+        (cap <= std::numeric_limits<size_type>::max() / 4) &&
+        "size will cause integer overflow in calcHashCapacity");
+
+    return llvm::PowerOf2Ceil(cap * 4 / 3 + 1);
+  }
+
+  /// A const-expr version of \c calcHashCapacity() using 64-bit arithmetic.
+  /// NOTE: it must not be used at runtime since it might be slow.
+  static constexpr uint64_t constCalcHashCapacity64(uint64_t cap) {
+    return constPowerOf2Ceil(cap * 4 / 3 + 1);
+  }
+
+  /// A constexpr compatible version of llvm::PowerOf2Ceil().
+  /// NOTE: it must not be used at runtime since it might be slow.
+  static constexpr uint64_t constPowerOf2Ceil(uint64_t A, uint64_t ceil = 1) {
+    return ceil >= A ? ceil : constPowerOf2Ceil(A, ceil << 1);
   }
 
   /// Hash a symbol ID. For now it is the identity hash.
@@ -291,7 +295,8 @@ class DictPropertyMap final : public VariableSizeRuntimeCell,
       SymbolID symbolID);
 
   /// Allocate a new property map with the specified capacity, copy the existing
-  /// valid entries into it.
+  /// valid entries into it. If the specified capacity exceeds the maximum
+  /// possible capacity, an exception will be raised.
   /// \param[in,out] selfHandleRef the original object handle on input, the new
   ///   object handle on output.
   /// \param newCapacity the capacity of the new object's descriptor array.
@@ -308,6 +313,67 @@ class DictPropertyMap final : public VariableSizeRuntimeCell,
         descriptorCapacity, hashCapacity);
   }
 
+  /// Calculate the maximum capacity of DictPropertyMap at compile time using
+  /// binary search in the solution space, since we can't solve the equation
+  /// directly.
+  ///
+  /// Some of these constexpr functions can be made more readable and or more
+  /// efficient c++14 by using variables (T31421960).
+  /// @{
+
+  /// The maximum alignment padding a compiler might insert before a field or at
+  /// the end of a struct. We use this for a conservative (but reasonable)
+  /// estimate of the allocation size of the object.
+  static constexpr uint64_t kAlignPadding = alignof(std::max_align_t) - 1;
+
+  /// Calculate a conservative approximate size of DictPropertyMap in bytes,
+  /// given a capacity. The calculation is performed using 64-bit arithmetic to
+  /// avoid overflow.
+  /// NOTE: it must not be used at runtime since it might be slow.
+  static constexpr uint64_t constApproxAllocSize64(uint32_t cap) {
+    static_assert(
+        alignof(DictPropertyMap) <= kAlignPadding + 1,
+        "DictPropertyMap exceeds supported alignment");
+    static_assert(
+        alignof(DictPropertyMap::DescriptorPair) <= kAlignPadding + 1,
+        "DictPropertyMap::DescriptorPair exceeds supported alignment");
+    static_assert(
+        alignof(DictPropertyMap::HashPair) <= kAlignPadding + 1,
+        "DictPropertyMap::HashPair exceeds supported alignment");
+
+    return sizeof(DictPropertyMap) + kAlignPadding +
+        sizeof(DictPropertyMap::DescriptorPair) * (uint64_t)cap +
+        kAlignPadding +
+        sizeof(DictPropertyMap::HashPair) * constCalcHashCapacity64(cap) +
+        kAlignPadding;
+  }
+
+  /// Return true if DictPropertyMap with the specified capacity is guaranteed
+  /// to fit within the GC's maxumum allocation size. The check is conservative:
+  /// it might a few return false negatives at the end of the range.
+  /// NOTE: it must not be used at runtime since it might be slow.
+  static constexpr bool constWouldFitAllocation(uint32_t cap) {
+    return constApproxAllocSize64(cap) <= GC::maxAllocationSize();
+  }
+
+  /// In the range of capacity values [first ... first + len), find the largest
+  /// value for which wouldFitAllocation() returns true.
+  /// NOTE: it must not be used at runtime since it might be slow.
+  static constexpr uint32_t constFindMaxCapacity(uint32_t first, uint32_t len) {
+    return len == 0
+        ? first - 1
+        : (constWouldFitAllocation(first + len / 2)
+               ? constFindMaxCapacity(first + len / 2 + 1, len - len / 2 - 1)
+               : constFindMaxCapacity(first, len / 2));
+  }
+
+  /// A place to put things in order to avoid restructins on using constexpr
+  /// functions declared in the same class.
+  struct detail;
+  friend struct detail;
+
+  /// @}
+
  protected:
   size_t numTrailingObjects(OverloadToken<DescriptorPair>) const {
     return descriptorCapacity_;
@@ -409,12 +475,6 @@ inline ExecutionStatus DictPropertyMap::add(
   return ExecutionStatus::RETURNED;
 }
 
-/* static */
-inline bool DictPropertyMap::wouldFit(size_type capacity) {
-  return allocationSize(capacity, calcHashCapacity(capacity)) <=
-      GC::maxAllocationSize();
-}
-
 } // namespace vm
 } // namespace hermes
 

lib/VM/DictPropertyMap.cpp

@@ -14,6 +14,36 @@ HERMES_SLOW_STATISTIC(NumExtraHashProbes, "Number of extra hash probes");
 namespace hermes {
 namespace vm {
 
+struct DictPropertyMap::detail {
+  /// The upper bound of the search when trying to find the maximum capacity
+  /// of this object, given GC::maxAllocationSize().
+  /// It was chosen to be a value that is certain to not fit into an allocation;
+  /// at the same time we want to make it smaller, so/ we have arbitrarily
+  /// chosen to divide the max allocation size by two, which is still guaranteed
+  /// not to fit.
+  static constexpr uint32_t kSearchUpperBound = GC::maxAllocationSize() / 2;
+
+  static_assert(
+      !DictPropertyMap::constWouldFitAllocation(kSearchUpperBound),
+      "kSearchUpperBound should not fit into an allocation");
+
+  /// The maximum capacity of DictPropertyMap, given GC::maxAllocationSize().
+  static constexpr uint32_t kMaxCapacity =
+      DictPropertyMap::constFindMaxCapacity(0, kSearchUpperBound);
+
+  // Double-check that kMaxCapacity is reasonable.
+  static_assert(
+      DictPropertyMap::constApproxAllocSize64(kMaxCapacity) <=
+          GC::maxAllocationSize(),
+      "invalid kMaxCapacity");
+
+  // Ensure that it is safe to double capacities without checking for overflow
+  // until we exceed kMaxCapacity.
+  static_assert(
+      kMaxCapacity < (1u << 31),
+      "kMaxCapacity is unrealistically large");
+};
+
 VTable DictPropertyMap::vt{CellKind::DictPropertyMapKind, 0};
 
 void DictPropertyMapBuildMeta(const GCCell *cell, Metadata::Builder &mb) {
@@ -24,6 +54,25 @@ void DictPropertyMapBuildMeta(const GCCell *cell, Metadata::Builder &mb) {
       sizeof(DictPropertyMap::DescriptorPair));
 }
 
+DictPropertyMap::size_type DictPropertyMap::getMaxCapacity() {
+  return detail::kMaxCapacity;
+}
+
+CallResult<PseudoHandle<DictPropertyMap>> DictPropertyMap::create(
+    Runtime *runtime,
+    size_type capacity) {
+  if (LLVM_UNLIKELY(capacity > detail::kMaxCapacity)) {
+    return runtime->raiseRangeError(
+        TwineChar16("Property storage exceeds ") + detail::kMaxCapacity +
+        " properties");
+  }
+  size_type hashCapacity = calcHashCapacity(capacity);
+  void *mem = runtime->alloc</*fixedSize*/ false>(
+      allocationSize(capacity, hashCapacity));
+  return createPseudoHandle(
+      new (mem) DictPropertyMap(runtime, capacity, hashCapacity));
+}
+
 std::pair<bool, DictPropertyMap::HashPair *> DictPropertyMap::lookupEntryFor(
     DictPropertyMap *self,
     SymbolID symbolID) {
@@ -152,13 +201,26 @@ DictPropertyMap::findOrAdd(
   // sufficient to only check for the latter.
 
   if (self->numDescriptors_ == self->descriptorCapacity_) {
+    size_type newCapacity;
+    if (self->numProperties_ == self->descriptorCapacity_) {
+      // Double the new capacity, up to kMaxCapacity. However make sure that
+      // we try to allocate at least one extra property. If we are already
+      // exactly at kMaxCapacity, there is nothing we can do, so grow() will
+      // simply fail.
+      newCapacity = self->numProperties_ * 2;
+      if (newCapacity > detail::kMaxCapacity)
+        newCapacity =
+            std::max(toRValue(detail::kMaxCapacity), self->numProperties_ + 1);
+    } else {
+      // Calculate the new capacity to be exactly as much as we need to
+      // accomodate the deleted list plus one extra property. It it happens
+      // to exceed kMaxCapacity, there is nothing we can do, so grow() will
+      // raise an exception.
+      newCapacity = self->numProperties_ + 1 + self->deletedListSize_;
+    }
+
     if (LLVM_UNLIKELY(
-            grow(
-                selfHandleRef,
-                runtime,
-                self->numProperties_ == self->descriptorCapacity_
-                    ? self->numProperties_ * 2
-                    : self->numProperties_ + 1 + self->deletedListSize_) ==
+            grow(selfHandleRef, runtime, newCapacity) ==
             ExecutionStatus::EXCEPTION)) {
       return ExecutionStatus::EXCEPTION;
     }

lib/VM/HiddenClass.cpp

@@ -690,7 +690,7 @@ void HiddenClass::initializeMissingPropertyMap(
   }
 
   assert(
-      DictPropertyMap::wouldFit(entries.size()) &&
+      entries.size() <= DictPropertyMap::getMaxCapacity() &&
       "There shouldn't ever be this many properties");
   // Allocate the map with the correct size.
   auto res = DictPropertyMap::create(

unittests/VMRuntime/DictPropertyMapTest.cpp

@@ -129,4 +129,53 @@ TEST_F(DictPropertyMapTest, SmokeTest) {
         });
   }
 }
+
+TEST_F(DictPropertyMapTest, CreateOverCapacityTest) {
+  (void)DictPropertyMap::create(runtime);
+  ASSERT_EQ(
+      ExecutionStatus::EXCEPTION,
+      DictPropertyMap::create(runtime, DictPropertyMap::getMaxCapacity() + 1));
+}
+
+TEST_F(DictPropertyMapTest, GrowOverCapacityTest) {
+  // Don't do the test if it requires too many properties. Just cross our
+  // fingers and hope it works.
+  auto const maxCapacity = DictPropertyMap::getMaxCapacity();
+  if (maxCapacity > 500000)
+    return;
+
+  auto res = DictPropertyMap::create(runtime);
+  ASSERT_RETURNED(res);
+  MutableHandle<DictPropertyMap> map{runtime, res->get()};
+
+  MutableHandle<> value{runtime};
+  NamedPropertyDescriptor desc(PropertyFlags{}, 0);
+
+  auto marker = gcScope.createMarker();
+  for (unsigned i = 0; i < maxCapacity; ++i) {
+    value.set(HermesValue::encodeNumberValue(i));
+    auto symRes = valueToSymbolID(runtime, value);
+    ASSERT_RETURNED(symRes);
+    ASSERT_RETURNED(DictPropertyMap::add(map, runtime, **symRes, desc));
+
+    gcScope.flushToMarker(marker);
+  }
+
+  value.set(HermesValue::encodeNumberValue(maxCapacity));
+  auto symRes = valueToSymbolID(runtime, value);
+  ASSERT_RETURNED(symRes);
+  ASSERT_EQ(
+      ExecutionStatus::EXCEPTION,
+      DictPropertyMap::add(map, runtime, **symRes, desc));
+  runtime->clearThrownValue();
+
+  // Try it again.
+  value.set(HermesValue::encodeNumberValue(maxCapacity + 1));
+  symRes = valueToSymbolID(runtime, value);
+  ASSERT_RETURNED(symRes);
+  ASSERT_EQ(
+      ExecutionStatus::EXCEPTION,
+      DictPropertyMap::add(map, runtime, **symRes, desc));
+  runtime->clearThrownValue();
+}
 } // namespace

unittests/VMRuntime/TestHelpers.h

@@ -33,7 +33,7 @@ static constexpr uint32_t kMaxHeapSmall = 1 << 11;
 static constexpr uint32_t kInitHeapSize = 1 << 16;
 static constexpr uint32_t kMaxHeapSize = 1 << 19;
 static constexpr uint32_t kInitHeapLarge = 1 << 20;
-static constexpr uint32_t kMaxHeapLarge = 1 << 23;
+static constexpr uint32_t kMaxHeapLarge = 1 << 24;
 
 static const GCConfig::Builder kTestGCConfigBuilder =
     GCConfig::Builder()