Bug 722946 - Add method to HashTable for inline rekeying/removal; r=luke

It is possible in several places to have unconstrained client objects as the key
of a HashMap/HashSet.  For generational and compacting GC, we will need to move
these objects in memory.  This means we will need to remove and re-insert these
keys in a new location, during GC.  Because we cannot safely allocate memory
for this, we need a specialized algorithm inside the HashTable itself.

js/public/HashTable.h

@@ -96,7 +96,7 @@ class HashTableEntry {
     void setCollision(HashNumber collisionBit) {
         JS_ASSERT(isLive()); keyHash |= collisionBit;
     }
-    void unsetCollision()         { JS_ASSERT(isLive()); keyHash &= ~sCollisionBit; }
+    void unsetCollision()         { keyHash &= ~sCollisionBit; }
     bool hasCollision() const     { JS_ASSERT(isLive()); return keyHash & sCollisionBit; }
     bool matchHash(HashNumber hn) { return (keyHash & ~sCollisionBit) == hn; }
     HashNumber getKeyHash() const { JS_ASSERT(!hasCollision()); return keyHash; }
@@ -158,14 +158,9 @@ class HashTable : private AllocPolicy
     {
         friend class HashTable;
         HashNumber keyHash;
-#ifdef DEBUG
-        uint64_t mutationCount;
+        DebugOnly<uint64_t> mutationCount;
 
-        AddPtr(Entry &entry, HashNumber hn, uint64_t mutationCount)
-            : Ptr(entry), keyHash(hn), mutationCount(mutationCount) {}
-#else
         AddPtr(Entry &entry, HashNumber hn) : Ptr(entry), keyHash(hn) {}
-#endif
       public:
         /* Leaves AddPtr uninitialized. */
         AddPtr() {}
@@ -222,6 +217,7 @@ class HashTable : private AllocPolicy
         friend class HashTable;
 
         HashTable &table;
+        bool added;
         bool removed;
 
         /* Not copyable. */
@@ -230,7 +226,7 @@ class HashTable : private AllocPolicy
 
       public:
         template<class Map> explicit
-        Enum(Map &map) : Range(map.all()), table(map.impl), removed(false) {}
+        Enum(Map &map) : Range(map.all()), table(map.impl), added(false), removed(false) {}
 
         /*
          * Removes the |front()| element from the table, leaving |front()|
@@ -246,14 +242,35 @@ class HashTable : private AllocPolicy
             removed = true;
         }
 
+        /*
+         * Removes the |front()| element and re-inserts it into the table with
+         * a new key at the new Lookup position.  |front()| is invalid after
+         * this operation until the next call to |popFront()|.
+         */
+        void rekeyFront(Key &k) {
+            JS_ASSERT(&k != &HashPolicy::getKey(this->cur->t));
+            JS_ASSERT(!table.match(*this->cur, k));
+            Entry e = *this->cur;
+            HashPolicy::setKey(e.t, k);
+            table.remove(*this->cur);
+            table.add(k, e);
+            added = true;
+        }
+
         /* Potentially rehashes the table. */
         ~Enum() {
+            if (added)
+                table.checkOverloaded();
             if (removed)
                 table.checkUnderloaded();
         }
 
         /* Can be used to end the enumeration before the destructor. */
         void endEnumeration() {
+            if (added) {
+                table.checkOverloaded();
+                added = false;
+            }
             if (removed) {
                 table.checkUnderloaded();
                 removed = false;
@@ -290,11 +307,9 @@ class HashTable : private AllocPolicy
 #   define METER(x)
 #endif
 
-#ifdef DEBUG
     friend class js::ReentrancyGuard;
-    mutable bool entered;
-    uint64_t     mutationCount;
-#endif
+    mutable DebugOnly<bool> entered;
+    DebugOnly<uint64_t>     mutationCount;
 
     /* The default initial capacity is 16, but you can ask for as small as 4. */
     static const unsigned sMinSizeLog2  = 2;
@@ -364,11 +379,9 @@ class HashTable : private AllocPolicy
         entryCount(0),
         gen(0),
         removedCount(0),
-        table(NULL)
-#ifdef DEBUG
-        , entered(false),
+        table(NULL),
+        entered(false),
         mutationCount(0)
-#endif
     {}
 
     bool init(uint32_t length)
@@ -424,8 +437,22 @@ class HashTable : private AllocPolicy
         return hash0 >> shift;
     }
 
-    static HashNumber hash2(HashNumber hash0, uint32_t log2, uint32_t shift) {
-        return ((hash0 << log2) >> shift) | 1;
+    struct DoubleHash {
+        HashNumber h2;
+        HashNumber sizeMask;
+    };
+
+    DoubleHash hash2(HashNumber curKeyHash, uint32_t hashShift) const {
+        unsigned sizeLog2 = sHashBits - hashShift;
+        DoubleHash dh = {
+            ((curKeyHash << sizeLog2) >> hashShift) | 1,
+            (HashNumber(1) << sizeLog2) - 1
+        };
+        return dh;
+    }
+
+    static HashNumber applyDoubleHash(HashNumber h1, const DoubleHash &dh) {
+        return (h1 - dh.h2) & dh.sizeMask;
     }
 
     bool overloaded() {
@@ -467,9 +494,7 @@ class HashTable : private AllocPolicy
         }
 
         /* Collision: double hash. */
-        unsigned sizeLog2 = sHashBits - hashShift;
-        HashNumber h2 = hash2(keyHash, sizeLog2, hashShift);
-        HashNumber sizeMask = (HashNumber(1) << sizeLog2) - 1;
+        DoubleHash dh = hash2(keyHash, hashShift);
 
         /* Save the first removed entry pointer so we can recycle later. */
         Entry *firstRemoved = NULL;
@@ -483,8 +508,7 @@ class HashTable : private AllocPolicy
             }
 
             METER(stats.steps++);
-            h1 -= h2;
-            h1 &= sizeMask;
+            h1 = applyDoubleHash(h1, dh);
 
             entry = &table[h1];
             if (entry->isFree()) {
@@ -525,17 +549,14 @@ class HashTable : private AllocPolicy
         }
 
         /* Collision: double hash. */
-        unsigned sizeLog2 = sHashBits - hashShift;
-        HashNumber h2 = hash2(keyHash, sizeLog2, hashShift);
-        HashNumber sizeMask = (HashNumber(1) << sizeLog2) - 1;
+        DoubleHash dh = hash2(keyHash, hashShift);
 
         while(true) {
             JS_ASSERT(!entry->isRemoved());
             entry->setCollision();
 
             METER(stats.steps++);
-            h1 -= h2;
-            h1 &= sizeMask;
+            h1 = applyDoubleHash(h1, dh);
 
             entry = &table[h1];
             if (entry->isFree()) {
@@ -579,6 +600,44 @@ class HashTable : private AllocPolicy
         return true;
     }
 
+    void add(const Lookup &l, const Entry &e)
+    {
+        HashNumber keyHash = prepareHash(l);
+        Entry &entry = lookup(l, keyHash, sCollisionBit);
+
+        if (entry.isRemoved()) {
+            METER(stats.addOverRemoved++);
+            removedCount--;
+            keyHash |= sCollisionBit;
+        }
+
+        entry.t = e.t;
+        entry.setLive(keyHash);
+        entryCount++;
+        mutationCount++;
+    }
+
+    bool checkOverloaded()
+    {
+        if (overloaded()) {
+            /* Compress if a quarter or more of all entries are removed. */
+            int deltaLog2;
+            if (removedCount >= (capacity() >> 2)) {
+                METER(stats.compresses++);
+                deltaLog2 = 0;
+            } else {
+                METER(stats.grows++);
+                deltaLog2 = 1;
+            }
+
+            (void) changeTableSize(deltaLog2);
+
+            return true;
+        }
+
+        return false;
+    }
+
     void remove(Entry &e)
     {
         METER(stats.removes++);
@@ -590,9 +649,7 @@ class HashTable : private AllocPolicy
             e.setFree();
         }
         entryCount--;
-#ifdef DEBUG
         mutationCount++;
-#endif
     }
 
     void checkUnderloaded()
@@ -615,9 +672,7 @@ class HashTable : private AllocPolicy
         }
         removedCount = 0;
         entryCount = 0;
-#ifdef DEBUG
         mutationCount++;
-#endif
     }
 
     void finish()
@@ -626,15 +681,13 @@ class HashTable : private AllocPolicy
 
         if (!table)
             return;
-        
+
         destroyTable(*this, table, capacity());
         table = NULL;
         gen++;
         entryCount = 0;
         removedCount = 0;
-#ifdef DEBUG
         mutationCount++;
-#endif
     }
 
     Range all() const {
@@ -675,11 +728,9 @@ class HashTable : private AllocPolicy
         ReentrancyGuard g(*this);
         HashNumber keyHash = prepareHash(l);
         Entry &entry = lookup(l, keyHash, sCollisionBit);
-#ifdef DEBUG
-        return AddPtr(entry, keyHash, mutationCount);
-#else
-        return AddPtr(entry, keyHash);
-#endif
+        AddPtr p(entry, keyHash);
+        p.mutationCount = mutationCount;
+        return p;
     }
 
     bool add(AddPtr &p)
@@ -699,31 +750,14 @@ class HashTable : private AllocPolicy
             removedCount--;
             p.keyHash |= sCollisionBit;
         } else {
-            /* If alpha is >= .75, grow or compress the table. */
-            if (overloaded()) {
-                /* Compress if a quarter or more of all entries are removed. */
-                int deltaLog2;
-                if (removedCount >= (capacity() >> 2)) {
-                    METER(stats.compresses++);
-                    deltaLog2 = 0;
-                } else {
-                    METER(stats.grows++);
-                    deltaLog2 = 1;
-                }
-
-                if (!changeTableSize(deltaLog2))
-                    return false;
-
+            if (checkOverloaded())
                 /* Preserve the validity of |p.entry|. */
                 p.entry = &findFreeEntry(p.keyHash);
-            }
         }
 
         p.entry->setLive(p.keyHash);
         entryCount++;
-#ifdef DEBUG
         mutationCount++;
-#endif
         return true;
     }
 
@@ -750,9 +784,7 @@ class HashTable : private AllocPolicy
 
     bool relookupOrAdd(AddPtr& p, const Lookup &l, const T& t)
     {
-#ifdef DEBUG
         p.mutationCount = mutationCount;
-#endif
         {
             ReentrancyGuard g(*this);
             p.entry = &lookup(l, p.keyHash, sCollisionBit);
@@ -767,6 +799,7 @@ class HashTable : private AllocPolicy
         remove(*p.entry);
         checkUnderloaded();
     }
+
 #undef METER
 };
 
@@ -880,7 +913,7 @@ class HashMapEntry
     template<typename KeyInput, typename ValueInput>
     HashMapEntry(const KeyInput &k, const ValueInput &v) : key(k), value(v) {}
 
-    HashMapEntry(MoveRef<HashMapEntry> rhs) 
+    HashMapEntry(MoveRef<HashMapEntry> rhs)
       : key(Move(rhs->key)), value(Move(rhs->value)) { }
     void operator=(MoveRef<HashMapEntry> rhs) {
         const_cast<Key &>(key) = Move(rhs->key);
@@ -939,6 +972,7 @@ class HashMap
     {
         typedef Key KeyType;
         static const Key &getKey(Entry &e) { return e.key; }
+        static void setKey(Entry &e, Key &k) { const_cast<Key &>(e.key) = k; }
     };
     typedef detail::HashTable<Entry, MapHashPolicy, AllocPolicy> Impl;
 
@@ -1068,7 +1102,7 @@ class HashMap
         return impl.sizeOfExcludingThis(mallocSizeOf);
     }
     size_t sizeOfIncludingThis(JSMallocSizeOfFun mallocSizeOf) const {
-        /* 
+        /*
          * Don't just call |impl.sizeOfExcludingThis()| because there's no
          * guarantee that |impl| is the first field in HashMap.
          */
@@ -1176,6 +1210,7 @@ class HashSet
     struct SetOps : HashPolicy {
         typedef T KeyType;
         static const KeyType &getKey(const T &t) { return t; }
+        static void setKey(T &t, KeyType &k) { t = k; }
     };
     typedef detail::HashTable<const T, SetOps, AllocPolicy> Impl;
 
@@ -1278,7 +1313,7 @@ class HashSet
         return impl.sizeOfExcludingThis(mallocSizeOf);
     }
     size_t sizeOfIncludingThis(JSMallocSizeOfFun mallocSizeOf) const {
-        /* 
+        /*
          * Don't just call |impl.sizeOfExcludingThis()| because there's no
          * guarantee that |impl| is the first field in HashSet.
          */

js/src/jsapi-tests/Makefile.in

@@ -69,6 +69,7 @@ CPPSRCS = \
   testGCOutOfMemory.cpp \
   testOOM.cpp \
   testGetPropertyDefault.cpp \
+  testHashTable.cpp \
   testIndexToString.cpp \
   testIntString.cpp \
   testIntTypesABI.cpp \

js/src/jsapi-tests/testHashTable.cpp

@@ -0,0 +1,289 @@
+#include "tests.h"
+
+#include <sys/time.h>
+
+#include "js/HashTable.h"
+
+//#define FUZZ
+
+typedef js::HashMap<uint32_t, uint32_t, js::DefaultHasher<uint32_t>, js::SystemAllocPolicy> IntMap;
+typedef js::HashSet<uint32_t, js::DefaultHasher<uint32_t>, js::SystemAllocPolicy> IntSet;
+
+/*
+ * The rekeying test as conducted by adding only keys masked with 0x0000FFFF
+ * that are unique. We rekey by shifting left 16 bits.
+ */
+#ifdef FUZZ
+const size_t TestSize = 0x0000FFFF / 2;
+const size_t TestIterations = SIZE_MAX;
+#else
+const size_t TestSize = 10000;
+const size_t TestIterations = 10;
+#endif
+
+JS_STATIC_ASSERT(TestSize <= 0x0000FFFF / 2);
+
+struct LowToHigh
+{
+    static uint32_t rekey(uint32_t initial) {
+        if (initial > uint32_t(0x0000FFFF))
+            return initial;
+        return initial << 16;
+    }
+
+    static bool shouldBeRemoved(uint32_t initial) {
+        return false;
+    }
+};
+
+struct LowToHighWithRemoval
+{
+    static uint32_t rekey(uint32_t initial) {
+        if (initial > uint32_t(0x0000FFFF))
+            return initial;
+        return initial << 16;
+    }
+
+    static bool shouldBeRemoved(uint32_t initial) {
+        if (initial >= 0x00010000)
+            return (initial >> 16) % 2 == 0;
+        return initial % 2 == 0;
+    }
+};
+
+static bool
+MapsAreEqual(IntMap &am, IntMap &bm)
+{
+    bool equal = true;
+    if (am.count() != bm.count()) {
+        equal = false;
+        fprintf(stderr, "A.count() == %u and B.count() == %u\n", am.count(), bm.count());
+    }
+    for (IntMap::Range r = am.all(); !r.empty(); r.popFront()) {
+        if (!bm.has(r.front().key)) {
+            equal = false;
+            fprintf(stderr, "B does not have %x which is in A\n", r.front().key);
+        }
+    }
+    for (IntMap::Range r = bm.all(); !r.empty(); r.popFront()) {
+        if (!am.has(r.front().key)) {
+            equal = false;
+            fprintf(stderr, "A does not have %x which is in B\n", r.front().key);
+        }
+    }
+    return equal;
+}
+
+static bool
+SetsAreEqual(IntSet &am, IntSet &bm)
+{
+    bool equal = true;
+    if (am.count() != bm.count()) {
+        equal = false;
+        fprintf(stderr, "A.count() == %u and B.count() == %u\n", am.count(), bm.count());
+    }
+    for (IntSet::Range r = am.all(); !r.empty(); r.popFront()) {
+        if (!bm.has(r.front())) {
+            equal = false;
+            fprintf(stderr, "B does not have %x which is in A\n", r.front());
+        }
+    }
+    for (IntSet::Range r = bm.all(); !r.empty(); r.popFront()) {
+        if (!am.has(r.front())) {
+            equal = false;
+            fprintf(stderr, "A does not have %x which is in B\n", r.front());
+        }
+    }
+    return equal;
+}
+
+static bool
+AddLowKeys(IntMap *am, IntMap *bm, int seed)
+{
+    size_t i = 0;
+    srand(seed);
+    while (i < TestSize) {
+        uint32_t n = rand() & 0x0000FFFF;
+        if (!am->has(n)) {
+            if (bm->has(n))
+                return false;
+            am->putNew(n, n);
+            bm->putNew(n, n);
+            i++;
+        }
+    }
+    return true;
+}
+
+static bool
+AddLowKeys(IntSet *as, IntSet *bs, int seed)
+{
+    size_t i = 0;
+    srand(seed);
+    while (i < TestSize) {
+        uint32_t n = rand() & 0x0000FFFF;
+        if (!as->has(n)) {
+            if (bs->has(n))
+                return false;
+            as->putNew(n);
+            bs->putNew(n);
+            i++;
+        }
+    }
+    return true;
+}
+
+template <class NewKeyFunction>
+static bool
+SlowRekey(IntMap *m) {
+    IntMap tmp;
+    tmp.init();
+
+    for (IntMap::Range r = m->all(); !r.empty(); r.popFront()) {
+        if (NewKeyFunction::shouldBeRemoved(r.front().key))
+            continue;
+        uint32_t hi = NewKeyFunction::rekey(r.front().key);
+        if (tmp.has(hi))
+            return false;
+        tmp.putNew(hi, r.front().value);
+    }
+
+    m->clear();
+    for (IntMap::Range r = tmp.all(); !r.empty(); r.popFront()) {
+        m->putNew(r.front().key, r.front().value);
+    }
+
+    return true;
+}
+
+template <class NewKeyFunction>
+static bool
+SlowRekey(IntSet *s) {
+    IntSet tmp;
+    tmp.init();
+
+    for (IntSet::Range r = s->all(); !r.empty(); r.popFront()) {
+        if (NewKeyFunction::shouldBeRemoved(r.front()))
+            continue;
+        uint32_t hi = NewKeyFunction::rekey(r.front());
+        if (tmp.has(hi))
+            return false;
+        tmp.putNew(hi);
+    }
+
+    s->clear();
+    for (IntSet::Range r = tmp.all(); !r.empty(); r.popFront()) {
+        s->putNew(r.front());
+    }
+
+    return true;
+}
+
+BEGIN_TEST(testHashRekeyManual)
+{
+    IntMap am, bm;
+    am.init();
+    bm.init();
+    for (size_t i = 0; i < TestIterations; ++i) {
+#ifdef FUZZ
+        fprintf(stderr, "map1: %lu\n", i);
+#endif
+        CHECK(AddLowKeys(&am, &bm, i));
+        CHECK(MapsAreEqual(am, bm));
+
+        for (IntMap::Enum e(am); !e.empty(); e.popFront()) {
+            uint32_t tmp = LowToHigh::rekey(e.front().key);
+            if (tmp != e.front().key)
+                e.rekeyFront(tmp);
+        }
+        CHECK(SlowRekey<LowToHigh>(&bm));
+
+        CHECK(MapsAreEqual(am, bm));
+        am.clear();
+        bm.clear();
+    }
+
+    IntSet as, bs;
+    as.init();
+    bs.init();
+    for (size_t i = 0; i < TestIterations; ++i) {
+#ifdef FUZZ
+        fprintf(stderr, "set1: %lu\n", i);
+#endif
+        CHECK(AddLowKeys(&as, &bs, i));
+        CHECK(SetsAreEqual(as, bs));
+
+        for (IntSet::Enum e(as); !e.empty(); e.popFront()) {
+            uint32_t tmp = LowToHigh::rekey(e.front());
+            if (tmp != e.front())
+                e.rekeyFront(tmp);
+        }
+        CHECK(SlowRekey<LowToHigh>(&bs));
+
+        CHECK(SetsAreEqual(as, bs));
+        as.clear();
+        bs.clear();
+    }
+
+    return true;
+}
+END_TEST(testHashRekeyManual)
+
+BEGIN_TEST(testHashRekeyManualRemoval)
+{
+    IntMap am, bm;
+    am.init();
+    bm.init();
+    for (size_t i = 0; i < TestIterations; ++i) {
+#ifdef FUZZ
+        fprintf(stderr, "map2: %lu\n", i);
+#endif
+        CHECK(AddLowKeys(&am, &bm, i));
+        CHECK(MapsAreEqual(am, bm));
+
+        for (IntMap::Enum e(am); !e.empty(); e.popFront()) {
+            if (LowToHighWithRemoval::shouldBeRemoved(e.front().key)) {
+                e.removeFront();
+            } else {
+                uint32_t tmp = LowToHighWithRemoval::rekey(e.front().key);
+                if (tmp != e.front().key)
+                    e.rekeyFront(tmp);
+            }
+        }
+        CHECK(SlowRekey<LowToHighWithRemoval>(&bm));
+
+        CHECK(MapsAreEqual(am, bm));
+        am.clear();
+        bm.clear();
+    }
+
+    IntSet as, bs;
+    as.init();
+    bs.init();
+    for (size_t i = 0; i < TestIterations; ++i) {
+#ifdef FUZZ
+        fprintf(stderr, "set1: %lu\n", i);
+#endif
+        CHECK(AddLowKeys(&as, &bs, i));
+        CHECK(SetsAreEqual(as, bs));
+
+        for (IntSet::Enum e(as); !e.empty(); e.popFront()) {
+            if (LowToHighWithRemoval::shouldBeRemoved(e.front())) {
+                e.removeFront();
+            } else {
+                uint32_t tmp = LowToHighWithRemoval::rekey(e.front());
+                if (tmp != e.front())
+                    e.rekeyFront(tmp);
+            }
+        }
+        CHECK(SlowRekey<LowToHighWithRemoval>(&bs));
+
+        CHECK(SetsAreEqual(as, bs));
+        as.clear();
+        bs.clear();
+    }
+
+    return true;
+}
+END_TEST(testHashRekeyManualRemoval)
+

js/src/jsopcode.cpp

@@ -2530,15 +2530,6 @@ InitSprintStack(JSContext *cx, SprintStack *ss, JSPrinter *jp, unsigned depth)
     return JS_TRUE;
 }
 
-template <typename T>
-void
-Swap(T &a, T &b)
-{
-    T tmp = a;
-    a = b;
-    b = tmp;
-}
-
 /*
  * If nb is non-negative, decompile nb bytecodes starting at pc.  Otherwise
  * the decompiler starts at pc and continues until it reaches an opcode for

js/src/jsutil.h

@@ -273,6 +273,15 @@ PodEqual(T *one, T *two, size_t len)
     return !memcmp(one, two, len * sizeof(T));
 }
 
+template <class T>
+JS_ALWAYS_INLINE static void
+Swap(T &t, T &u)
+{
+    T tmp(Move(t));
+    t = Move(u);
+    u = Move(tmp);
+}
+
 JS_ALWAYS_INLINE static size_t
 UnsignedPtrDiff(const void *bigger, const void *smaller)
 {