From 10db00cf8a81fa8e039f431119a57fc553a647ad Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Tue, 14 Jun 2011 19:21:47 -0700 Subject: [PATCH] Bug 660039: Provide a WeakMap usable from C++. r=jorendorff Remove WeakMap class; implement the JavaScript object using functions static to jsweakmap.cpp. Define a new WeakMap class template, parameterized by Key and Value types, and accepting a MarkPolicy argument saying how to mark them. Add assertions to check that we check and set the right mark bits, and tests that trip them in the presence of mistakes in earlier revisions of this patch. --- js/src/jsapi.cpp | 2 +- js/src/jscntxt.h | 4 +- js/src/jsgc.cpp | 9 +- js/src/jsweakmap.cpp | 214 +++++++++------------ js/src/jsweakmap.h | 181 +++++++++++++++-- js/src/tests/js1_8_5/extensions/weakmap.js | 4 + 6 files changed, 261 insertions(+), 153 deletions(-) diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp index 6e2c2200c657..c87d22fec522 100644 --- a/js/src/jsapi.cpp +++ b/js/src/jsapi.cpp @@ -1605,7 +1605,7 @@ static JSStdName standard_class_atoms[] = { #endif {js_InitJSONClass, EAGER_ATOM_AND_CLASP(JSON)}, {js_InitTypedArrayClasses, EAGER_CLASS_ATOM(ArrayBuffer), &js::ArrayBuffer::slowClass}, - {js_InitWeakMapClass, EAGER_CLASS_ATOM(WeakMap), &WeakMap::jsclass}, + {js_InitWeakMapClass, EAGER_CLASS_ATOM(WeakMap), &js::WeakMapClass}, {NULL, 0, NULL, NULL} }; diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h index 285fc0430dd6..aac102c9f188 100644 --- a/js/src/jscntxt.h +++ b/js/src/jscntxt.h @@ -117,6 +117,8 @@ namespace mjit { class JaegerCompartment; } +class WeakMapBase; + /* * GetSrcNote cache to avoid O(n^2) growth in finding a source note for a * given pc in a script. We use the script->code pointer to tag the cache, @@ -423,7 +425,7 @@ struct JSRuntime { int64 gcJitReleaseTime; JSGCMode gcMode; volatile bool gcIsNeeded; - JSObject *gcWeakMapList; + js::WeakMapBase *gcWeakMapList; /* Pre-allocated space for the GC mark stacks. Pointer type ensures alignment. */ void *gcMarkStackObjs[js::OBJECT_MARK_STACK_SIZE / sizeof(void *)]; diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index ee3bf24db2db..4b16b8e422fe 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -927,8 +927,6 @@ js_FinishGC(JSRuntime *rt) rt->compartments.clear(); rt->atomsCompartment = NULL; - rt->gcWeakMapList = NULL; - for (GCChunkSet::Range r(rt->gcChunkSet.all()); !r.empty(); r.popFront()) ReleaseGCChunk(rt, r.front()); rt->gcChunkSet.clear(); @@ -1513,7 +1511,7 @@ namespace js { * * To implement such delayed marking of the children with minimal overhead for * the normal case of sufficient native stack, the code adds a field per - * arena. The field marlingdelay->link links all arenas with delayed things + * arena. The field markingDelay->link links all arenas with delayed things * into a stack list with the pointer to stack top in * GCMarker::unmarkedArenaStackTop. delayMarkingChildren adds * arenas to the stack as necessary while markDelayedChildren pops the arenas @@ -2285,7 +2283,7 @@ MarkAndSweep(JSContext *cx, JSCompartment *comp, JSGCInvocationKind gckind GCTIM * Mark weak roots. */ while (true) { - if (!js_TraceWatchPoints(&gcmarker) && !WeakMap::markIteratively(&gcmarker)) + if (!js_TraceWatchPoints(&gcmarker) && !WeakMapBase::markAllIteratively(&gcmarker)) break; gcmarker.drainMarkStack(); } @@ -2320,7 +2318,7 @@ MarkAndSweep(JSContext *cx, JSCompartment *comp, JSGCInvocationKind gckind GCTIM GCTIMESTAMP(startSweep); /* Finalize unreachable (key,value) pairs in all weak maps. */ - WeakMap::sweep(cx); + WeakMapBase::sweepAll(&gcmarker); js_SweepAtomState(cx); @@ -2657,6 +2655,7 @@ GCCycle(JSContext *cx, JSCompartment *comp, JSGCInvocationKind gckind GCTIMER_P rt->gcRegenShapes = false; rt->setGCLastBytes(rt->gcBytes); rt->gcCurrentCompartment = NULL; + rt->gcWeakMapList = NULL; for (JSCompartment **c = rt->compartments.begin(); c != rt->compartments.end(); ++c) (*c)->setGCLastBytes((*c)->gcBytes); diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp index 51ab12538a8b..d225b2ca1216 100644 --- a/js/src/jsweakmap.cpp +++ b/js/src/jsweakmap.cpp @@ -54,25 +54,43 @@ using namespace js; +namespace js { + +bool +WeakMapBase::markAllIteratively(JSTracer *tracer) +{ + bool markedAny = false; + JSRuntime *rt = tracer->context->runtime; + for (WeakMapBase *m = rt->gcWeakMapList; m; m = m->next) { + if (m->markIteratively(tracer)) + markedAny = true; + } + return markedAny; +} + +void +WeakMapBase::sweepAll(JSTracer *tracer) +{ + JSRuntime *rt = tracer->context->runtime; + for (WeakMapBase *m = rt->gcWeakMapList; m; m = m->next) + m->sweep(tracer); +} + +} /* namespace js */ + bool JSObject::isWeakMap() const { - return getClass() == &WeakMap::jsclass; + return getClass() == &WeakMapClass; } -namespace js { +typedef WeakMap ObjectValueMap; -WeakMap::WeakMap(JSContext *cx) : - map(cx), - next(NULL) +static ObjectValueMap * +GetObjectMap(JSObject *obj) { -} - -WeakMap * -WeakMap::fromJSObject(JSObject *obj) -{ - JS_ASSERT(obj->getClass() == &WeakMap::jsclass); - return (WeakMap *)obj->getPrivate(); + JS_ASSERT(obj->isWeakMap()); + return (ObjectValueMap *)obj->getPrivate(); } static JSObject * @@ -85,14 +103,14 @@ NonNullObject(JSContext *cx, Value *vp) return &vp->toObject(); } -JSBool -WeakMap::has(JSContext *cx, uintN argc, Value *vp) +static JSBool +WeakMap_has(JSContext *cx, uintN argc, Value *vp) { JSObject *obj = ToObject(cx, &vp[1]); if (!obj) return false; if (!obj->isWeakMap()) { - ReportIncompatibleMethod(cx, vp, &WeakMap::jsclass); + ReportIncompatibleMethod(cx, vp, &WeakMapClass); return false; } if (argc < 1) { @@ -103,9 +121,9 @@ WeakMap::has(JSContext *cx, uintN argc, Value *vp) JSObject *key = NonNullObject(cx, &vp[2]); if (!key) return false; - WeakMap *weakmap = fromJSObject(obj); - if (weakmap) { - ObjectValueMap::Ptr ptr = weakmap->map.lookup(key); + ObjectValueMap *map = GetObjectMap(obj); + if (map) { + ObjectValueMap::Ptr ptr = map->lookup(key); if (ptr) { *vp = BooleanValue(true); return true; @@ -116,14 +134,14 @@ WeakMap::has(JSContext *cx, uintN argc, Value *vp) return true; } -JSBool -WeakMap::get(JSContext *cx, uintN argc, Value *vp) +static JSBool +WeakMap_get(JSContext *cx, uintN argc, Value *vp) { JSObject *obj = ToObject(cx, &vp[1]); if (!obj) return false; if (!obj->isWeakMap()) { - ReportIncompatibleMethod(cx, vp, &WeakMap::jsclass); + ReportIncompatibleMethod(cx, vp, &WeakMapClass); return false; } if (argc < 1) { @@ -134,9 +152,9 @@ WeakMap::get(JSContext *cx, uintN argc, Value *vp) JSObject *key = NonNullObject(cx, &vp[2]); if (!key) return false; - WeakMap *weakmap = fromJSObject(obj); - if (weakmap) { - ObjectValueMap::Ptr ptr = weakmap->map.lookup(key); + ObjectValueMap *map = GetObjectMap(obj); + if (map) { + ObjectValueMap::Ptr ptr = map->lookup(key); if (ptr) { *vp = ptr->value; return true; @@ -147,14 +165,14 @@ WeakMap::get(JSContext *cx, uintN argc, Value *vp) return true; } -JSBool -WeakMap::delete_(JSContext *cx, uintN argc, Value *vp) +static JSBool +WeakMap_delete(JSContext *cx, uintN argc, Value *vp) { JSObject *obj = ToObject(cx, &vp[1]); if (!obj) return false; if (!obj->isWeakMap()) { - ReportIncompatibleMethod(cx, vp, &WeakMap::jsclass); + ReportIncompatibleMethod(cx, vp, &WeakMapClass); return false; } if (argc < 1) { @@ -165,11 +183,11 @@ WeakMap::delete_(JSContext *cx, uintN argc, Value *vp) JSObject *key = NonNullObject(cx, &vp[2]); if (!key) return false; - WeakMap *weakmap = fromJSObject(obj); - if (weakmap) { - ObjectValueMap::Ptr ptr = weakmap->map.lookup(key); + ObjectValueMap *map = GetObjectMap(obj); + if (map) { + ObjectValueMap::Ptr ptr = map->lookup(key); if (ptr) { - weakmap->map.remove(ptr); + map->remove(ptr); *vp = BooleanValue(true); return true; } @@ -179,14 +197,14 @@ WeakMap::delete_(JSContext *cx, uintN argc, Value *vp) return true; } -JSBool -WeakMap::set(JSContext *cx, uintN argc, Value *vp) +static JSBool +WeakMap_set(JSContext *cx, uintN argc, Value *vp) { JSObject *obj = ToObject(cx, &vp[1]); if (!obj) return false; if (!obj->isWeakMap()) { - ReportIncompatibleMethod(cx, vp, &WeakMap::jsclass); + ReportIncompatibleMethod(cx, vp, &WeakMapClass); return false; } if (argc < 1) { @@ -199,18 +217,18 @@ WeakMap::set(JSContext *cx, uintN argc, Value *vp) return false; Value value = (argc > 1) ? vp[3] : UndefinedValue(); - WeakMap *table = (WeakMap *)obj->getPrivate(); - if (!table) { - table = cx->new_(cx); - if (!table->map.init()) { - cx->delete_(table); + ObjectValueMap *map = GetObjectMap(obj); + if (!map) { + map = cx->new_(cx); + if (!map->init()) { + cx->delete_(map); goto out_of_memory; } - obj->setPrivate(table); + obj->setPrivate(map); } *vp = UndefinedValue(); - if (!table->map.put(key, value)) + if (!map->put(key, value)) goto out_of_memory; return true; @@ -219,93 +237,31 @@ WeakMap::set(JSContext *cx, uintN argc, Value *vp) return false; } -void -WeakMap::mark(JSTracer *trc, JSObject *obj) +static void +WeakMap_mark(JSTracer *trc, JSObject *obj) { - WeakMap *table = fromJSObject(obj); - if (table) { - if (IS_GC_MARKING_TRACER(trc)) { - if (table->map.empty()) { - trc->context->delete_(table); - obj->setPrivate(NULL); - return; - } - JSRuntime *rt = trc->context->runtime; - table->next = rt->gcWeakMapList; - rt->gcWeakMapList = obj; + ObjectValueMap *map = GetObjectMap(obj); + if (map) { + if (IS_GC_MARKING_TRACER(trc) && map->empty()) { + trc->context->delete_(map); + obj->setPrivate(NULL); } else { - for (ObjectValueMap::Range r = table->map.all(); !r.empty(); r.popFront()) { - JSObject *key = r.front().key; - Value &value = r.front().value; - js::gc::MarkObject(trc, *key, "key"); - js::gc::MarkValue(trc, value, "value"); - } + map->trace(trc); } } } -/* - * Walk through the previously collected list of tables and mark rows - * iteratively. - */ -bool -WeakMap::markIteratively(JSTracer *trc) +static void +WeakMap_finalize(JSContext *cx, JSObject *obj) { - JSContext *cx = trc->context; - JSRuntime *rt = cx->runtime; - - bool again = false; - JSObject *obj = rt->gcWeakMapList; - while (obj) { - WeakMap *table = fromJSObject(obj); - for (ObjectValueMap::Range r = table->map.all(); !r.empty(); r.popFront()) { - JSObject *key = r.front().key; - Value &value = r.front().value; - if (value.isMarkable() && !IsAboutToBeFinalized(cx, key)) { - /* If the key is alive, mark the value if needed. */ - if (IsAboutToBeFinalized(cx, value.toGCThing())) { - js::gc::MarkValue(trc, value, "value"); - /* We revived a value with children, we have to iterate again. */ - if (value.isGCThing()) - again = true; - } - } - } - obj = table->next; - } - return again; + ObjectValueMap *map = GetObjectMap(obj); + cx->delete_(map); } -void -WeakMap::sweep(JSContext *cx) +static JSBool +WeakMap_construct(JSContext *cx, uintN argc, Value *vp) { - JSRuntime *rt = cx->runtime; - - JSObject *obj = rt->gcWeakMapList; - while (obj) { - WeakMap *table = fromJSObject(obj); - for (ObjectValueMap::Enum e(table->map); !e.empty(); e.popFront()) { - if (IsAboutToBeFinalized(cx, e.front().key)) - e.removeFront(); - } - obj = table->next; - } - - rt->gcWeakMapList = NULL; -} - -void -WeakMap::finalize(JSContext *cx, JSObject *obj) -{ - WeakMap *table = fromJSObject(obj); - if (table) - cx->delete_(table); -} - -JSBool -WeakMap::construct(JSContext *cx, uintN argc, Value *vp) -{ - JSObject *obj = NewBuiltinClassInstance(cx, &WeakMap::jsclass); + JSObject *obj = NewBuiltinClassInstance(cx, &WeakMapClass); if (!obj) return false; @@ -315,7 +271,9 @@ WeakMap::construct(JSContext *cx, uintN argc, Value *vp) return true; } -Class WeakMap::jsclass = { +namespace js { + +Class WeakMapClass = { "WeakMap", JSCLASS_HAS_PRIVATE | JSCLASS_HAS_CACHED_PROTO(JSProto_WeakMap), @@ -326,31 +284,31 @@ Class WeakMap::jsclass = { EnumerateStub, ResolveStub, ConvertStub, - WeakMap::finalize, + WeakMap_finalize, NULL, /* reserved0 */ NULL, /* checkAccess */ NULL, /* call */ NULL, /* construct */ NULL, /* xdrObject */ NULL, /* hasInstance */ - WeakMap::mark + WeakMap_mark }; } -JSFunctionSpec WeakMap::methods[] = { - JS_FN("has", WeakMap::has, 1, 0), - JS_FN("get", WeakMap::get, 2, 0), - JS_FN("delete", WeakMap::delete_, 1, 0), - JS_FN("set", WeakMap::set, 2, 0), +static JSFunctionSpec weak_map_methods[] = { + JS_FN("has", WeakMap_has, 1, 0), + JS_FN("get", WeakMap_get, 2, 0), + JS_FN("delete", WeakMap_delete, 1, 0), + JS_FN("set", WeakMap_set, 2, 0), JS_FS_END }; JSObject * js_InitWeakMapClass(JSContext *cx, JSObject *obj) { - JSObject *proto = js_InitClass(cx, obj, NULL, &WeakMap::jsclass, WeakMap::construct, 0, - NULL, WeakMap::methods, NULL, NULL); + JSObject *proto = js_InitClass(cx, obj, NULL, &WeakMapClass, WeakMap_construct, 0, + NULL, weak_map_methods, NULL, NULL); if (!proto) return NULL; diff --git a/js/src/jsweakmap.h b/js/src/jsweakmap.h index 617104297571..3b4c2dab8517 100644 --- a/js/src/jsweakmap.h +++ b/js/src/jsweakmap.h @@ -48,37 +48,182 @@ namespace js { -typedef js::HashMap, RuntimeAllocPolicy> - ObjectValueMap; +// A subclass template of js::HashMap whose keys and values may be garbage-collected. When +// a key is collected, the table entry disappears, dropping its reference to the value. +// +// More precisely: +// +// A WeakMap entry is collected if and only if either the WeakMap or the entry's key +// is collected. If an entry is not collected, it remains in the WeakMap and it has a +// strong reference to the value. +// +// You must call this table's 'mark' method when the object of which it is a part is +// reached by the garbage collection tracer. Once a table is known to be live, the +// implementation takes care of the iterative marking needed for weak tables and removing +// table entries when collection is complete. +// +// You may provide your own MarkPolicy class to specify how keys and values are marked; a +// policy template provides default definitions for some common key/value type +// combinations. +// +// Details: +// +// The interface is as for a js::HashMap, with the following additions: +// +// - You must call the WeakMap's 'trace' member function when you discover that the map is +// part of a live object. (You'll typically call this from the containing type's 'trace' +// function.) +// +// - There is no AllocPolicy parameter; these are used with our garbage collector, so +// RuntimeAllocPolicy is hard-wired. +// +// - Optional fourth template parameter is a class MarkPolicy, with the following constructor: +// +// MarkPolicy(JSTracer *) +// +// and the following static member functions: +// +// bool keyMarked(Key &k) +// bool valueMarked(Value &v) +// Return true if k/v has been marked as reachable by the collector, false otherwise. +// void markKey(Key &k, const char *description) +// void markValue(Value &v, const char *description) +// Mark k/v as reachable by the collector, using trc. Use description to identify +// k/v in debugging. (markKey is used only for non-marking tracers, other code +// using the GC heap tracing functions to map the heap for some purpose or other.) +// +// If omitted, this parameter defaults to js::DefaultMarkPolicy, a policy +// template with the obvious definitions for some typical SpiderMonkey type combinations. -class WeakMap { - ObjectValueMap map; - JSObject *next; +// A policy template holding default marking algorithms for common type combinations. This +// provides default types for WeakMap's MarkPolicy template parameter. +template class DefaultMarkPolicy; - static WeakMap *fromJSObject(JSObject *obj); +// Common base class for all WeakMap specializations. The collector uses this to call +// their markIteratively and sweep methods. +class WeakMapBase { + public: + WeakMapBase() : next(NULL) { } - static JSBool has(JSContext *cx, uintN argc, Value *vp); - static JSBool get(JSContext *cx, uintN argc, Value *vp); - static JSBool delete_(JSContext *cx, uintN argc, Value *vp); - static JSBool set(JSContext *cx, uintN argc, Value *vp); + void trace(JSTracer *tracer) { + if (IS_GC_MARKING_TRACER(tracer)) { + // We don't do anything with a WeakMap at trace time. Rather, we wait until as + // many keys as possible have been marked, and add ourselves to the list of + // known-live WeakMaps to be scanned in the iterative marking phase, by + // markAllIteratively. + JSRuntime *rt = tracer->context->runtime; + next = rt->gcWeakMapList; + rt->gcWeakMapList = this; + } else { + // If we're not actually doing garbage collection, the keys won't be marked + // nicely as needed by the true ephemeral marking algorithm --- custom tracers + // must use their own means for cycle detection. So here we do a conservative + // approximation: pretend all keys are live. + nonMarkingTrace(tracer); + } + } + // Garbage collector entry points. + + // Check all weak maps that have been marked as live so far in this garbage + // collection, and mark the values of all entries that have become strong references + // to them. Return true if we marked any new values, indicating that we need to make + // another pass. In other words, mark my marked maps' marked members' mid-collection. + static bool markAllIteratively(JSTracer *tracer); + + // Remove entries whose keys are dead from all weak maps marked as live in this + // garbage collection. + static void sweepAll(JSTracer *tracer); protected: - static void mark(JSTracer *trc, JSObject *obj); - static void finalize(JSContext *cx, JSObject *obj); + // Instance member functions called by the above. Instantiations of WeakMap override + // these with definitions appropriate for their Key and Value types. + virtual void nonMarkingTrace(JSTracer *tracer) = 0; + virtual bool markIteratively(JSTracer *tracer) = 0; + virtual void sweep(JSTracer *tracer) = 0; + + private: + // Link in a list of all the WeakMaps we have marked in this garbage collection, + // headed by JSRuntime::gcWeakMapList. + WeakMapBase *next; +}; + +template , + class MarkPolicy = DefaultMarkPolicy > +class WeakMap : public HashMap, public WeakMapBase { + private: + typedef HashMap Base; + typedef typename Base::Range Range; + typedef typename Base::Enum Enum; public: - WeakMap(JSContext *cx); + WeakMap(JSContext *cx) : Base(cx) { } - static JSBool construct(JSContext *cx, uintN argc, Value *vp); + private: + void nonMarkingTrace(JSTracer *tracer) { + MarkPolicy t(tracer); + for (Range r = Base::all(); !r.empty(); r.popFront()) { + t.markKey(r.front().key, "WeakMap entry key"); + t.markValue(r.front().value, "WeakMap entry value"); + } + } - static bool markIteratively(JSTracer *trc); - static void sweep(JSContext *cx); + bool markIteratively(JSTracer *tracer) { + MarkPolicy t(tracer); + bool markedAny = false; + for (Range r = Base::all(); !r.empty(); r.popFront()) { + /* If the key is alive, mark the value if needed. */ + if (!t.valueMarked(r.front().value) && t.keyMarked(r.front().key)) { + t.markValue(r.front().value, "WeakMap entry with live key"); + /* We revived a value with children, we have to iterate again. */ + markedAny = true; + } + } + return markedAny; + } - static Class jsclass; - static JSFunctionSpec methods[]; + void sweep(JSTracer *tracer) { + MarkPolicy t(tracer); + for (Enum e(*this); !e.empty(); e.popFront()) { + if (!t.keyMarked(e.front().key)) + e.removeFront(); + } +#if DEBUG + // Once we've swept, all edges should stay within the known-live part of the graph. + for (Range r = Base::all(); !r.empty(); r.popFront()) { + JS_ASSERT(t.keyMarked(r.front().key)); + JS_ASSERT(t.valueMarked(r.front().value)); + } +#endif + } }; +// Marking policy for maps from JSObject pointers to js::Values. +template <> +class DefaultMarkPolicy { + private: + JSTracer *tracer; + public: + DefaultMarkPolicy(JSTracer *t) : tracer(t) { } + bool keyMarked(JSObject *k) { return !IsAboutToBeFinalized(tracer->context, k); } + bool valueMarked(const Value &v) { + if (v.isMarkable()) + return !IsAboutToBeFinalized(tracer->context, v.toGCThing()); + else + return true; + } + void markKey(JSObject *k, const char *description) { + js::gc::MarkObject(tracer, *k, description); + } + void markValue(const Value &v, const char *description) { + js::gc::MarkValue(tracer, v, description); + } +}; + +// The class of JavaScript WeakMap objects. +extern Class WeakMapClass; + } extern JSObject * diff --git a/js/src/tests/js1_8_5/extensions/weakmap.js b/js/src/tests/js1_8_5/extensions/weakmap.js index 92eee6f872ae..934975a92d96 100644 --- a/js/src/tests/js1_8_5/extensions/weakmap.js +++ b/js/src/tests/js1_8_5/extensions/weakmap.js @@ -95,6 +95,10 @@ function test() check(function() typeof map.get(key) == "undefined"); check(function() !map.has(key)); + var value = { }; + map.set(new Object(), value); + gc(); gc(); gc(); + print ("done"); reportCompare(0, TestFailCount, "weak map tests");