Bug 843907 - Move MapObject and SetObject's key to manual post-barriers; r=jorendorff

--HG--
extra : rebase_source : ae9d62120c1a0f212c266bbf3879010cd48e3124

js/src/builtin/MapObject.cpp

@@ -229,18 +229,8 @@ class OrderedHashTable
      * The effect on live Ranges is the same as removing all entries; in
      * particular, those Ranges are still live and will see any entries added
      * after a successful clear().
-     *
-     * The DoNotCallDestructors specialization is for use during a GC when the
-     * OrderedHashTable contains pointers to GC things in other arenas. Since
-     * it is invalid to touch objects in other arenas during sweeping, we need
-     * to not trigger destructors on the pointers contained in the table in
-     * this case.
      */
-    enum CallDestructors {
-        DoNotCallDtors = false,
-        DoCallDtors = true
-    };
-    bool clear(CallDestructors callDestructors = DoCallDtors) {
+    bool clear() {
         if (dataLength != 0) {
             Data **oldHashTable = hashTable;
             Data *oldData = data;
@@ -254,9 +244,7 @@ class OrderedHashTable
             }
 
             alloc.free_(oldHashTable);
-            if (callDestructors)
-                destroyData(oldData, oldDataLength);
-            alloc.free_(oldData);
+            freeData(oldData, oldDataLength);
             for (Range *r = ranges; r; r = r->next)
                 r->onClear();
         }
@@ -500,6 +488,49 @@ class OrderedHashTable
 
     Range all() { return Range(*this); }
 
+    /*
+     * Change the value of the given key.
+     *
+     * This calls Ops::hash on both the current key and the new key.
+     * Ops::hash on the current key must return the same hash code as
+     * when the entry was added to the table.
+     */
+    void rekeyOneEntry(const Key &current, const Key &newKey, const T &element) {
+        if (current == newKey)
+            return;
+
+        Data *entry = lookup(current, prepareHash(current));
+        if (!entry)
+            return;
+
+        HashNumber oldHash = prepareHash(current) >> hashShift;
+        HashNumber newHash = prepareHash(newKey) >> hashShift;
+
+        entry->element = element;
+
+        // Remove this entry from its old hash chain. (If this crashes
+        // reading NULL, it would mean we did not find this entry on
+        // the hash chain where we expected it. That probably means the
+        // key's hash code changed since it was inserted, breaking the
+        // hash code invariant.)
+        Data **ep = &hashTable[oldHash];
+        while (*ep != entry)
+            ep = &(*ep)->chain;
+        *ep = entry->chain;
+
+        // Add it to the new hash chain. We could just insert it at the
+        // beginning of the chain. Instead, we do a bit of work to
+        // preserve the invariant that hash chains always go in reverse
+        // insertion order (descending memory order). No code currently
+        // depends on this invariant, so it's fine to kill it if
+        // needed.
+        ep = &hashTable[newHash];
+        while (*ep && *ep > entry)
+            ep = &(*ep)->chain;
+        entry->chain = *ep;
+        *ep = entry;
+    }
+
   private:
     /* Logarithm base 2 of the number of buckets in the hash table initially. */
     static uint32_t initialBucketsLog2() { return 1; }
@@ -702,8 +733,14 @@ class OrderedHashMap
     Entry *get(const Key &key)                      { return impl.get(key); }
     bool put(const Key &key, const Value &value)    { return impl.put(Entry(key, value)); }
     bool remove(const Key &key, bool *foundp)       { return impl.remove(key, foundp); }
-    bool clear()                                    { return impl.clear(Impl::DoCallDtors); }
-    bool clearWithoutCallingDestructors()           { return impl.clear(Impl::DoNotCallDtors); }
+    bool clear()                                    { return impl.clear(); }
+
+    void rekeyOneEntry(const Key &current, const Key &newKey) {
+        const Entry *e = get(current);
+        if (!e)
+            return;
+        return impl.rekeyOneEntry(current, newKey, Entry(newKey, e->value));
+    }
 };
 
 template <class T, class OrderedHashPolicy, class AllocPolicy>
@@ -730,8 +767,11 @@ class OrderedHashSet
     Range all()                                     { return impl.all(); }
     bool put(const T &value)                        { return impl.put(value); }
     bool remove(const T &value, bool *foundp)       { return impl.remove(value, foundp); }
-    bool clear()                                    { return impl.clear(Impl::DoCallDtors); }
-    bool clearWithoutCallingDestructors()           { return impl.clear(Impl::DoNotCallDtors); }
+    bool clear()                                    { return impl.clear(); }
+
+    void rekeyOneEntry(const T &current, const T &newKey) {
+        return impl.rekeyOneEntry(current, newKey, newKey);
+    }
 };
 
 }  // namespace js
@@ -743,7 +783,7 @@ bool
 HashableValue::setValue(JSContext *cx, const Value &v)
 {
     if (v.isString()) {
-        // Atomize so that hash() and equals() are fast and infallible.
+        // Atomize so that hash() and operator==() are fast and infallible.
         JSString *str = AtomizeString<CanGC>(cx, v.toString(), DoNotInternAtom);
         if (!str)
             return false;
@@ -779,7 +819,7 @@ HashableValue::hash() const
 }
 
 bool
-HashableValue::equals(const HashableValue &other) const
+HashableValue::operator==(const HashableValue &other) const
 {
     // Two HashableValues are equal if they have equal bits.
     bool b = (value.asRawBits() == other.value.asRawBits());
@@ -1070,13 +1110,48 @@ MapObject::mark(JSTracer *trc, RawObject obj)
     }
 }
 
+#ifdef JSGC_GENERATIONAL
+template <typename TableType>
+class OrderedHashTableRef : public gc::BufferableRef
+{
+    TableType *table;
+    HashableValue key;
+
+  public:
+    OrderedHashTableRef(TableType *t, const HashableValue &k) : table(t), key(k) {}
+
+    bool match(void *location) {
+        if (!table->has(key))
+            return false;
+        for (typename TableType::Range r = table->all(); !r.empty(); r.popFront()) {
+            if ((void*)&r.front() == location)
+                return true;
+        }
+        return false;
+    }
+
+    void mark(JSTracer *trc) {
+        HashableValue prior = key;
+        key = key.mark(trc);
+        table->rekeyOneEntry(prior, key);
+    }
+};
+#endif
+
+template <typename TableType>
+static void
+WriteBarrierPost(JSRuntime *rt, TableType *table, const HashableValue &key)
+{
+#ifdef JSGC_GENERATIONAL
+    rt->gcStoreBuffer.putGeneric(OrderedHashTableRef<TableType>(table, key));
+#endif
+}
+
 void
 MapObject::finalize(FreeOp *fop, RawObject obj)
 {
-    if (ValueMap *map = obj->asMap().getData()) {
-        map->clearWithoutCallingDestructors();
+    if (ValueMap *map = obj->asMap().getData())
         fop->delete_(map);
-    }
 }
 
 JSBool
@@ -1121,6 +1196,7 @@ MapObject::construct(JSContext *cx, unsigned argc, Value *vp)
                 js_ReportOutOfMemory(cx);
                 return false;
             }
+            WriteBarrierPost(cx->runtime, map, hkey);
         }
         if (!iter.close())
             return false;
@@ -1215,11 +1291,12 @@ MapObject::set_impl(JSContext *cx, CallArgs args)
 
     ValueMap &map = extract(args);
     ARG0_KEY(cx, args, key);
-    RelocatableValue rval(args.length() > 1 ? args[1] : UndefinedValue());
+    RelocatableValue rval(args.get(1));
     if (!map.put(key, rval)) {
         js_ReportOutOfMemory(cx);
         return false;
     }
+    WriteBarrierPost(cx->runtime, &map, key);
     args.rval().setUndefined();
     return true;
 }
@@ -1528,10 +1605,8 @@ void
 SetObject::finalize(FreeOp *fop, RawObject obj)
 {
     SetObject *setobj = static_cast<SetObject *>(obj);
-    if (ValueSet *set = setobj->getData()) {
-        set->clearWithoutCallingDestructors();
+    if (ValueSet *set = setobj->getData())
         fop->delete_(set);
-    }
 }
 
 JSBool
@@ -1562,6 +1637,7 @@ SetObject::construct(JSContext *cx, unsigned argc, Value *vp)
                 js_ReportOutOfMemory(cx);
                 return false;
             }
+            WriteBarrierPost(cx->runtime, set, key);
         }
         if (!iter.close())
             return false;
@@ -1632,6 +1708,7 @@ SetObject::add_impl(JSContext *cx, CallArgs args)
         js_ReportOutOfMemory(cx);
         return false;
     }
+    WriteBarrierPost(cx->runtime, &set, key);
     args.rval().setUndefined();
     return true;
 }

js/src/builtin/MapObject.h

@@ -32,7 +32,7 @@ class HashableValue {
     struct Hasher {
         typedef HashableValue Lookup;
         static HashNumber hash(const Lookup &v) { return v.hash(); }
-        static bool match(const HashableValue &k, const Lookup &l) { return k.equals(l); }
+        static bool match(const HashableValue &k, const Lookup &l) { return k == l; }
         static bool isEmpty(const HashableValue &v) { return v.value.isMagic(JS_HASH_KEY_EMPTY); }
         static void makeEmpty(HashableValue *vp) { vp->value = MagicValue(JS_HASH_KEY_EMPTY); }
     };
@@ -41,7 +41,7 @@ class HashableValue {
 
     bool setValue(JSContext *cx, const Value &v);
     HashNumber hash() const;
-    bool equals(const HashableValue &other) const;
+    bool operator==(const HashableValue &other) const;
     HashableValue mark(JSTracer *trc) const;
     Value get() const { return value.get(); }
 

js/src/gc/RootMarking.cpp

@@ -16,6 +16,7 @@
 #include "jsprf.h"
 #include "jswatchpoint.h"
 
+#include "builtin/MapObject.h"
 #include "frontend/Parser.h"
 #include "gc/GCInternals.h"
 #include "gc/Marking.h"

js/src/jit-test/tests/collections/Map-Set-moving-gc.js

@@ -0,0 +1,16 @@
+var m = new Map;
+var s = new Set;
+
+var A = [];
+for (var i = 0; i < 1024; ++i) {
+    var key = {i:i};
+    m.set(key, i);
+    s.add(key);
+    A.push(key);
+}
+gc();
+for (var i in A) {
+    var key = A[i];
+    assertEq(m.has(key), true);
+    assertEq(s.has(key), true);
+}

js/src/jsobjinlines.h

@@ -24,7 +24,6 @@
 #include "jstypedarray.h"
 #include "jswrapper.h"
 
-#include "builtin/MapObject.h"
 #include "builtin/Iterator-inl.h"
 #include "gc/Barrier.h"
 #include "gc/Marking.h"