commit 3d0935940c65ed6ca3053ba1575b53604014f108
parent c098d04ec19ed6bdc271ad006f5c36bb56af39a6
Author: Steve Fink <sfink@mozilla.com>
Date: Tue, 14 Oct 2025 19:24:30 +0000
Bug 1935829 - Prevent no-progress enterWeakMarkingMode churning by finishing a GC if necessary r=jonco
Differential Revision: https://phabricator.services.mozilla.com/D266261
Diffstat:
8 files changed, 158 insertions(+), 17 deletions(-)
diff --git a/js/public/SliceBudget.h b/js/public/SliceBudget.h
@@ -27,12 +27,16 @@ struct JS_PUBLIC_API TimeBudget {
: budget(mozilla::TimeDuration::FromMilliseconds(milliseconds)) {}
void setDeadlineFromNow();
+ double progress(mozilla::TimeStamp t) const {
+ return (t - (deadline - budget)) / budget;
+ }
};
struct JS_PUBLIC_API WorkBudget {
const int64_t budget;
explicit WorkBudget(int64_t work) : budget(work) {}
+ double progress(int64_t work) const { return double(work) / double(budget); }
};
struct UnlimitedBudget {};
@@ -82,6 +86,11 @@ class JS_PUBLIC_API SliceBudget {
// the predicted idle time.
bool extended = false;
+ // Temporarily allow going over budget. (isOverBudget() will return false,
+ // though step() will still have an effect and will matter once this is set
+ // back to false.)
+ bool keepGoing = false;
+
private:
explicit SliceBudget(InterruptRequestFlag* irqPtr)
: counter(irqPtr ? StepsPerExpensiveCheck : UnlimitedCounter),
@@ -121,7 +130,21 @@ class JS_PUBLIC_API SliceBudget {
}
}
- bool isOverBudget() { return counter <= 0 && checkOverBudget(); }
+ bool isOverBudget() {
+ return counter <= 0 && !keepGoing && checkOverBudget();
+ }
+
+ // Return the fraction of progress towards the deadline. Note that this can
+ // return values outside of the range [0,1].
+ double progress() const {
+ if (isUnlimited()) {
+ return 0.0;
+ }
+ if (isTimeBudget()) {
+ return budget.as<TimeBudget>().progress(mozilla::TimeStamp::Now());
+ }
+ return budget.as<WorkBudget>().progress(workBudget() - counter);
+ }
bool isWorkBudget() const { return budget.is<WorkBudget>(); }
bool isTimeBudget() const { return budget.is<TimeBudget>(); }
diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp
@@ -2901,6 +2901,12 @@ static bool CurrentGC(JSContext* cx, unsigned argc, Value* vp) {
}
# endif
+ val = BooleanValue(gc.finishMarkingDuringSweeping);
+ if (!JS_DefineProperty(cx, result, "finishMarkingDuringSweeping", val,
+ JSPROP_ENUMERATE)) {
+ return false;
+ }
+
args.rval().setObject(*result);
return true;
}
diff --git a/js/src/gc/GC.cpp b/js/src/gc/GC.cpp
@@ -1858,8 +1858,14 @@ int SliceBudget::describe(char* buffer, size_t maxlen) const {
return snprintf(buffer, maxlen, "unlimited");
}
+ const char* nonstop = "";
+ if (keepGoing) {
+ nonstop = "nonstop ";
+ }
+
if (isWorkBudget()) {
- return snprintf(buffer, maxlen, "work(%" PRId64 ")", workBudget());
+ return snprintf(buffer, maxlen, "%swork(%" PRId64 ")", nonstop,
+ workBudget());
}
const char* interruptStr = "";
@@ -1870,7 +1876,7 @@ int SliceBudget::describe(char* buffer, size_t maxlen) const {
if (idle) {
extra = extended ? " (started idle but extended)" : " (idle)";
}
- return snprintf(buffer, maxlen, "%s%" PRId64 "ms%s", interruptStr,
+ return snprintf(buffer, maxlen, "%s%s%" PRId64 "ms%s", nonstop, interruptStr,
timeBudget(), extra);
}
@@ -4591,6 +4597,7 @@ MOZ_NEVER_INLINE GCRuntime::IncrementalResult GCRuntime::gcCycle(
IncrementalResult result =
budgetIncrementalGC(nonincrementalByAPI, reason, budget);
+
if (result != IncrementalResult::Ok && incrementalState == State::NotActive) {
// The collection was reset or aborted and has finished.
return result;
diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h
@@ -1055,6 +1055,9 @@ class GCRuntime {
GCSchedulingTunables tunables;
GCSchedulingState schedulingState;
MainThreadData<bool> fullGCRequested;
+ // If an enterWeakMarking slice takes too long, suppress yielding during the
+ // next slice.
+ MainThreadData<bool> finishMarkingDuringSweeping;
// Helper thread configuration.
MainThreadData<double> helperThreadRatio;
diff --git a/js/src/gc/Marking.cpp b/js/src/gc/Marking.cpp
@@ -2408,13 +2408,13 @@ IncrementalProgress JS::Zone::enterWeakMarkingMode(GCMarker* marker,
CellColor srcColor = gc::detail::GetEffectiveColor(marker, src);
auto& edges = r.front().value();
+ size_t numEdges = edges.length();
if (IsMarked(srcColor) && edges.length() > 0) {
- uint32_t steps = edges.length();
marker->markEphemeronEdges(edges, AsMarkColor(srcColor));
- budget.step(steps);
- if (budget.isOverBudget()) {
- return NotFinished;
- }
+ }
+ budget.step(1 + numEdges);
+ if (budget.isOverBudget()) {
+ return NotFinished;
}
}
diff --git a/js/src/gc/Sweeping.cpp b/js/src/gc/Sweeping.cpp
@@ -615,6 +615,31 @@ IncrementalProgress GCRuntime::markWeakReferences(
auto leaveOnExit =
mozilla::MakeScopeExit([&] { marker().leaveWeakMarkingMode(); });
+ // If enterWeakMarkingMode takes up at least 80% of a slice, finish marking
+ // completely in the next slice before yielding again. This avoids the problem
+ // where scanning gcEphemeronEdges (which must be done at the beginning of
+ // each slice) takes longer than a slice and therefore no (or little) progress
+ // can be made per slice.
+ double progressBeforeEnterWMM = budget.progress();
+ auto checkSlowEnter = mozilla::MakeScopeExit([&] {
+ // Called only when returning NotFinished.
+ if (budget.progress() - progressBeforeEnterWMM > 0.8) {
+ // Overran the budget. Finish the marking synchronously in the next slice.
+ // Repeatedly returning to the mutator would require re-scanning the full
+ // edge table in every slice, and we already know that this will take up
+ // most or all of a single slice budget.
+ finishMarkingDuringSweeping = true;
+ }
+ });
+
+ // The previous logic is for the first enterWeakMarkingMode slice. This logic
+ // then kicks in for the next slice, to update the budget to actually keep
+ // going.
+ if (!budget.isUnlimited() && finishMarkingDuringSweeping) {
+ JS_LOG(gc, Info, "enterWeakMarkingMode finishing marking in next slice");
+ budget.keepGoing = true;
+ }
+
if (marker().enterWeakMarkingMode()) {
// If there was an 'enter-weak-marking-mode' token in the queue, then it
// and everything after it will still be in the queue so we can process
@@ -659,6 +684,7 @@ IncrementalProgress GCRuntime::markWeakReferences(
}
assertNoMarkingWork();
+ checkSlowEnter.release(); // No need to lengthen next slice.
return Finished;
}
@@ -1283,6 +1309,9 @@ IncrementalProgress GCRuntime::endMarkingSweepGroup(JS::GCContext* gcx,
// We must not yield after this point before we start sweeping the group.
safeToYield = false;
+ // If we temporarily prevented yielding during marking, release the hold now.
+ budget.keepGoing = false;
+
MaybeCheckWeakMapMarking(this);
return Finished;
@@ -1609,6 +1638,7 @@ IncrementalProgress GCRuntime::beginSweepingSweepGroup(JS::GCContext* gcx,
using namespace gcstats;
AutoSCC scc(stats(), sweepGroupIndex);
+ finishMarkingDuringSweeping = false;
bool sweepingAtoms = false;
for (SweepGroupZonesIter zone(this); !zone.done(); zone.next()) {
diff --git a/js/src/jit-test/tests/gc/weak-marking-03.js b/js/src/jit-test/tests/gc/weak-marking-03.js
@@ -28,11 +28,18 @@ function reportMarks(prefix = "") {
return markstr;
}
-function startGCMarking() {
+function startGCMarking(untilQueuePos) {
startgc(100000);
while (gcstate() === "Prepare" || gcstate() === "MarkRoots") {
gcslice(100000);
}
+ // If running with --no-threads, the GC might decide to suspend early if
+ // scanning the ephemeron table takes too long. Wait until all of the queue
+ // entries that the test expects have been processed.
+ while (currentgc().queuePos < untilQueuePos) {
+ gcslice(1000);
+ }
+
}
function purgeKey() {
@@ -54,7 +61,7 @@ function purgeKey() {
vals.key = vals.val = null;
- startGCMarking();
+ startGCMarking(2);
// getMarks() returns map/key/value
assertEq(getMarks().join("/"), "black/unmarked/unmarked",
"marked the map black");
@@ -94,7 +101,7 @@ function removeKey() {
enqueueMark(m);
enqueueMark("yield");
- startGCMarking();
+ startGCMarking(2);
reportMarks("first: ");
var marks = getMarks();
assertEq(marks[0], "black", "map is black");
@@ -112,7 +119,7 @@ function removeKey() {
// Do it again, but this time, remove all other roots.
m.set(vals.key, vals.val);
vals.key = vals.val = null;
- startGCMarking();
+ startGCMarking(2);
marks = getMarks();
assertEq(marks[0], "black", "map is black");
assertEq(marks[1], "unmarked", "key not marked yet");
@@ -314,7 +321,7 @@ function grayMarkingMapFirst() {
};
print("Starting incremental GC");
- startGCMarking();
+ startGCMarking(2);
// Checkpoint 1, after marking map
showmarks();
var marks = getMarks();
@@ -406,7 +413,7 @@ function grayMarkingMapLast() {
};
print("Starting incremental GC");
- startGCMarking();
+ startGCMarking(3);
// Checkpoint 1, after marking key
showmarks();
var marks = labeledMarks();
@@ -471,7 +478,7 @@ function grayMapKey() {
vals.key = vals.val = null;
- startGCMarking();
+ startGCMarking(4);
assertEq(getMarks().join("/"), "gray/unmarked/unmarked",
"marked the map gray");
@@ -604,7 +611,7 @@ function blackDuringGray() {
};
print("Starting incremental GC");
- startGCMarking();
+ startGCMarking(2);
// Checkpoint 1, after marking delegate black
showmarks();
var marks = getMarks();
@@ -666,7 +673,7 @@ function blackDuringGrayImplicit() {
};
print("Starting incremental GC");
- startGCMarking();
+ startGCMarking(3);
// Checkpoint 1, after marking map gray
showmarks();
var marks = getMarks();
diff --git a/js/src/jit-test/tests/gc/weak-marking-budget.js b/js/src/jit-test/tests/gc/weak-marking-budget.js
@@ -0,0 +1,65 @@
+// Make enterWeakMarkingMode expensive so it gets forced into a single slice.
+
+gczeal(0); // We need full control here.
+
+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.
+ }
+}
+
+// Slowly work forward until we reach Mark.
+startgc(10);
+while (["Prepare", "MarkRoots"].includes(gcstate())) {
+ gcslice(10);
+}
+assertEq(gcstate(), "Mark");
+
+// This will yield before leaving marking, in order to give the first sweep
+// slice a full budget.
+print("gcslice(10000) #1");
+gcslice(10000);
+assertEq(gcstate(), "Mark");
+
+// First Sweep slice will hit the long enterWeakMarkingMode and yield as soon as
+// the budget runs out, and set up the next Sweep slice to finish.
+print("gcslice(10000) #2");
+gcslice(10000);
+assertEq(gcstate(), "Sweep");
+hasFunction["currentgc"] && assertEq(currentgc().finishMarkingDuringSweeping, true);
+
+// This slice will finish the marking, but will go way over budget and so will
+// yield as soon as the marking is done. This will still be during Sweep (in the
+// middle of sweepWeakCaches).
+print("gcslice(1) #3");
+// Use more than gcslice(1) because it is possible to get a few things added to
+// the mark stack from read barriers.
+gcslice(100);
+assertEq(gcstate(), "Sweep");
+hasFunction["currentgc"] && assertEq(currentgc().finishMarkingDuringSweeping, false);
+
+// There's still a lot of sweeping left to do, because all of the dead stuff
+// needs to be finalized.
+finishgc();
+
+// Do another GC without a slow enterWMM, to confirm that the extra slice is not
+// requested. (The previous GC will have thrown out all of the WeakMaps'
+// entries, so this will just be doing one step for each of the 1000 WeakMaps
+// instead of 1000 * (1 + 100) for the WeakMaps plus their keys.)
+startgc(10);
+while (["Prepare", "MarkRoots"].includes(gcstate())) {
+ gcslice(10);
+}
+assertEq(gcstate(), "Mark");
+
+gcslice(10000);
+assertEq(gcstate(), "Mark");
+
+gcslice(1);
+assertEq(gcstate(), "Sweep");
+hasFunction["currentgc"] && assertEq(currentgc().finishMarkingDuringSweeping, false);
+
+gcslice(1);
+assertEq(gcstate(), "Sweep");
+hasFunction["currentgc"] && assertEq(currentgc().finishMarkingDuringSweeping, false);
