commit 448d989e1e3be5c6dbffdd94eded49a2ce09258e
parent 47845a79208786f8de20475ebe424d5a0128e5e1
Author: Cristian Tuns <ctuns@mozilla.com>
Date: Tue, 14 Oct 2025 00:40:55 -0400
Revert "Bug 1935829 - Use tagged ptrs for EphemeronEdge r=jonco" for causing SM bustages in Sweeping.cpp
This reverts commit 47845a79208786f8de20475ebe424d5a0128e5e1.
Revert "Bug 1935829 - Prevent no-progress enterWeakMarkingMode churning by finishing a GC if necessary r=jonco"
This reverts commit c70a763817dac5749308e67f5f57057f32021595.
Diffstat:
10 files changed, 31 insertions(+), 177 deletions(-)
diff --git a/js/public/SliceBudget.h b/js/public/SliceBudget.h
@@ -82,11 +82,6 @@ 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),
@@ -126,9 +121,7 @@ class JS_PUBLIC_API SliceBudget {
}
}
- bool isOverBudget() {
- return counter <= 0 && !keepGoing && checkOverBudget();
- }
+ bool isOverBudget() { return counter <= 0 && checkOverBudget(); }
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,12 +2901,6 @@ 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,14 +1858,8 @@ 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, "%swork(%" PRId64 ")", nonstop,
- workBudget());
+ return snprintf(buffer, maxlen, "work(%" PRId64 ")", workBudget());
}
const char* interruptStr = "";
@@ -1876,7 +1870,7 @@ int SliceBudget::describe(char* buffer, size_t maxlen) const {
if (idle) {
extra = extended ? " (started idle but extended)" : " (idle)";
}
- return snprintf(buffer, maxlen, "%s%s%" PRId64 "ms%s", nonstop, interruptStr,
+ return snprintf(buffer, maxlen, "%s%" PRId64 "ms%s", interruptStr,
timeBudget(), extra);
}
@@ -4597,7 +4591,6 @@ 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/GCMarker.h b/js/src/gc/GCMarker.h
@@ -53,34 +53,16 @@ class MarkStackIter;
class ParallelMarkTask;
class UnmarkGrayTracer;
-// Ephemerons are edges from a source to a target that are only materialized
-// into a table when the owner is marked. (The owner is something like a
-// WeakMap, which contains a set of ephemerons each going from a WeakMap key to
-// its value.) When marking a ephemeron, only the color of the owner is needed:
-// the target is marked with the minimum (least-marked) color of the owner and
-// source. So an EphemeronEdge need store only the owner color and the target
-// pointer, which can fit into a tagged pointer since targets are aligned Cells.
-//
-// Note: if the owner's color changes, new EphemeronEdges will be created for
-// it.
-class EphemeronEdge {
- static constexpr uintptr_t ColorMask = 0x3;
- static_assert(uintptr_t(MarkColor::Gray) <= ColorMask);
- static_assert(uintptr_t(MarkColor::Black) <= ColorMask);
- static_assert(ColorMask < CellAlignBytes);
-
- uintptr_t taggedTarget;
-
- public:
- EphemeronEdge(MarkColor color, Cell* cell)
- : taggedTarget(uintptr_t(cell) | uintptr_t(color)) {
- MOZ_ASSERT((uintptr_t(cell) & ColorMask) == 0);
- }
-
- MarkColor color() const { return MarkColor(taggedTarget & ColorMask); }
- Cell* target() const {
- return reinterpret_cast<Cell*>(taggedTarget & ~ColorMask);
- }
+// Ephemeron edges have two source nodes and one target, and mark the target
+// with the minimum (least-marked) color of the sources. Currently, one of
+// those sources will always be a WeakMapBase, so this will refer to its color
+// at the time the edge is traced through. The other source's color will be
+// given by the current mark color of the GCMarker.
+struct EphemeronEdge {
+ MarkColor color;
+ Cell* target;
+
+ EphemeronEdge(MarkColor color_, Cell* cell) : color(color_), target(cell) {}
};
using EphemeronEdgeVector = Vector<EphemeronEdge, 2, js::SystemAllocPolicy>;
diff --git a/js/src/gc/GCRuntime.h b/js/src/gc/GCRuntime.h
@@ -1055,9 +1055,6 @@ 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
@@ -742,10 +742,10 @@ void GCMarker::markEphemeronEdges(EphemeronEdgeVector& edges,
DebugOnly<size_t> initialLength = edges.length();
for (auto& edge : edges) {
- MarkColor targetColor = std::min(srcColor, MarkColor(edge.color()));
+ MarkColor targetColor = std::min(srcColor, MarkColor(edge.color));
MOZ_ASSERT(markColor() >= targetColor);
if (targetColor == markColor()) {
- ApplyGCThingTyped(edge.target(), edge.target()->getTraceKind(),
+ ApplyGCThingTyped(edge.target, edge.target->getTraceKind(),
[this](auto t) {
markAndTraverse<MarkingOptions::MarkImplicitEdges>(t);
});
@@ -762,7 +762,7 @@ void GCMarker::markEphemeronEdges(EphemeronEdgeVector& edges,
// delegate zone to get marked later, look up an edge in this table, and
// then try to mark something in a Zone that is no longer marking.
if (srcColor == MarkColor::Black && markColor() == MarkColor::Black) {
- edges.eraseIf([](auto& edge) { return edge.color() == MarkColor::Black; });
+ edges.eraseIf([](auto& edge) { return edge.color == MarkColor::Black; });
}
}
@@ -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(1 + numEdges);
- if (budget.isOverBudget()) {
- return NotFinished;
+ budget.step(steps);
+ if (budget.isOverBudget()) {
+ return NotFinished;
+ }
}
}
diff --git a/js/src/gc/Sweeping.cpp b/js/src/gc/Sweeping.cpp
@@ -615,31 +615,6 @@ 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
@@ -684,7 +659,6 @@ IncrementalProgress GCRuntime::markWeakReferences(
}
assertNoMarkingWork();
- checkSlowEnter.release(); // No need to lengthen next slice.
return Finished;
}
@@ -1309,9 +1283,6 @@ 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;
@@ -1638,7 +1609,6 @@ 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/gc/Zone.cpp b/js/src/gc/Zone.cpp
@@ -257,10 +257,7 @@ static void EraseIf(js::gc::EphemeronEdgeVector& entries, Pred pred) {
static void SweepEphemeronEdgesWhileMinorSweeping(
js::gc::EphemeronEdgeVector& entries) {
EraseIf(entries, [](js::gc::EphemeronEdge& edge) -> bool {
- Cell* target = edge.target();
- bool dead = IsAboutToBeFinalizedDuringMinorSweep(&target);
- edge = js::gc::EphemeronEdge(edge.color(), target);
- return dead;
+ return IsAboutToBeFinalizedDuringMinorSweep(&edge.target);
});
}
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,18 +28,11 @@ function reportMarks(prefix = "") {
return markstr;
}
-function startGCMarking(untilQueuePos) {
+function startGCMarking() {
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() {
@@ -61,7 +54,7 @@ function purgeKey() {
vals.key = vals.val = null;
- startGCMarking(2);
+ startGCMarking();
// getMarks() returns map/key/value
assertEq(getMarks().join("/"), "black/unmarked/unmarked",
"marked the map black");
@@ -101,7 +94,7 @@ function removeKey() {
enqueueMark(m);
enqueueMark("yield");
- startGCMarking(2);
+ startGCMarking();
reportMarks("first: ");
var marks = getMarks();
assertEq(marks[0], "black", "map is black");
@@ -119,7 +112,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(2);
+ startGCMarking();
marks = getMarks();
assertEq(marks[0], "black", "map is black");
assertEq(marks[1], "unmarked", "key not marked yet");
@@ -321,7 +314,7 @@ function grayMarkingMapFirst() {
};
print("Starting incremental GC");
- startGCMarking(2);
+ startGCMarking();
// Checkpoint 1, after marking map
showmarks();
var marks = getMarks();
@@ -413,7 +406,7 @@ function grayMarkingMapLast() {
};
print("Starting incremental GC");
- startGCMarking(3);
+ startGCMarking();
// Checkpoint 1, after marking key
showmarks();
var marks = labeledMarks();
@@ -478,7 +471,7 @@ function grayMapKey() {
vals.key = vals.val = null;
- startGCMarking(4);
+ startGCMarking();
assertEq(getMarks().join("/"), "gray/unmarked/unmarked",
"marked the map gray");
@@ -611,7 +604,7 @@ function blackDuringGray() {
};
print("Starting incremental GC");
- startGCMarking(2);
+ startGCMarking();
// Checkpoint 1, after marking delegate black
showmarks();
var marks = getMarks();
@@ -673,7 +666,7 @@ function blackDuringGrayImplicit() {
};
print("Starting incremental GC");
- startGCMarking(3);
+ startGCMarking();
// 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
@@ -1,65 +0,0 @@
-// 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);
