From d55d894e7e4cf3c3acb77c6e389f16bb8845e874 Mon Sep 17 00:00:00 2001 From: Terrence Cole Date: Thu, 21 Feb 2013 17:36:38 -0800 Subject: [PATCH] Bug 843907 - Move MapObject and SetObject's key to manual post-barriers; r=jorendorff --HG-- extra : rebase_source : ae9d62120c1a0f212c266bbf3879010cd48e3124 --- js/src/builtin/MapObject.cpp | 131 ++++++++++++++---- js/src/builtin/MapObject.h | 4 +- js/src/gc/RootMarking.cpp | 1 + .../tests/collections/Map-Set-moving-gc.js | 16 +++ js/src/jsobjinlines.h | 1 - 5 files changed, 123 insertions(+), 30 deletions(-) create mode 100644 js/src/jit-test/tests/collections/Map-Set-moving-gc.js diff --git a/js/src/builtin/MapObject.cpp b/js/src/builtin/MapObject.cpp index 9f8b8a37dec4..1f6258f49bcc 100644 --- a/js/src/builtin/MapObject.cpp +++ b/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 ¤t, 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 ¤t, const Key &newKey) { + const Entry *e = get(current); + if (!e) + return; + return impl.rekeyOneEntry(current, newKey, Entry(newKey, e->value)); + } }; template @@ -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 ¤t, 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(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 +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 +static void +WriteBarrierPost(JSRuntime *rt, TableType *table, const HashableValue &key) +{ +#ifdef JSGC_GENERATIONAL + rt->gcStoreBuffer.putGeneric(OrderedHashTableRef(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(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; } diff --git a/js/src/builtin/MapObject.h b/js/src/builtin/MapObject.h index 55e5e3c626af..3661c4cc498b 100644 --- a/js/src/builtin/MapObject.h +++ b/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(); } diff --git a/js/src/gc/RootMarking.cpp b/js/src/gc/RootMarking.cpp index cc9ba63ceb8b..4d3b7a9ea03d 100644 --- a/js/src/gc/RootMarking.cpp +++ b/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" diff --git a/js/src/jit-test/tests/collections/Map-Set-moving-gc.js b/js/src/jit-test/tests/collections/Map-Set-moving-gc.js new file mode 100644 index 000000000000..e603756dc32d --- /dev/null +++ b/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); +} diff --git a/js/src/jsobjinlines.h b/js/src/jsobjinlines.h index f319f5a5452c..ea3a03d3ae4e 100644 --- a/js/src/jsobjinlines.h +++ b/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"