From 92e575160e836f453517ecd9ce7acd4f77124cb7 Mon Sep 17 00:00:00 2001 From: Jon Coppeard Date: Fri, 16 May 2014 09:44:11 +0100 Subject: [PATCH] Bug 982561 - Convert JSCompartment's list of marked weakmaps into list of all weakmaps r=terrence --- js/src/jsgc.cpp | 21 +++++---- js/src/jsweakmap.cpp | 98 ++++++++++++++++++++++++++-------------- js/src/jsweakmap.h | 78 +++++++++++++------------------- js/src/vm/WeakMapPtr.cpp | 1 - 4 files changed, 109 insertions(+), 89 deletions(-) diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 45e697e6eaa..28b90037bdb 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -2972,8 +2972,8 @@ GCRuntime::beginMarkPhase() } for (GCCompartmentsIter c(rt); !c.done(); c.next()) { - /* Reset weak map list for the compartments being collected. */ - WeakMapBase::resetCompartmentWeakMapList(c); + /* Unmark all weak maps in the compartments being collected. */ + WeakMapBase::unmarkCompartment(c); } if (isFull) @@ -3173,14 +3173,17 @@ js::gc::MarkingValidator::nonIncrementalMark() } /* - * Temporarily clear the lists of live weakmaps and array buffers for the - * compartments we are collecting. + * Temporarily clear the weakmaps' mark flags and the lists of live array + * buffers for the compartments we are collecting. */ - WeakMapVector weakmaps; + WeakMapSet markedWeakMaps; + if (!markedWeakMaps.init()) + return; + ArrayBufferVector arrayBuffers; for (GCCompartmentsIter c(runtime); !c.done(); c.next()) { - if (!WeakMapBase::saveCompartmentWeakMapList(c, weakmaps) || + if (!WeakMapBase::saveCompartmentMarkedWeakMaps(c, markedWeakMaps) || !ArrayBufferObject::saveArrayBufferList(c, arrayBuffers)) { return; @@ -3194,7 +3197,7 @@ js::gc::MarkingValidator::nonIncrementalMark() initialized = true; for (GCCompartmentsIter c(runtime); !c.done(); c.next()) { - WeakMapBase::resetCompartmentWeakMapList(c); + WeakMapBase::unmarkCompartment(c); ArrayBufferObject::resetArrayBufferList(c); } @@ -3250,10 +3253,10 @@ js::gc::MarkingValidator::nonIncrementalMark() } for (GCCompartmentsIter c(runtime); !c.done(); c.next()) { - WeakMapBase::resetCompartmentWeakMapList(c); + WeakMapBase::unmarkCompartment(c); ArrayBufferObject::resetArrayBufferList(c); } - WeakMapBase::restoreCompartmentWeakMapLists(weakmaps); + WeakMapBase::restoreCompartmentMarkedWeakMaps(markedWeakMaps); ArrayBufferObject::restoreArrayBufferLists(arrayBuffers); gc->incrementalState = state; diff --git a/js/src/jsweakmap.cpp b/js/src/jsweakmap.cpp index 52ad5818235..c4459e5c71d 100644 --- a/js/src/jsweakmap.cpp +++ b/js/src/jsweakmap.cpp @@ -24,14 +24,47 @@ using namespace js; WeakMapBase::WeakMapBase(JSObject *memOf, JSCompartment *c) : memberOf(memOf), compartment(c), - next(WeakMapNotInList) + next(WeakMapNotInList), + marked(false) { JS_ASSERT_IF(memberOf, memberOf->compartment() == c); } WeakMapBase::~WeakMapBase() { - JS_ASSERT(next == WeakMapNotInList); + JS_ASSERT(!isInList()); +} + +void +WeakMapBase::trace(JSTracer *tracer) +{ + JS_ASSERT(isInList()); + if (IS_GC_MARKING_TRACER(tracer)) { + // We don't trace any of the WeakMap entries at this time, just record + // record the fact that the WeakMap has been marked. Enties are marked + // in the iterative marking phase by markAllIteratively(), which happens + // when many keys as possible have been marked already. + JS_ASSERT(tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps); + marked = true; + } 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 + // such as the cycle collector must use their own means for cycle detection. + // So here we do a conservative approximation: pretend all keys are live. + if (tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps) + return; + + nonMarkingTraceValues(tracer); + if (tracer->eagerlyTraceWeakMaps() == TraceWeakMapKeysValues) + nonMarkingTraceKeys(tracer); + } +} + +void +WeakMapBase::unmarkCompartment(JSCompartment *c) +{ + for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next) + m->marked = false; } bool @@ -39,7 +72,7 @@ WeakMapBase::markCompartmentIteratively(JSCompartment *c, JSTracer *tracer) { bool markedAny = false; for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next) { - if (m->markIteratively(tracer)) + if (m->marked && m->markIteratively(tracer)) markedAny = true; } return markedAny; @@ -48,8 +81,25 @@ WeakMapBase::markCompartmentIteratively(JSCompartment *c, JSTracer *tracer) void WeakMapBase::sweepCompartment(JSCompartment *c) { + WeakMapBase **tailPtr = &c->gcWeakMapList; + for (WeakMapBase *m = c->gcWeakMapList, *next; m; m = next) { + next = m->next; + if (m->marked) { + m->sweep(); + *tailPtr = m; + tailPtr = &m->next; + } else { + /* Destroy the hash map now to catch any use after this point. */ + m->finish(); + m->next = WeakMapNotInList; + } + } + *tailPtr = nullptr; + +#ifdef DEBUG for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next) - m->sweep(); + JS_ASSERT(m->isInList() && m->marked); +#endif } void @@ -62,41 +112,24 @@ WeakMapBase::traceAllMappings(WeakMapTracer *tracer) } } -void -WeakMapBase::resetCompartmentWeakMapList(JSCompartment *c) -{ - JS_ASSERT(WeakMapNotInList != nullptr); - - WeakMapBase *m = c->gcWeakMapList; - c->gcWeakMapList = nullptr; - while (m) { - WeakMapBase *n = m->next; - m->next = WeakMapNotInList; - m = n; - } -} - bool -WeakMapBase::saveCompartmentWeakMapList(JSCompartment *c, WeakMapVector &vector) +WeakMapBase::saveCompartmentMarkedWeakMaps(JSCompartment *c, WeakMapSet &markedWeakMaps) { - WeakMapBase *m = c->gcWeakMapList; - while (m) { - if (!vector.append(m)) + for (WeakMapBase *m = c->gcWeakMapList; m; m = m->next) { + if (m->marked && !markedWeakMaps.put(m)) return false; - m = m->next; } return true; } void -WeakMapBase::restoreCompartmentWeakMapLists(WeakMapVector &vector) +WeakMapBase::restoreCompartmentMarkedWeakMaps(WeakMapSet &markedWeakMaps) { - for (WeakMapBase **p = vector.begin(); p != vector.end(); p++) { - WeakMapBase *m = *p; - JS_ASSERT(m->next == WeakMapNotInList); - JSCompartment *c = m->compartment; - m->next = c->gcWeakMapList; - c->gcWeakMapList = m; + for (WeakMapSet::Range r = markedWeakMaps.all(); !r.empty(); r.popFront()) { + WeakMapBase *map = r.front(); + JS_ASSERT(map->compartment->zone()->isGCMarking()); + JS_ASSERT(!map->marked); + map->marked = true; } } @@ -166,8 +199,8 @@ WeakMap_clear_impl(JSContext *cx, CallArgs args) { JS_ASSERT(IsWeakMap(args.thisv())); - // We can't js_delete the weakmap because the data gathered during GC - // is used by the Cycle Collector + // We can't js_delete the weakmap because the data gathered during GC is + // used by the Cycle Collector. if (ObjectValueMap *map = args.thisv().toObject().as().getMap()) map->clear(); @@ -397,7 +430,6 @@ static void WeakMap_finalize(FreeOp *fop, JSObject *obj) { if (ObjectValueMap *map = obj->as().getMap()) { - map->check(); #ifdef DEBUG map->~ObjectValueMap(); memset(static_cast(map), 0xdc, sizeof(*map)); diff --git a/js/src/jsweakmap.h b/js/src/jsweakmap.h index cfd375e16ee..0f1bc6d036a 100644 --- a/js/src/jsweakmap.h +++ b/js/src/jsweakmap.h @@ -33,7 +33,7 @@ namespace js { // The value for the next pointer for maps not in the map list. static WeakMapBase * const WeakMapNotInList = reinterpret_cast(1); -typedef Vector WeakMapVector; +typedef HashSet, SystemAllocPolicy> WeakMapSet; // Common base class for all WeakMap specializations. The collector uses this to call // their markIteratively and sweep methods. @@ -42,62 +42,35 @@ class WeakMapBase { WeakMapBase(JSObject *memOf, JSCompartment *c); virtual ~WeakMapBase(); - 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. - JS_ASSERT(tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps); - - // Add ourselves to the list if we are not already in the list. We can already - // be in the list if the weak map is marked more than once due delayed marking. - if (next == WeakMapNotInList) { - next = compartment->gcWeakMapList; - compartment->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 - // such as the cycle collector must use their own means for cycle detection. - // So here we do a conservative approximation: pretend all keys are live. - if (tracer->eagerlyTraceWeakMaps() == DoNotTraceWeakMaps) - return; - - nonMarkingTraceValues(tracer); - if (tracer->eagerlyTraceWeakMaps() == TraceWeakMapKeysValues) - nonMarkingTraceKeys(tracer); - } - } + void trace(JSTracer *tracer); // Garbage collector entry points. + // Unmark all weak maps in a compartment. + static void unmarkCompartment(JSCompartment *c); + // Check all weak maps in a compartment that have been marked as live 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 markCompartmentIteratively(JSCompartment *c, JSTracer *tracer); - // Remove entries whose keys are dead from all weak maps in a compartment marked as - // live in this garbage collection. + // Sweep the weak maps in a compartment, removing dead weak maps and removing + // entries of live weak maps whose keys are dead. static void sweepCompartment(JSCompartment *c); // Trace all delayed weak map bindings. Used by the cycle collector. static void traceAllMappings(WeakMapTracer *tracer); bool isInList() { return next != WeakMapNotInList; } - void check() { JS_ASSERT(!isInList()); } - // Remove everything from the weak map list for a compartment. - static void resetCompartmentWeakMapList(JSCompartment *c); + // Save information about which weak maps are marked for a compartment. + static bool saveCompartmentMarkedWeakMaps(JSCompartment *c, WeakMapSet &markedWeakMaps); - // Save the live weak map list for a compartment, appending the data to a vector. - static bool saveCompartmentWeakMapList(JSCompartment *c, WeakMapVector &vector); + // Restore information about which weak maps are marked for many compartments. + static void restoreCompartmentMarkedWeakMaps(WeakMapSet &markedWeakMaps); - // Restore live weak map lists for multiple compartments from a vector. - static void restoreCompartmentWeakMapLists(WeakMapVector &vector); - - // Remove a weakmap from the live weakmaps list + // Remove a weakmap from its compartment's weakmaps list. static void removeWeakMapFromList(WeakMapBase *weakmap); protected: @@ -108,6 +81,7 @@ class WeakMapBase { virtual bool markIteratively(JSTracer *tracer) = 0; virtual void sweep() = 0; virtual void traceMappings(WeakMapTracer *tracer) = 0; + virtual void finish() = 0; // Object that this weak map is part of, if any. JSObject *memberOf; @@ -115,14 +89,13 @@ class WeakMapBase { // Compartment that this weak map is part of. JSCompartment *compartment; - private: - // Link in a list of WeakMaps to mark iteratively and sweep in this garbage - // collection, headed by JSCompartment::gcWeakMapList. The last element of - // the list has nullptr as its next. Maps not in the list have - // WeakMapNotInList as their next. We must distinguish these cases to - // avoid creating infinite lists when a weak map gets traced twice due to - // delayed marking. + // Link in a list of all WeakMaps in a compartment, headed by + // JSCompartment::gcWeakMapList. The last element of the list has nullptr as + // its next. Maps not in the list have WeakMapNotInList as their next. WeakMapBase *next; + + // Whether this object has been traced during garbage collection. + bool marked; }; template , publ explicit WeakMap(JSContext *cx, JSObject *memOf = nullptr) : Base(cx->runtime()), WeakMapBase(memOf, cx->compartment()) { } + bool init(uint32_t len = 16) { + if (!Base::init(len)) + return false; + next = compartment->gcWeakMapList; + compartment->gcWeakMapList = this; + marked = JS::IsIncrementalGCInProgress(compartment->runtimeFromMainThread()); + return true; + } + private: bool markValue(JSTracer *trc, Value *x) { if (gc::IsMarked(x)) @@ -216,6 +198,10 @@ class WeakMap : public HashMap, publ assertEntriesNotAboutToBeFinalized(); } + void finish() { + Base::finish(); + } + /* memberOf can be nullptr, which means that the map is not part of a JSObject. */ void traceMappings(WeakMapTracer *tracer) { for (Range r = Base::all(); !r.empty(); r.popFront()) { diff --git a/js/src/vm/WeakMapPtr.cpp b/js/src/vm/WeakMapPtr.cpp index ac620a0a16f..a6c2208a737 100644 --- a/js/src/vm/WeakMapPtr.cpp +++ b/js/src/vm/WeakMapPtr.cpp @@ -58,7 +58,6 @@ JS::WeakMapPtr::destroy() // of known live weakmaps. If we are, remove ourselves before deleting. if (map->isInList()) WeakMapBase::removeWeakMapFromList(map); - map->check(); js_delete(map); ptr = nullptr; }