Diff
Modified: branches/safari-603-branch/Source/_javascript_Core/ChangeLog (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-13 19:18:25 UTC (rev 210738)
@@ -1,3 +1,79 @@
+2017-01-13 Matthew Hanson <matthew_han...@apple.com>
+
+ Merge r210694. rdar://problem/29983526
+
+ 2017-01-12 Saam Barati <sbar...@apple.com>
+
+ Concurrent GC has a bug where we would detect a race but fail to rescan the object
+ https://bugs.webkit.org/show_bug.cgi?id=166960
+ <rdar://problem/29983526>
+
+ Reviewed by Filip Pizlo and Mark Lam.
+
+ We have code like this in JSC:
+
+ ```
+ Butterfly* butterfly = allocateMoreOutOfLineStorage(vm, oldOutOfLineCapacity, newOutOfLineCapacity);
+ nukeStructureAndSetButterfly(vm, structureID, butterfly);
+ structure->setLastOffset(newLastOffset);
+ WTF::storeStoreFence();
+ setStructureIDDirectly(structureID);
+ ```
+
+ Note that the collector could detect a race here, which sometimes
+ incorrectly caused us to not visit the object again.
+
+ Mutator Thread: M, Collector Thread: C, assuming sequential consistency via
+ proper barriers:
+
+ M: allocate new butterfly
+ M: Set nuked structure ID
+ M: Set butterfly (this does a barrier)
+ C: Start scanning O
+ C: load structure ID
+ C: See it's nuked and bail, (we used to rely on a write barrier to rescan).
+
+ We sometimes never rescanned here because we were calling
+ setStructureIDDirectly which doesn't do a write barrier.
+ (Note, the places that do this but call setStructure were
+ OK because setStructure will perform a write barrier.)
+
+ (This same issue also existed in places where the collector thread
+ detected races for Structure::m_offset, but places that changed
+ Structure::m_offset didn't perform a write barrier on the object
+ after changing its Structure's m_offset.)
+
+ To prevent such code from requiring every call site to perform
+ a write barrier on the object, I've changed the collector code
+ to keep a stack of cells to be revisited due to races. This stack
+ is then consulted when we do marking. Because such races are rare,
+ we have a single stack on Heap that is guarded by a lock.
+
+ * heap/Heap.cpp:
+ (JSC::Heap::Heap):
+ (JSC::Heap::~Heap):
+ (JSC::Heap::markToFixpoint):
+ (JSC::Heap::endMarking):
+ (JSC::Heap::buildConstraintSet):
+ (JSC::Heap::addToRaceMarkStack):
+ * heap/Heap.h:
+ (JSC::Heap::collectorSlotVisitor):
+ (JSC::Heap::mutatorMarkStack): Deleted.
+ * heap/SlotVisitor.cpp:
+ (JSC::SlotVisitor::didRace):
+ * heap/SlotVisitor.h:
+ (JSC::SlotVisitor::didRace):
+ (JSC::SlotVisitor::didNotRace): Deleted.
+ * heap/SlotVisitorInlines.h:
+ (JSC::SlotVisitor::didNotRace): Deleted.
+ * runtime/JSObject.cpp:
+ (JSC::JSObject::visitButterfly):
+ (JSC::JSObject::visitButterflyImpl):
+ * runtime/JSObjectInlines.h:
+ (JSC::JSObject::prepareToPutDirectWithoutTransition):
+ * runtime/Structure.cpp:
+ (JSC::Structure::flattenDictionaryStructure):
+
2017-01-12 Matthew Hanson <matthew_han...@apple.com>
Merge r210609. rdar://problem/27896585
Modified: branches/safari-603-branch/Source/_javascript_Core/heap/Heap.cpp (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/heap/Heap.cpp 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/heap/Heap.cpp 2017-01-13 19:18:25 UTC (rev 210738)
@@ -259,6 +259,7 @@
, m_machineThreads(this)
, m_collectorSlotVisitor(std::make_unique<SlotVisitor>(*this))
, m_mutatorMarkStack(std::make_unique<MarkStackArray>())
+ , m_raceMarkStack(std::make_unique<MarkStackArray>())
, m_handleSet(vm)
, m_codeBlocks(std::make_unique<CodeBlockSet>())
, m_jitStubRoutines(std::make_unique<JITStubRoutineSet>())
@@ -311,6 +312,7 @@
slotVisitor->clearMarkStacks();
m_collectorSlotVisitor->clearMarkStacks();
m_mutatorMarkStack->clear();
+ m_raceMarkStack->clear();
for (WeakBlock* block : m_logicallyEmptyWeakBlocks)
WeakBlock::destroy(*this, block);
@@ -526,6 +528,8 @@
m_mutatorMarkStack->clear();
}
+ RELEASE_ASSERT(m_raceMarkStack->isEmpty());
+
beginMarking();
m_parallelMarkersShouldExit = false;
@@ -785,11 +789,6 @@
void Heap::endMarking()
{
- if (!m_visitRaces.isEmpty()) {
- dataLog("Unresolved visit races: ", listDump(m_visitRaces), "\n");
- RELEASE_ASSERT_NOT_REACHED();
- }
-
m_collectorSlotVisitor->reset();
for (auto& parallelVisitor : m_parallelSlotVisitors)
@@ -797,6 +796,8 @@
assertSharedMarkStacksEmpty();
m_weakReferenceHarvesters.removeAll();
+
+ RELEASE_ASSERT(m_raceMarkStack->isEmpty());
m_objectSpace.endMarking();
setMutatorShouldBeFenced(Options::forceFencedBarrier());
@@ -2252,10 +2253,10 @@
MarkingConstraint::GreyedByExecution);
m_constraintSet->add(
- "Mms", "Mutator Mark Stack",
+ "Mrms", "Mutator+Race Mark Stack",
[this] (SlotVisitor& slotVisitor, const VisitingTimeout&) {
// Indicate to the fixpoint that we introduced work!
- size_t size = m_mutatorMarkStack->size();
+ size_t size = m_mutatorMarkStack->size() + m_raceMarkStack->size();
slotVisitor.addToVisitCount(size);
if (Options::logGC())
@@ -2262,9 +2263,10 @@
dataLog("(", size, ")");
m_mutatorMarkStack->transferTo(slotVisitor.mutatorMarkStack());
+ m_raceMarkStack->transferTo(slotVisitor.mutatorMarkStack());
},
[this] (SlotVisitor&) -> double {
- return m_mutatorMarkStack->size();
+ return m_mutatorMarkStack->size() + m_raceMarkStack->size();
},
MarkingConstraint::GreyedByExecution);
}
Modified: branches/safari-603-branch/Source/_javascript_Core/heap/Heap.h (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/heap/Heap.h 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/heap/Heap.h 2017-01-13 19:18:25 UTC (rev 210738)
@@ -139,7 +139,6 @@
MachineThreads& machineThreads() { return m_machineThreads; }
SlotVisitor& collectorSlotVisitor() { return *m_collectorSlotVisitor; }
- MarkStackArray& mutatorMarkStack() { return *m_mutatorMarkStack; }
JS_EXPORT_PRIVATE GCActivityCallback* fullActivityCallback();
JS_EXPORT_PRIVATE GCActivityCallback* edenActivityCallback();
@@ -540,7 +539,10 @@
std::unique_ptr<SlotVisitor> m_collectorSlotVisitor;
std::unique_ptr<MarkStackArray> m_mutatorMarkStack;
-
+
+ Lock m_raceMarkStackLock;
+ std::unique_ptr<MarkStackArray> m_raceMarkStack;
+
std::unique_ptr<MarkingConstraintSet> m_constraintSet;
// We pool the slot visitors used by parallel marking threads. It's useful to be able to
@@ -597,7 +599,6 @@
HashMap<void*, std::function<void()>> m_weakGCMaps;
- HashSet<VisitRaceKey> m_visitRaces;
Lock m_visitRaceLock;
Lock m_markingMutex;
Modified: branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitor.cpp (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitor.cpp 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitor.cpp 2017-01-13 19:18:25 UTC (rev 210738)
@@ -284,7 +284,6 @@
{
ASSERT(Heap::isMarkedConcurrently(cell));
ASSERT(!cell->isZapped());
- ASSERT(cell->cellState() == CellState::PossiblyGrey);
container.noteMarked();
@@ -702,10 +701,10 @@
if (Options::verboseVisitRace())
dataLog(toCString("GC visit race: ", race, "\n"));
- if (!ASSERT_DISABLED) {
- auto locker = holdLock(heap()->m_visitRaceLock);
- heap()->m_visitRaces.add(race);
- }
+ auto locker = holdLock(heap()->m_raceMarkStackLock);
+ JSCell* cell = race.cell();
+ cell->setCellState(CellState::PossiblyGrey);
+ heap()->m_raceMarkStack->append(cell);
}
void SlotVisitor::dump(PrintStream& out) const
Modified: branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitor.h (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitor.h 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitor.h 2017-01-13 19:18:25 UTC (rev 210738)
@@ -151,8 +151,6 @@
void didRace(const VisitRaceKey&);
void didRace(JSCell* cell, const char* reason) { didRace(VisitRaceKey(cell, reason)); }
- void didNotRace(const VisitRaceKey&);
- void didNotRace(JSCell* cell, const char* reason) { didNotRace(VisitRaceKey(cell, reason)); }
void visitAsConstraint(const JSCell*);
Modified: branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitorInlines.h (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitorInlines.h 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/heap/SlotVisitorInlines.h 2017-01-13 19:18:25 UTC (rev 210738)
@@ -108,18 +108,4 @@
return *m_heap.m_vm;
}
-inline void SlotVisitor::didNotRace(const VisitRaceKey& race)
-{
- if (ASSERT_DISABLED)
- return;
-
- if (!m_isVisitingMutatorStack) {
- // This is the first visit so we don't need to remove anything.
- return;
- }
-
- auto locker = holdLock(heap()->m_visitRaceLock);
- heap()->m_visitRaces.remove(race);
-}
-
} // namespace JSC
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/JSObject.cpp (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/JSObject.cpp 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/JSObject.cpp 2017-01-13 19:18:25 UTC (rev 210738)
@@ -120,9 +120,7 @@
{
static const char* raceReason = "JSObject::visitButterfly";
Structure* result = visitButterflyImpl(visitor);
- if (result)
- visitor.didNotRace(this, raceReason);
- else
+ if (!result)
visitor.didRace(this, raceReason);
return result;
}
@@ -188,7 +186,7 @@
//
// BEFORE: Scan the object with the structure and butterfly *before* the mutator's transition.
// AFTER: Scan the object with the structure and butterfly *after* the mutator's transition.
- // IGNORE: Give up, so long as the write barrier on PutNewStructure executes after ReadStructureEarly.
+ // IGNORE: Ignore the butterfly and call didRace to schedule us to be revisted again in the future.
//
// In other words, the collector will never see any torn structure/butterfly mix. It will
// always see the structure/butterfly before the transition or after but not in between.
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/JSObjectInlines.h (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/JSObjectInlines.h 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/JSObjectInlines.h 2017-01-13 19:18:25 UTC (rev 210738)
@@ -183,6 +183,7 @@
Butterfly* butterfly = allocateMoreOutOfLineStorage(vm, oldOutOfLineCapacity, newOutOfLineCapacity);
nukeStructureAndSetButterfly(vm, structureID, butterfly);
structure->setLastOffset(newLastOffset);
+ WTF::storeStoreFence();
setStructureIDDirectly(structureID);
} else
structure->setLastOffset(newLastOffset);
Modified: branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.cpp (210737 => 210738)
--- branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.cpp 2017-01-13 19:16:34 UTC (rev 210737)
+++ branches/safari-603-branch/Source/_javascript_Core/runtime/Structure.cpp 2017-01-13 19:18:25 UTC (rev 210738)
@@ -793,10 +793,14 @@
object->shiftButterflyAfterFlattening(locker, vm, this, afterOutOfLineCapacity);
}
- vm.heap.writeBarrier(object);
WTF::storeStoreFence();
object->setStructureIDDirectly(id());
-
+
+ // FIXME: This is probably no longer needed since we have a stronger mechanism
+ // for detecting races and rescanning an object.
+ // https://bugs.webkit.org/show_bug.cgi?id=166989
+ vm.heap.writeBarrier(object);
+
return this;
}
