Bug 1481138 - Remove Hash{Map,Set}::putNewInfallible. r=luke draft
authorNicholas Nethercote <nnethercote@mozilla.com>
Mon, 06 Aug 2018 11:52:17 +1000
changeset 826906 d09008e7d9d525043cd5399374f942fbabd5e2c7
parent 826905 9d96ceec60c7d9a25a3ca643fd52adbcff26cae8
child 826907 3a061c9a611bbbe2ba914f9d2afbc6ca47eb7d3f
push id118403
push usernnethercote@mozilla.com
push dateMon, 06 Aug 2018 07:02:00 +0000
reviewersluke
bugs1481138
milestone63.0a1
Bug 1481138 - Remove Hash{Map,Set}::putNewInfallible. r=luke The difference between putNew() and putNewInfallible() is subtle -- the latter omits three checks. Omitting two of them (SimulatedOOM and checkOverloaded()) is reasonable, but omitting the EnsureHash() check is dubious. Furthermore, putNewInfallible() has very few uses; in my opinion, not enough to warrant its existence. So this patch removes it, simplifying the API and the implementation. MozReview-Commit-ID: 3KfpVERhqTh
js/public/UbiNodeDominatorTree.h
js/src/vm/JSScript.cpp
js/src/vm/Shape.cpp
mfbt/HashTable.h
--- a/js/public/UbiNodeDominatorTree.h
+++ b/js/public/UbiNodeDominatorTree.h
@@ -359,17 +359,17 @@ class JS_PUBLIC_API(DominatorTree)
     static MOZ_MUST_USE bool mapNodesToTheirIndices(JS::ubi::Vector<Node>& postOrder,
                                                     NodeToIndexMap& map) {
         MOZ_ASSERT(!map.initialized());
         MOZ_ASSERT(postOrder.length() < UINT32_MAX);
         uint32_t length = postOrder.length();
         if (!map.init(length))
             return false;
         for (uint32_t i = 0; i < length; i++)
-            map.putNewInfallible(postOrder[i], i);
+            MOZ_ALWAYS_TRUE(map.putNew(postOrder[i], i));
         return true;
     }
 
     // Convert the Node -> NodeSet predecessorSets to a index -> Vector<index>
     // form.
     static MOZ_MUST_USE bool convertPredecessorSetsToVectors(
         const Node& root,
         JS::ubi::Vector<Node>& postOrder,
--- a/js/src/vm/JSScript.cpp
+++ b/js/src/vm/JSScript.cpp
@@ -3230,17 +3230,17 @@ js::GetSrcNote(GSNCache& cache, JSScript
         }
         if (cache.map.init(nsrcnotes)) {
             pc = script->code();
             for (jssrcnote* sn = script->notes(); !SN_IS_TERMINATOR(sn);
                  sn = SN_NEXT(sn))
             {
                 pc += SN_DELTA(sn);
                 if (SN_IS_GETTABLE(sn))
-                    cache.map.putNewInfallible(pc, sn);
+                    MOZ_ALWAYS_TRUE(cache.map.putNew(pc, sn));
             }
             cache.code = script->code();
         }
     }
 
     return result;
 }
 
--- a/js/src/vm/Shape.cpp
+++ b/js/src/vm/Shape.cpp
@@ -1628,18 +1628,18 @@ ShapeHasher::match(const Key k, const Lo
 
 static KidsHash*
 HashChildren(Shape* kid1, Shape* kid2)
 {
     auto hash = MakeUnique<KidsHash>();
     if (!hash || !hash->init(2))
         return nullptr;
 
-    hash->putNewInfallible(StackShape(kid1), kid1);
-    hash->putNewInfallible(StackShape(kid2), kid2);
+    MOZ_ALWAYS_TRUE(hash->putNew(StackShape(kid1), kid1));
+    MOZ_ALWAYS_TRUE(hash->putNew(StackShape(kid2), kid2));
     return hash.release();
 }
 
 bool
 PropertyTree::insertChild(JSContext* cx, Shape* parent, Shape* child)
 {
     MOZ_ASSERT(!parent->inDictionary());
     MOZ_ASSERT(!child->parent);
--- a/mfbt/HashTable.h
+++ b/mfbt/HashTable.h
@@ -276,24 +276,16 @@ public:
   // Like put(), but asserts that the given key is not already present.
   template<typename KeyInput, typename ValueInput>
   MOZ_MUST_USE bool putNew(KeyInput&& aKey, ValueInput&& aValue)
   {
     return mImpl.putNew(
       aKey, std::forward<KeyInput>(aKey), std::forward<ValueInput>(aValue));
   }
 
-  // Only call this to populate an empty map after reserving space with init().
-  template<typename KeyInput, typename ValueInput>
-  void putNewInfallible(KeyInput&& aKey, ValueInput&& aValue)
-  {
-    mImpl.putNewInfallible(
-      aKey, std::forward<KeyInput>(aKey), std::forward<ValueInput>(aValue));
-  }
-
   // Like |lookup(l)|, but on miss, |p = lookupForAdd(l)| allows efficient
   // insertion of Key |k| (where |HashPolicy::match(k,l) == true|) using
   // |add(p,k,v)|. After |add(p,k,v)|, |p| points to the new key/value. E.g.:
   //
   //   using HM = HashMap<int,char>;
   //   HM h;
   //   HM::AddPtr p = h.lookupForAdd(3);
   //   if (!p) {
@@ -578,23 +570,16 @@ public:
 
   // Like the other putNew(), but for when |Lookup| is different to |T|.
   template<typename U>
   MOZ_MUST_USE bool putNew(const Lookup& aLookup, U&& aU)
   {
     return mImpl.putNew(aLookup, std::forward<U>(aU));
   }
 
-  // Only call this to populate an empty set after reserving space with init().
-  template<typename U>
-  void putNewInfallible(const Lookup& aLookup, U&& aU)
-  {
-    mImpl.putNewInfallible(aLookup, std::forward<U>(aU));
-  }
-
   // Like |lookup(l)|, but on miss, |p = lookupForAdd(l)| allows efficient
   // insertion of T value |t| (where |HashPolicy::match(t,l) == true|) using
   // |add(p,t)|. After |add(p,t)|, |p| points to the new element. E.g.:
   //
   //   using HS = HashSet<int>;
   //   HS h;
   //   HS::AddPtr p = h.lookupForAdd(3);
   //   if (!p) {
@@ -1969,20 +1954,19 @@ private:
     // collision bits correctly on a subsequent pass or skipping the rehash
     // unless we are totally filled with tombstones: benchmark to find out
     // which approach is best.
   }
 
   // Note: |aLookup| may be a reference to a piece of |u|, so this function
   // must take care not to use |aLookup| after moving |u|.
   //
-  // Prefer to use putNewInfallible; this function does not check
-  // invariants.
+  // Prefer to use putNew; this function does not check invariants.
   template<typename... Args>
-  void putNewInfallibleInternal(const Lookup& aLookup, Args&&... aArgs)
+  void putNewInternal(const Lookup& aLookup, Args&&... aArgs)
   {
     MOZ_ASSERT(mTable);
 
     HashNumber keyHash = prepareHash(aLookup);
     Entry* entry = &findFreeEntry(keyHash);
     MOZ_ASSERT(entry);
 
     if (entry->isRemoved()) {
@@ -2166,41 +2150,34 @@ public:
 #ifdef DEBUG
     mMutationCount++;
     aPtr.mGeneration = generation();
     aPtr.mMutationCount = mMutationCount;
 #endif
     return true;
   }
 
-  // Note: |aLookup| may be a reference to a piece of |u|, so this function
-  // must take care not to use |aLookup| after moving |u|.
-  template<typename... Args>
-  void putNewInfallible(const Lookup& aLookup, Args&&... aArgs)
-  {
-    MOZ_ASSERT(!lookup(aLookup).found());
-    ReentrancyGuard g(*this);
-    putNewInfallibleInternal(aLookup, std::forward<Args>(aArgs)...);
-  }
-
   // Note: |aLookup| may be alias arguments in |aArgs|, so this function must
   // take care not to use |aLookup| after moving |aArgs|.
   template<typename... Args>
   MOZ_MUST_USE bool putNew(const Lookup& aLookup, Args&&... aArgs)
   {
+    MOZ_ASSERT(!lookup(aLookup).found());   // must precede the ReentrancyGuard
+    ReentrancyGuard g(*this);
+
     if (!this->checkSimulatedOOM()) {
       return false;
     }
     if (!EnsureHash<HashPolicy>(aLookup)) {
       return false;
     }
     if (checkOverloaded() == RehashFailed) {
       return false;
     }
-    putNewInfallible(aLookup, std::forward<Args>(aArgs)...);
+    putNewInternal(aLookup, std::forward<Args>(aArgs)...);
     return true;
   }
 
   // Note: |aLookup| may be a reference to a piece of |u|, so this function
   // must take care not to use |aLookup| after moving |u|.
   template<typename... Args>
   MOZ_MUST_USE bool relookupOrAdd(AddPtr& aPtr,
                                   const Lookup& aLookup,
@@ -2237,17 +2214,17 @@ public:
   {
     MOZ_ASSERT(mTable);
     ReentrancyGuard g(*this);
     MOZ_ASSERT(aPtr.found());
     MOZ_ASSERT(aPtr.mGeneration == generation());
     typename HashTableEntry<T>::NonConstT t(std::move(*aPtr));
     HashPolicy::setKey(t, const_cast<Key&>(aKey));
     remove(*aPtr.mEntry);
-    putNewInfallibleInternal(aLookup, std::move(t));
+    putNewInternal(aLookup, std::move(t));
   }
 
   void rekeyAndMaybeRehash(Ptr aPtr, const Lookup& aLookup, const Key& aKey)
   {
     rekeyWithoutRehash(aPtr, aLookup, aKey);
     checkOverRemoved();
   }
 };