- Revision
- 207263
- Author
- fpi...@apple.com
- Date
- 2016-10-12 16:56:34 -0700 (Wed, 12 Oct 2016)
Log Message
The blackening of CellState is a bad way of tracking if the object is being marked for the first time
https://bugs.webkit.org/show_bug.cgi?id=163343
Reviewed by Mark Lam.
When I first added the concept of NewGrey/OldGrey, I had the SlotVisitor store the old cell
state in itself, so that it could use it to decide what to do for reportExtraMemoryVisited().
Then I changed it in a recent commit, because I wanted the freedom to have SlotVisitor visit
multiple objects in tandem. But I never ended up using this capability. Still, I liked the
new way better: instead of the SlotVisitor rembemering the state-before-blackening, we would
make the object's state reflect whether it was black for the first time or not. That seemed
convenient.
Unfortunately it's wrong. After we blacken the object, a concurrent barrier could instantly
grey it. Then we would forget that we are visiting this object for the first time.
Subsequent visits will think that they are not the first. So, we will fail to do the right
thing in reportExtraMemoryVisited().
So, this reverts that change. This is a little more than just a revert, though. I've changed
the terminology a bit. For example, I got tired of reading Black and having to remind myself
that it really means that the object has begun being visited, instead of the more strict
meaning that implies that it has already been visited. We want to say that it's Black or
currently being scanned. I'm going to adopt Siebert's term for this: Anthracite [1]. So, our
black CellState is now called AnthraciteOrBlack.
[1] https://pdfs.semanticscholar.org/7ae4/633265aead1f8835cf7966e179d02c2c8a4b.pdf
* heap/CellState.h:
(JSC::isBlack): Deleted.
(JSC::blacken): Deleted.
* heap/Heap.cpp:
(JSC::Heap::addToRememberedSet):
(JSC::Heap::writeBarrierSlowPath):
* heap/Heap.h:
* heap/HeapInlines.h:
(JSC::Heap::reportExtraMemoryVisited):
(JSC::Heap::reportExternalMemoryVisited):
* heap/SlotVisitor.cpp:
(JSC::SlotVisitor::appendToMarkStack):
(JSC::SlotVisitor::visitChildren):
* heap/SlotVisitor.h:
* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::reportExtraMemoryVisited):
(JSC::SlotVisitor::reportExternalMemoryVisited):
* llint/LLIntData.cpp:
(JSC::LLInt::Data::performAssertions):
* llint/LowLevelInterpreter.asm:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (207262 => 207263)
--- trunk/Source/_javascript_Core/ChangeLog 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-10-12 23:56:34 UTC (rev 207263)
@@ -1,3 +1,54 @@
+2016-10-12 Filip Pizlo <fpi...@apple.com>
+
+ The blackening of CellState is a bad way of tracking if the object is being marked for the first time
+ https://bugs.webkit.org/show_bug.cgi?id=163343
+
+ Reviewed by Mark Lam.
+
+ When I first added the concept of NewGrey/OldGrey, I had the SlotVisitor store the old cell
+ state in itself, so that it could use it to decide what to do for reportExtraMemoryVisited().
+
+ Then I changed it in a recent commit, because I wanted the freedom to have SlotVisitor visit
+ multiple objects in tandem. But I never ended up using this capability. Still, I liked the
+ new way better: instead of the SlotVisitor rembemering the state-before-blackening, we would
+ make the object's state reflect whether it was black for the first time or not. That seemed
+ convenient.
+
+ Unfortunately it's wrong. After we blacken the object, a concurrent barrier could instantly
+ grey it. Then we would forget that we are visiting this object for the first time.
+ Subsequent visits will think that they are not the first. So, we will fail to do the right
+ thing in reportExtraMemoryVisited().
+
+ So, this reverts that change. This is a little more than just a revert, though. I've changed
+ the terminology a bit. For example, I got tired of reading Black and having to remind myself
+ that it really means that the object has begun being visited, instead of the more strict
+ meaning that implies that it has already been visited. We want to say that it's Black or
+ currently being scanned. I'm going to adopt Siebert's term for this: Anthracite [1]. So, our
+ black CellState is now called AnthraciteOrBlack.
+
+ [1] https://pdfs.semanticscholar.org/7ae4/633265aead1f8835cf7966e179d02c2c8a4b.pdf
+
+ * heap/CellState.h:
+ (JSC::isBlack): Deleted.
+ (JSC::blacken): Deleted.
+ * heap/Heap.cpp:
+ (JSC::Heap::addToRememberedSet):
+ (JSC::Heap::writeBarrierSlowPath):
+ * heap/Heap.h:
+ * heap/HeapInlines.h:
+ (JSC::Heap::reportExtraMemoryVisited):
+ (JSC::Heap::reportExternalMemoryVisited):
+ * heap/SlotVisitor.cpp:
+ (JSC::SlotVisitor::appendToMarkStack):
+ (JSC::SlotVisitor::visitChildren):
+ * heap/SlotVisitor.h:
+ * heap/SlotVisitorInlines.h:
+ (JSC::SlotVisitor::reportExtraMemoryVisited):
+ (JSC::SlotVisitor::reportExternalMemoryVisited):
+ * llint/LLIntData.cpp:
+ (JSC::LLInt::Data::performAssertions):
+ * llint/LowLevelInterpreter.asm:
+
2016-10-12 Mark Lam <mark....@apple.com>
Rename variables in arrayProtoFuncSplice() to match names in the spec.
Modified: trunk/Source/_javascript_Core/heap/CellState.h (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/CellState.h 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/CellState.h 2016-10-12 23:56:34 UTC (rev 207263)
@@ -30,28 +30,27 @@
namespace JSC {
enum class CellState : uint8_t {
- // The object is black for the first time during this GC.
- NewBlack = 0,
+ // The object is either currently being scanned (anthracite) or it has finished being scanned
+ // (black). It could be scanned for the first time this GC, or the Nth time - if it's anthracite
+ // then the SlotVisitor knows. We explicitly say "anthracite or black" to emphasize the fact that
+ // this is no guarantee that we have finished scanning the object, unless you also know that all
+ // SlotVisitors are done.
+ AnthraciteOrBlack = 0,
- // The object is black for the Nth time during this full GC cycle (N > 1). An object may get to
- // this state if it transitions from black back to grey during a concurrent GC, or because it
- // wound up in the remembered set because of a generational barrier.
- OldBlack = 1,
-
// The object is in eden. During GC, this means that the object has not been marked yet.
- NewWhite = 2,
+ NewWhite = 1,
// The object is grey - i.e. it will be scanned - and this is the first time in this GC that we are
// going to scan it. If this is an eden GC, this also means that the object is in eden.
- NewGrey = 3,
+ NewGrey = 2,
// The object is grey - i.e. it will be scanned - but it either belongs to old gen (if this is eden
// GC) or it is grey a second time in this current GC (because a concurrent store barrier requested
// re-greying).
- OldGrey = 4
+ OldGrey = 3
};
-static const unsigned blackThreshold = 1; // x <= blackThreshold means x is black.
+static const unsigned blackThreshold = 0; // x <= blackThreshold means x is AnthraciteOrBlack.
static const unsigned tautologicalThreshold = 100; // x <= tautologicalThreshold is always true.
inline bool isWithinThreshold(CellState cellState, unsigned threshold)
@@ -59,17 +58,4 @@
return static_cast<unsigned>(cellState) <= threshold;
}
-inline bool isBlack(CellState cellState)
-{
- return isWithinThreshold(cellState, blackThreshold);
-}
-
-inline CellState blacken(CellState cellState)
-{
- if (cellState == CellState::NewGrey)
- return CellState::NewBlack;
- ASSERT(cellState == CellState::NewBlack || cellState == CellState::OldBlack || cellState == CellState::OldGrey);
- return CellState::OldBlack;
-}
-
} // namespace JSC
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2016-10-12 23:56:34 UTC (rev 207263)
@@ -916,7 +916,7 @@
{
ASSERT(cell);
ASSERT(!Options::useConcurrentJIT() || !isCompilationThread());
- ASSERT(isBlack(cell->cellState()));
+ ASSERT(cell->cellState() == CellState::AnthraciteOrBlack);
// Indicate that this object is grey and that it's one of the following:
// - A re-greyed object during a concurrent collection.
// - An old remembered object.
@@ -1552,7 +1552,7 @@
// In this case, the barrierThreshold is the tautological threshold, so from could still be
// not black. But we can't know for sure until we fire off a fence.
WTF::storeLoadFence();
- if (!isBlack(from->cellState()))
+ if (from->cellState() != CellState::AnthraciteOrBlack)
return;
}
Modified: trunk/Source/_javascript_Core/heap/Heap.h (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/Heap.h 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2016-10-12 23:56:34 UTC (rev 207263)
@@ -181,11 +181,11 @@
// call both of these functions: Calling only one may trigger catastropic
// memory growth.
void reportExtraMemoryAllocated(size_t);
- void reportExtraMemoryVisited(JSCell*, size_t);
+ void reportExtraMemoryVisited(CellState oldState, size_t);
#if ENABLE(RESOURCE_USAGE)
// Use this API to report the subset of extra memory that lives outside this process.
- void reportExternalMemoryVisited(JSCell*, size_t);
+ void reportExternalMemoryVisited(CellState oldState, size_t);
size_t externalMemorySize() { return m_externalMemorySize; }
#endif
Modified: trunk/Source/_javascript_Core/heap/HeapInlines.h (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/HeapInlines.h 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/HeapInlines.h 2016-10-12 23:56:34 UTC (rev 207263)
@@ -158,10 +158,10 @@
reportExtraMemoryAllocatedSlowCase(size);
}
-inline void Heap::reportExtraMemoryVisited(JSCell* cell, size_t size)
+inline void Heap::reportExtraMemoryVisited(CellState oldState, size_t size)
{
// We don't want to double-count the extra memory that was reported in previous collections.
- if (operationInProgress() == EdenCollection && cell->cellState() == CellState::OldBlack)
+ if (operationInProgress() == EdenCollection && oldState == CellState::OldGrey)
return;
size_t* counter = &m_extraMemorySize;
@@ -174,10 +174,10 @@
}
#if ENABLE(RESOURCE_USAGE)
-inline void Heap::reportExternalMemoryVisited(JSCell* cell, size_t size)
+inline void Heap::reportExternalMemoryVisited(CellState oldState, size_t size)
{
// We don't want to double-count the external memory that was reported in previous collections.
- if (operationInProgress() == EdenCollection && cell->cellState() == CellState::OldBlack)
+ if (operationInProgress() == EdenCollection && oldState == CellState::OldGrey)
return;
size_t* counter = &m_externalMemorySize;
Modified: trunk/Source/_javascript_Core/heap/SlotVisitor.cpp (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/SlotVisitor.cpp 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/SlotVisitor.cpp 2016-10-12 23:56:34 UTC (rev 207263)
@@ -228,6 +228,7 @@
{
ASSERT(Heap::isMarkedConcurrently(cell));
ASSERT(!cell->isZapped());
+ ASSERT(cell->cellState() == CellState::NewGrey || cell->cellState() == CellState::OldGrey);
container.noteMarked();
@@ -293,8 +294,15 @@
SetCurrentCellScope currentCellScope(*this, cell);
- cell->setCellState(blacken(cell->cellState()));
+ m_oldCellState = cell->cellState();
+ // There is no race here - the cell state cannot change right now. Grey objects can only be
+ // visited by one marking thread. Neither the barrier nor marking will change the state of an
+ // object that is already grey.
+ ASSERT(m_oldCellState == CellState::OldGrey || m_oldCellState == CellState::NewGrey);
+
+ cell->setCellState(CellState::AnthraciteOrBlack);
+
WTF::storeLoadFence();
switch (cell->type()) {
@@ -318,7 +326,7 @@
}
if (UNLIKELY(m_heapSnapshotBuilder)) {
- if (cell->cellState() != CellState::OldBlack)
+ if (m_oldCellState == CellState::NewGrey)
m_heapSnapshotBuilder->appendNode(const_cast<JSCell*>(cell));
}
}
Modified: trunk/Source/_javascript_Core/heap/SlotVisitor.h (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/SlotVisitor.h 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/SlotVisitor.h 2016-10-12 23:56:34 UTC (rev 207263)
@@ -165,6 +165,7 @@
HeapSnapshotBuilder* m_heapSnapshotBuilder { nullptr };
JSCell* m_currentCell { nullptr };
+ CellState m_oldCellState;
public:
#if !ASSERT_DISABLED
Modified: trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h (207262 => 207263)
--- trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2016-10-12 23:56:34 UTC (rev 207263)
@@ -105,13 +105,13 @@
inline void SlotVisitor::reportExtraMemoryVisited(size_t size)
{
- heap()->reportExtraMemoryVisited(m_currentCell, size);
+ heap()->reportExtraMemoryVisited(m_oldCellState, size);
}
#if ENABLE(RESOURCE_USAGE)
inline void SlotVisitor::reportExternalMemoryVisited(size_t size)
{
- heap()->reportExternalMemoryVisited(m_currentCell, size);
+ heap()->reportExternalMemoryVisited(m_oldCellState, size);
}
#endif
Modified: trunk/Source/_javascript_Core/llint/LLIntData.cpp (207262 => 207263)
--- trunk/Source/_javascript_Core/llint/LLIntData.cpp 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/llint/LLIntData.cpp 2016-10-12 23:56:34 UTC (rev 207263)
@@ -214,7 +214,7 @@
STATIC_ASSERT(GetPutInfo::initializationBits == 0xffc00);
STATIC_ASSERT(MarkedBlock::blockSize == 16 * 1024);
- STATIC_ASSERT(blackThreshold == 1);
+ STATIC_ASSERT(blackThreshold == 0);
ASSERT(bitwise_cast<uintptr_t>(ShadowChicken::Packet::tailMarker()) == static_cast<uintptr_t>(0x7a11));
Modified: trunk/Source/_javascript_Core/llint/LowLevelInterpreter.asm (207262 => 207263)
--- trunk/Source/_javascript_Core/llint/LowLevelInterpreter.asm 2016-10-12 23:17:52 UTC (rev 207262)
+++ trunk/Source/_javascript_Core/llint/LowLevelInterpreter.asm 2016-10-12 23:56:34 UTC (rev 207263)
@@ -409,7 +409,7 @@
const MarkedBlockSize = 16 * 1024
const MarkedBlockMask = ~(MarkedBlockSize - 1)
-const BlackThreshold = 1
+const BlackThreshold = 0
# Allocation constants
if JSVALUE64
