commit a3b31c015d6d9f3565aa7b1804ce28719a6e4a6e
parent ed8bfd0077d5b18ed54d24bede9384b30af838ae
Author: Jon Coppeard <jcoppeard@mozilla.com>
Date: Tue, 18 Nov 2025 09:16:38 +0000
Bug 1995021 - Sweep nursery weakmap keys on minor GC r=sfink
Sweep nursery weakmap keys at the end of minor GC and remove the entries for
any that are dead. We still have to mark keys with delegates at the start.
Differential Revision: https://phabricator.services.mozilla.com/D270563
Diffstat:
6 files changed, 225 insertions(+), 11 deletions(-)
diff --git a/js/src/gc/Nursery.cpp b/js/src/gc/Nursery.cpp
@@ -1713,7 +1713,11 @@ void js::Nursery::traceRoots(AutoGCSession& session, TenuringTracer& mover) {
}
endProfile(ProfileKey::MarkDebugger);
+ // This should happen after debugger marking as this also marks weak map
+ // entries.
+ startProfile(ProfileKey::TraceWeakMaps);
traceWeakMaps(mover);
+ endProfile(ProfileKey::TraceWeakMaps);
}
bool js::Nursery::shouldTenureEverything(JS::GCReason reason) {
@@ -2065,6 +2069,8 @@ void js::Nursery::sweep() {
sweepMapAndSetObjects();
+ sweepWeakMaps();
+
runtime()->caches().sweepAfterMinorGC(&trc);
}
@@ -2596,12 +2602,23 @@ void js::Nursery::sweepMapAndSetObjects() {
}
void Nursery::traceWeakMaps(TenuringTracer& trc) {
- // Conservatively assume all weak map keys and values are live.
MOZ_ASSERT(trc.weakMapAction() == JS::WeakMapTraceAction::TraceKeysAndValues);
weakMapsWithNurseryEntries_.eraseIf(
[&](WeakMapBase* wm) { return wm->traceNurseryEntriesOnMinorGC(&trc); });
}
+void js::Nursery::sweepWeakMaps() {
+ // This sweeps all weak maps that contain nursery keys to remove entries for
+ // keys that have not survived. Nursery values in weak maps are always
+ // promoted.
+
+ // Don't update retained size for weak maps here.
+ AutoSetThreadGCUse setUse(runtime()->gcContext(), GCUse::Unspecified);
+
+ weakMapsWithNurseryEntries_.eraseIf(
+ [&](WeakMapBase* wm) { return wm->sweepAfterMinorGC(); });
+}
+
void js::Nursery::joinSweepTask() { sweepTask->join(); }
void js::Nursery::joinDecommitTask() { decommitTask->join(); }
diff --git a/js/src/gc/Nursery.h b/js/src/gc/Nursery.h
@@ -42,6 +42,7 @@
_(CheckHashTables, "ckTbls") \
_(MarkRuntime, "mkRntm") \
_(MarkDebugger, "mkDbgr") \
+ _(TraceWeakMaps, "trWkMp") \
_(SweepCaches, "swpCch") \
_(CollectToObjFP, "colObj") \
_(CollectToStrFP, "colStr") \
@@ -499,6 +500,7 @@ class Nursery {
void sweepMapAndSetObjects();
void traceWeakMaps(gc::TenuringTracer& trc);
+ void sweepWeakMaps();
void sweepStringsWithBuffer();
diff --git a/js/src/gc/WeakMap-inl.h b/js/src/gc/WeakMap-inl.h
@@ -102,13 +102,19 @@ void WeakMap<K, V, AP>::assertMapIsSameZoneWithValue(const BarrieredValue& v) {
#endif
}
+// Initial length chosen to give minimum table capacity on creation.
+//
+// Using the default initial length instead means we will often reallocate the
+// table on sweep because it's too big for the number of entries.
+static constexpr size_t InitialWeakMapLength = 0;
+
template <class K, class V, class AP>
WeakMap<K, V, AP>::WeakMap(JSContext* cx, JSObject* memOf)
: WeakMap(cx->zone(), memOf) {}
template <class K, class V, class AP>
WeakMap<K, V, AP>::WeakMap(JS::Zone* zone, JSObject* memOf)
- : WeakMapBase(memOf, zone), map_(zone) {
+ : WeakMapBase(memOf, zone), map_(zone, InitialWeakMapLength) {
static_assert(std::is_same_v<typename RemoveBarrier<K>::Type, K>);
static_assert(std::is_same_v<typename RemoveBarrier<V>::Type, V>);
@@ -382,6 +388,7 @@ void WeakMap<K, V, AP>::addNurseryKey(const K& key) {
if (nurseryKeys.length() >= map().count() / 2) {
// Don't bother recording every key if there a lot of them. We will scan the
// map instead.
+ nurseryKeys.clear();
nurseryKeysValid = false;
return;
}
@@ -393,6 +400,10 @@ void WeakMap<K, V, AP>::addNurseryKey(const K& key) {
template <class K, class V, class AP>
bool WeakMap<K, V, AP>::traceNurseryEntriesOnMinorGC(JSTracer* trc) {
+ // Called on minor GC to trace nursery keys that have delegates and nursery
+ // values. Nursery keys without delegates are swept at the end of minor GC if
+ // they do not survive.
+
MOZ_ASSERT(hasNurseryEntries);
using Entry = typename Map::Entry;
@@ -402,7 +413,10 @@ bool WeakMap<K, V, AP>::traceNurseryEntriesOnMinorGC(JSTracer* trc) {
bool hasNurseryValue = !JS::GCPolicy<V>::isTenured(entry.value());
MOZ_ASSERT(key == entry.key());
- TraceManuallyBarrieredEdge(trc, &key, "WeakMap nursery key");
+ JSObject* delegate = gc::detail::GetDelegate(gc::MaybeForwarded(key));
+ if (delegate) {
+ TraceManuallyBarrieredEdge(trc, &key, "WeakMap nursery key");
+ }
bool hasNurseryKey = !JS::GCPolicy<K>::isTenured(key);
bool keyUpdated = key != entry.key();
@@ -413,7 +427,19 @@ bool WeakMap<K, V, AP>::traceNurseryEntriesOnMinorGC(JSTracer* trc) {
nurseryKeys.mutableEraseIf([&](K& key) {
auto ptr = lookupUnbarriered(key);
if (!ptr) {
- return true;
+ if (!gc::IsForwarded(key)) {
+ return true;
+ }
+
+ // WeakMap::trace might have marked the key in the table already so if
+ // the key was forwarded try looking up the forwarded key too.
+ //
+ // TODO: Try to update cached nursery information there instead.
+ key = gc::Forwarded(key);
+ ptr = lookupUnbarriered(key);
+ if (!ptr) {
+ return true;
+ }
}
auto [keyUpdated, hasNurseryKeyOrValue] = traceEntry(key, *ptr);
@@ -425,7 +451,7 @@ bool WeakMap<K, V, AP>::traceNurseryEntriesOnMinorGC(JSTracer* trc) {
return !hasNurseryKeyOrValue;
});
} else {
- nurseryKeys.clear();
+ MOZ_ASSERT(nurseryKeys.empty());
nurseryKeysValid = true;
for (Enum e(*this); !e.empty(); e.popFront()) {
@@ -446,6 +472,132 @@ bool WeakMap<K, V, AP>::traceNurseryEntriesOnMinorGC(JSTracer* trc) {
}
hasNurseryEntries = !nurseryKeysValid || !nurseryKeys.empty();
+
+#ifdef DEBUG
+ bool foundNurseryEntries = false;
+ for (Enum e(*this); !e.empty(); e.popFront()) {
+ if (!JS::GCPolicy<K>::isTenured(e.front().key()) ||
+ !JS::GCPolicy<V>::isTenured(e.front().value())) {
+ foundNurseryEntries = true;
+ }
+ }
+ MOZ_ASSERT_IF(foundNurseryEntries, hasNurseryEntries);
+#endif
+
+ return !hasNurseryEntries;
+}
+
+template <class K, class V, class AP>
+bool WeakMap<K, V, AP>::sweepAfterMinorGC() {
+#ifdef DEBUG
+ MOZ_ASSERT(hasNurseryEntries);
+ bool foundNurseryEntries = false;
+ for (Enum e(*this); !e.empty(); e.popFront()) {
+ if (!JS::GCPolicy<K>::isTenured(e.front().key()) ||
+ !JS::GCPolicy<V>::isTenured(e.front().value())) {
+ foundNurseryEntries = true;
+ }
+ }
+ MOZ_ASSERT(foundNurseryEntries);
+#endif
+
+ using Entry = typename Map::Entry;
+ using Result = std::tuple<bool /* shouldRemove */, bool /* keyUpdated */,
+ bool /* hasNurseryKeyOrValue */>;
+ auto sweepEntry = [](K& key, const Entry& entry) -> Result {
+ bool hasNurseryValue = !JS::GCPolicy<V>::isTenured(entry.value());
+ MOZ_ASSERT(!gc::IsForwarded(entry.value().get()));
+
+ gc::Cell* keyCell = gc::ToMarkable(key);
+ if (!gc::InCollectedNurseryRegion(keyCell)) {
+ bool hasNurseryKey = !JS::GCPolicy<K>::isTenured(key);
+ return {false, false, hasNurseryKey || hasNurseryValue};
+ }
+
+ if (!gc::IsForwarded(key)) {
+ return {true, false, false};
+ }
+
+ key = gc::Forwarded(key);
+ MOZ_ASSERT(key != entry.key());
+
+ bool hasNurseryKey = !JS::GCPolicy<K>::isTenured(key);
+
+ return {false, true, hasNurseryKey || hasNurseryValue};
+ };
+
+ if (nurseryKeysValid) {
+ nurseryKeys.mutableEraseIf([&](K& key) {
+ auto ptr = lookupUnbarriered(key);
+ if (!ptr) {
+ if (!gc::IsForwarded(key)) {
+ return true;
+ }
+
+ // WeakMap::trace might have marked the key in the table already so if
+ // the key was forwarded try looking up the forwarded key too.
+ //
+ // TODO: Try to update cached nursery information there instead.
+ key = gc::Forwarded(key);
+ ptr = lookupUnbarriered(key);
+ if (!ptr) {
+ return true;
+ }
+ }
+
+ auto [shouldRemove, keyUpdated, hasNurseryKeyOrValue] =
+ sweepEntry(key, *ptr);
+ if (shouldRemove) {
+ map().remove(ptr);
+ return true;
+ }
+
+ if (keyUpdated) {
+ map().rekeyAs(ptr->key(), key, key);
+ }
+
+ return !hasNurseryKeyOrValue;
+ });
+ } else {
+ MOZ_ASSERT(nurseryKeys.empty());
+ nurseryKeysValid = true;
+
+ for (Enum e(*this); !e.empty(); e.popFront()) {
+ Entry& entry = e.front();
+
+ K key = entry.key();
+ auto [shouldRemove, keyUpdated, hasNurseryKeyOrValue] =
+ sweepEntry(key, entry);
+
+ if (shouldRemove) {
+ e.removeFront();
+ continue;
+ }
+
+ if (keyUpdated) {
+ entry.mutableKey() = key;
+ e.rekeyFront(key);
+ }
+
+ if (hasNurseryKeyOrValue) {
+ addNurseryKey(key);
+ }
+ }
+ }
+
+ hasNurseryEntries = !nurseryKeysValid || !nurseryKeys.empty();
+
+#ifdef DEBUG
+ foundNurseryEntries = false;
+ for (Enum e(*this); !e.empty(); e.popFront()) {
+ if (!JS::GCPolicy<K>::isTenured(e.front().key()) ||
+ !JS::GCPolicy<V>::isTenured(e.front().value())) {
+ foundNurseryEntries = true;
+ }
+ }
+ MOZ_ASSERT_IF(foundNurseryEntries, hasNurseryEntries);
+#endif
+
return !hasNurseryEntries;
}
diff --git a/js/src/gc/WeakMap.h b/js/src/gc/WeakMap.h
@@ -148,9 +148,6 @@ class WeakMapBase : public mozilla::LinkedListElement<WeakMapBase> {
[[nodiscard]] static bool findSweepGroupEdgesForZone(JS::Zone* atomsZone,
JS::Zone* mapZone);
- // Sweep the marked weak maps in a zone, updating moved keys.
- static void sweepZoneAfterMinorGC(JS::Zone* zone);
-
// Trace all weak map bindings. Used by the cycle collector.
static void traceAllMappings(WeakMapTracer* tracer);
@@ -183,6 +180,7 @@ class WeakMapBase : public mozilla::LinkedListElement<WeakMapBase> {
// Trace any keys and values that are in the nursery. Return false if any
// remain in the nursery.
virtual bool traceNurseryEntriesOnMinorGC(JSTracer* trc) = 0;
+ virtual bool sweepAfterMinorGC() = 0;
// We have a key that, if it or its delegate is marked, may lead to a WeakMap
// value getting marked. Insert the necessary edges into the appropriate
@@ -516,6 +514,7 @@ class WeakMap : public WeakMapBase {
void traceMappings(WeakMapTracer* tracer) override;
bool traceNurseryEntriesOnMinorGC(JSTracer* trc) override;
+ bool sweepAfterMinorGC() override;
};
} /* namespace js */
diff --git a/js/src/jit-test/tests/gc/bug-1995021.js b/js/src/jit-test/tests/gc/bug-1995021.js
@@ -0,0 +1,36 @@
+// Test basic behaviour of weak maps with nursery keys.
+
+gczeal(0);
+
+let wm = new WeakMap();
+
+function size() {
+ return nondeterministicGetWeakMapSize(wm);
+}
+
+assertEq(size(), 0);
+
+wm.set({}, {});
+assertEq(size(), 1);
+
+minorgc();
+assertEq(size(), 0);
+
+let o = {};
+wm.set(o, {});
+minorgc();
+assertEq(size(), 1);
+
+o = undefined;
+gc();
+assertEq(size(), 0);
+
+let g = newGlobal({newCompartment: true});
+o = g.eval('new Object()');
+wm.set(o, {});
+minorgc();
+assertEq(size(), 1);
+
+o = undefined;
+gc();
+assertEq(size(), 0);
diff --git a/js/src/jit-test/tests/gc/weak-marking-budget.js b/js/src/jit-test/tests/gc/weak-marking-budget.js
@@ -2,13 +2,21 @@
gczeal(0); // We need full control here.
+var keys = [];
var maps = Array(1000).fill().map(() => new WeakMap);
for (const map of maps) {
- for (let i = 0; i < 100; i++) {
- map.set({}, {}); // These will all die, but will need to be put into the ephemeron table first.
- }
+ for (let i = 0; i < 100; i++) {
+ // The key will die the next major collection , but will need to be put
+ // into the ephemeron table first.
+ let key = {};
+ keys.push(key);
+ map.set(key, {});
+ }
}
+minorgc();
+keys = undefined;
+
// Slowly work forward until we reach Mark.
startgc(10);
while (["Prepare", "MarkRoots"].includes(gcstate())) {
