- Revision
- 210451
- Author
- [email protected]
- Date
- 2017-01-06 12:23:25 -0800 (Fri, 06 Jan 2017)
Log Message
DeferGC::~DeferGC should be super cheap
https://bugs.webkit.org/show_bug.cgi?id=166626
Reviewed by Saam Barati.
JSTests:
* stress/slow-path-generator-updating-current-node-dfg.js:
* stress/unshift-array-storage.js:
Source/_javascript_Core:
Right now, ~DeferGC requires running the collector's full collectIfNecessaryOrDefer()
hook, which is super big. Normally, that hook would only be called from GC slow paths,
so it ought to be possible to add complex logic to it. It benefits the GC algorithm to
make that code smart, not necessarily fast.
The right thing for it to do is to have ~DeferGC check a boolean to see if
collectIfNecessaryOrDefer() had previously deferred anything, and only call it if that
is true. That's what this patch does.
Unfortunately, this means that we lose the collectAccordingToDeferGCProbability mode,
which we used for two tests. Since I could only see two tests that used this mode, I
felt that it was better to enhance the GC than to keep the tests. I filed bug 166627 to
bring back something like that mode.
Although this patch does make some paths faster, its real goal is to ensure that bug
165963 can add more logic to collectIfNecessaryOrDefer() without introducing a big
regression. Until then, I wouldn't be surprised if this patch was a progression, but I'm
not betting on it.
* heap/Heap.cpp:
(JSC::Heap::collectIfNecessaryOrDefer):
(JSC::Heap::decrementDeferralDepthAndGCIfNeededSlow):
(JSC::Heap::canCollect): Deleted.
(JSC::Heap::shouldCollectHeuristic): Deleted.
(JSC::Heap::shouldCollect): Deleted.
(JSC::Heap::collectAccordingToDeferGCProbability): Deleted.
(JSC::Heap::decrementDeferralDepthAndGCIfNeeded): Deleted.
* heap/Heap.h:
* heap/HeapInlines.h:
(JSC::Heap::incrementDeferralDepth):
(JSC::Heap::decrementDeferralDepth):
(JSC::Heap::decrementDeferralDepthAndGCIfNeeded):
(JSC::Heap::mayNeedToStop):
(JSC::Heap::stopIfNecessary):
* runtime/Options.h:
Modified Paths
Diff
Modified: trunk/JSTests/ChangeLog (210450 => 210451)
--- trunk/JSTests/ChangeLog 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/JSTests/ChangeLog 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,3 +1,13 @@
+2016-12-30 Filip Pizlo <[email protected]>
+
+ DeferGC::~DeferGC should be super cheap
+ https://bugs.webkit.org/show_bug.cgi?id=166626
+
+ Reviewed by Saam Barati.
+
+ * stress/slow-path-generator-updating-current-node-dfg.js:
+ * stress/unshift-array-storage.js:
+
2017-01-04 JF Bastien <[email protected]>
WebAssembly: sections with the same name are allowed
Modified: trunk/JSTests/stress/slow-path-generator-updating-current-node-dfg.js (210450 => 210451)
--- trunk/JSTests/stress/slow-path-generator-updating-current-node-dfg.js 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/JSTests/stress/slow-path-generator-updating-current-node-dfg.js 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,4 +1,7 @@
-//@ runFTLNoCJIT("--deferGCShouldCollectWithProbability=true", "--deferGCProbability=1.0")
+// FIXME: Bring back something like the deferGC probability mode.
+// https://bugs.webkit.org/show_bug.cgi?id=166627
+//@ skip
+// //@ runFTLNoCJIT("--deferGCShouldCollectWithProbability=true", "--deferGCProbability=1.0")
function foo(a) {
return a.push(25);
Modified: trunk/JSTests/stress/unshift-array-storage.js (210450 => 210451)
--- trunk/JSTests/stress/unshift-array-storage.js 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/JSTests/stress/unshift-array-storage.js 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,4 +1,7 @@
-//@ runFTLNoCJIT("--scribbleFreeCells=true", "--deferGCShouldCollectWithProbability=true", "--deferGCProbability=1")
+// FIXME: Bring back something like the deferGC probability mode.
+// https://bugs.webkit.org/show_bug.cgi?id=166627
+//@ skip
+// //@ runFTLNoCJIT("--scribbleFreeCells=true", "--deferGCShouldCollectWithProbability=true", "--deferGCProbability=1")
// Create some array storage.
var array = [];
Modified: trunk/Source/_javascript_Core/ChangeLog (210450 => 210451)
--- trunk/Source/_javascript_Core/ChangeLog 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,3 +1,46 @@
+2016-12-30 Filip Pizlo <[email protected]>
+
+ DeferGC::~DeferGC should be super cheap
+ https://bugs.webkit.org/show_bug.cgi?id=166626
+
+ Reviewed by Saam Barati.
+
+ Right now, ~DeferGC requires running the collector's full collectIfNecessaryOrDefer()
+ hook, which is super big. Normally, that hook would only be called from GC slow paths,
+ so it ought to be possible to add complex logic to it. It benefits the GC algorithm to
+ make that code smart, not necessarily fast.
+
+ The right thing for it to do is to have ~DeferGC check a boolean to see if
+ collectIfNecessaryOrDefer() had previously deferred anything, and only call it if that
+ is true. That's what this patch does.
+
+ Unfortunately, this means that we lose the collectAccordingToDeferGCProbability mode,
+ which we used for two tests. Since I could only see two tests that used this mode, I
+ felt that it was better to enhance the GC than to keep the tests. I filed bug 166627 to
+ bring back something like that mode.
+
+ Although this patch does make some paths faster, its real goal is to ensure that bug
+ 165963 can add more logic to collectIfNecessaryOrDefer() without introducing a big
+ regression. Until then, I wouldn't be surprised if this patch was a progression, but I'm
+ not betting on it.
+
+ * heap/Heap.cpp:
+ (JSC::Heap::collectIfNecessaryOrDefer):
+ (JSC::Heap::decrementDeferralDepthAndGCIfNeededSlow):
+ (JSC::Heap::canCollect): Deleted.
+ (JSC::Heap::shouldCollectHeuristic): Deleted.
+ (JSC::Heap::shouldCollect): Deleted.
+ (JSC::Heap::collectAccordingToDeferGCProbability): Deleted.
+ (JSC::Heap::decrementDeferralDepthAndGCIfNeeded): Deleted.
+ * heap/Heap.h:
+ * heap/HeapInlines.h:
+ (JSC::Heap::incrementDeferralDepth):
+ (JSC::Heap::decrementDeferralDepth):
+ (JSC::Heap::decrementDeferralDepthAndGCIfNeeded):
+ (JSC::Heap::mayNeedToStop):
+ (JSC::Heap::stopIfNecessary):
+ * runtime/Options.h:
+
2017-01-05 Filip Pizlo <[email protected]>
AutomaticThread timeout shutdown leaves a small window where notify() would think that the thread is still running
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (210450 => 210451)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2003-2009, 2011, 2013-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
* Copyright (C) 2007 Eric Seidel <[email protected]>
*
* This library is free software; you can redistribute it and/or
@@ -2023,29 +2023,6 @@
addToRememberedSet(from);
}
-bool Heap::canCollect()
-{
- if (isDeferred())
- return false;
- if (!m_isSafeToCollect)
- return false;
- if (mutatorState() == MutatorState::HelpingGC)
- return false;
- return true;
-}
-
-bool Heap::shouldCollectHeuristic()
-{
- if (Options::gcMaxHeapSize())
- return m_bytesAllocatedThisCycle > Options::gcMaxHeapSize();
- return m_bytesAllocatedThisCycle > m_maxEdenSize;
-}
-
-bool Heap::shouldCollect()
-{
- return canCollect() && shouldCollectHeuristic();
-}
-
bool Heap::isCurrentThreadBusy()
{
return mayBeGCThread() || mutatorState() != MutatorState::Running;
@@ -2077,49 +2054,55 @@
bool Heap::collectIfNecessaryOrDefer(GCDeferralContext* deferralContext)
{
- if (!canCollect())
+ ASSERT(!DisallowGC::isGCDisallowedOnCurrentThread());
+
+ if (!m_isSafeToCollect)
return false;
+ if (mutatorState() == MutatorState::HelpingGC)
+ return false;
+ if (!Options::useGC())
+ return false;
- if (deferralContext) {
- deferralContext->m_shouldGC |=
- !!(m_worldState.load() & (shouldStopBit | needFinalizeBit | gcDidJITBit));
- } else
- stopIfNecessary();
+ if (mayNeedToStop()) {
+ if (deferralContext)
+ deferralContext->m_shouldGC = true;
+ else if (isDeferred())
+ m_didDeferGCWork = true;
+ else
+ stopIfNecessary();
+ }
- if (!shouldCollectHeuristic())
- return false;
+ if (UNLIKELY(Options::gcMaxHeapSize())) {
+ if (m_bytesAllocatedThisCycle <= Options::gcMaxHeapSize())
+ return false;
+ } else {
+ if (m_bytesAllocatedThisCycle <= m_maxEdenSize)
+ return false;
+ }
if (deferralContext)
deferralContext->m_shouldGC = true;
+ else if (isDeferred())
+ m_didDeferGCWork = true;
else
collectAsync();
return true;
}
-void Heap::collectAccordingToDeferGCProbability()
+void Heap::decrementDeferralDepthAndGCIfNeededSlow()
{
- if (isDeferred() || !m_isSafeToCollect || collectionScope() || mutatorState() == MutatorState::HelpingGC)
+ // Can't do anything if we're still deferred.
+ if (m_deferralDepth)
return;
-
- if (randomNumber() < Options::deferGCProbability()) {
- collectAsync();
- return;
- }
-
- // If our coin flip told us not to GC, we still might GC,
- // but we GC according to our memory pressure markers.
+
+ ASSERT(!isDeferred());
+
+ m_didDeferGCWork = false;
+ // FIXME: Bring back something like the DeferGCProbability mode.
+ // https://bugs.webkit.org/show_bug.cgi?id=166627
collectIfNecessaryOrDefer();
}
-void Heap::decrementDeferralDepthAndGCIfNeeded()
-{
- decrementDeferralDepth();
- if (UNLIKELY(Options::deferGCShouldCollectWithProbability()))
- collectAccordingToDeferGCProbability();
- else
- collectIfNecessaryOrDefer();
-}
-
void Heap::registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback)
{
m_weakGCMaps.add(weakGCMap, WTFMove(pruningCallback));
Modified: trunk/Source/_javascript_Core/heap/Heap.h (210450 => 210451)
--- trunk/Source/_javascript_Core/heap/Heap.h 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,7 +1,7 @@
/*
* Copyright (C) 1999-2000 Harri Porten ([email protected])
* Copyright (C) 2001 Peter Kelly ([email protected])
- * Copyright (C) 2003-2009, 2013-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -185,9 +185,7 @@
JS_EXPORT_PRIVATE void collectAllGarbageIfNotDoneRecently();
JS_EXPORT_PRIVATE void collectAllGarbage();
- bool canCollect();
bool shouldCollectHeuristic();
- bool shouldCollect();
// Queue up a collection. Returns immediately. This will not queue a collection if a collection
// of equal or greater strength exists. Full collections are stronger than std::nullopt collections
@@ -204,7 +202,6 @@
JS_EXPORT_PRIVATE void collectSync(std::optional<CollectionScope> = std::nullopt);
bool collectIfNecessaryOrDefer(GCDeferralContext* = nullptr); // Returns true if it did collect.
- void collectAccordingToDeferGCProbability();
void completeAllJITPlans();
@@ -268,7 +265,7 @@
void addReference(JSCell*, ArrayBuffer*);
- bool isDeferred() const { return !!m_deferralDepth || !Options::useGC(); }
+ bool isDeferred() const { return !!m_deferralDepth; }
StructureIDTable& structureIDTable() { return m_structureIDTable; }
@@ -341,6 +338,8 @@
// already be called for you at the right times.
void stopIfNecessary();
+ bool mayNeedToStop();
+
// This is a much stronger kind of stopping of the collector, and it may require waiting for a
// while. This is meant to be a legacy API for clients of collectAllGarbage that expect that there
// is no GC before or after that function call. After calling this, you are free to start GCs
@@ -488,7 +487,8 @@
void incrementDeferralDepth();
void decrementDeferralDepth();
- JS_EXPORT_PRIVATE void decrementDeferralDepthAndGCIfNeeded();
+ void decrementDeferralDepthAndGCIfNeeded();
+ JS_EXPORT_PRIVATE void decrementDeferralDepthAndGCIfNeededSlow();
size_t threadVisitCount();
size_t threadBytesVisited();
@@ -579,6 +579,7 @@
Vector<HeapObserver*> m_observers;
unsigned m_deferralDepth;
+ bool m_didDeferGCWork { false };
std::unique_ptr<HeapVerifier> m_verifier;
Modified: trunk/Source/_javascript_Core/heap/HeapInlines.h (210450 => 210451)
--- trunk/Source/_javascript_Core/heap/HeapInlines.h 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/heap/HeapInlines.h 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2014-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -315,16 +315,47 @@
inline void Heap::incrementDeferralDepth()
{
- RELEASE_ASSERT(m_deferralDepth < 100); // Sanity check to make sure this doesn't get ridiculous.
+ ASSERT(!mayBeGCThread() || m_collectorBelievesThatTheWorldIsStopped);
m_deferralDepth++;
}
inline void Heap::decrementDeferralDepth()
{
- RELEASE_ASSERT(m_deferralDepth >= 1);
+ ASSERT(!mayBeGCThread() || m_collectorBelievesThatTheWorldIsStopped);
m_deferralDepth--;
}
+inline void Heap::decrementDeferralDepthAndGCIfNeeded()
+{
+ ASSERT(!mayBeGCThread() || m_collectorBelievesThatTheWorldIsStopped);
+ m_deferralDepth--;
+
+ if (UNLIKELY(m_didDeferGCWork)) {
+ decrementDeferralDepthAndGCIfNeededSlow();
+
+ // Here are the possible relationships between m_deferralDepth and m_didDeferGCWork.
+ // Note that prior to the call to decrementDeferralDepthAndGCIfNeededSlow,
+ // m_didDeferGCWork had to have been true. Now it can be either false or true. There is
+ // nothing we can reliably assert.
+ //
+ // Possible arrangements of m_didDeferGCWork and !!m_deferralDepth:
+ //
+ // Both false: We popped out of all DeferGCs and we did whatever work was deferred.
+ //
+ // Only m_didDeferGCWork is true: We stopped for GC and the GC did DeferGC. This is
+ // possible because of how we handle the baseline JIT's worklist. It's also perfectly
+ // safe because it only protects reportExtraMemory. We can just ignore this.
+ //
+ // Only !!m_deferralDepth is true: m_didDeferGCWork had been set spuriously. It is only
+ // cleared by decrementDeferralDepthAndGCIfNeededSlow(). So, if we had deferred work but
+ // then decrementDeferralDepth()'d, then we might have the bit set even if we GC'd since
+ // then.
+ //
+ // Both true: We're in a recursive ~DeferGC. We wanted to do something about the
+ // deferred work, but were unable to.
+ }
+}
+
inline HashSet<MarkedArgumentBuffer*>& Heap::markListSet()
{
if (!m_markListSet)
@@ -363,11 +394,15 @@
releaseAccessSlow();
}
+inline bool Heap::mayNeedToStop()
+{
+ return m_worldState.loadRelaxed() != hasAccessBit;
+}
+
inline void Heap::stopIfNecessary()
{
- if (m_worldState.loadRelaxed() == hasAccessBit)
- return;
- stopIfNecessarySlow();
+ if (mayNeedToStop())
+ stopIfNecessarySlow();
}
inline void Heap::writeBarrierOpaqueRoot(void* root)
Modified: trunk/Source/_javascript_Core/runtime/Options.cpp (210450 => 210451)
--- trunk/Source/_javascript_Core/runtime/Options.cpp 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/runtime/Options.cpp 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2011-2012, 2014-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -418,7 +418,6 @@
ASSERT((static_cast<int64_t>(Options::thresholdForOptimizeAfterLongWarmUp()) << Options::reoptimizationRetryCounterMax()) > 0);
ASSERT((static_cast<int64_t>(Options::thresholdForOptimizeAfterLongWarmUp()) << Options::reoptimizationRetryCounterMax()) <= static_cast<int64_t>(std::numeric_limits<int32_t>::max()));
- ASSERT(Options::deferGCProbability() >= 0.0 && Options::deferGCProbability() <= 1.0);
#if ENABLE(LLINT_STATS)
LLInt::Data::loadStats();
Modified: trunk/Source/_javascript_Core/runtime/Options.h (210450 => 210451)
--- trunk/Source/_javascript_Core/runtime/Options.h 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/runtime/Options.h 2017-01-06 20:23:25 UTC (rev 210451)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2011-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -330,8 +330,6 @@
v(double, minHeapUtilization, 0.8, Normal, nullptr) \
v(double, minMarkedBlockUtilization, 0.9, Normal, nullptr) \
v(unsigned, slowPathAllocsBetweenGCs, 0, Normal, "force a GC on every Nth slow path alloc, where N is specified by this option") \
- v(bool, deferGCShouldCollectWithProbability, false, Normal, "If true, we perform a collection based on flipping a coin according the probability in the 'deferGCProbability' option when DeferGC is destructed.") \
- v(double, deferGCProbability, 1.0, Normal, "Should be a number between 0 and 1. 1 means DeferGC always GCs when it's destructed and GCing is safe. 0.7 means we force GC 70% the time on DeferGC destruction.") \
\
v(double, percentCPUPerMBForFullTimer, 0.0003125, Normal, nullptr) \
v(double, percentCPUPerMBForEdenTimer, 0.0025, Normal, nullptr) \
Modified: trunk/Source/_javascript_Core/runtime/ScriptExecutable.cpp (210450 => 210451)
--- trunk/Source/_javascript_Core/runtime/ScriptExecutable.cpp 2017-01-06 20:17:48 UTC (rev 210450)
+++ trunk/Source/_javascript_Core/runtime/ScriptExecutable.cpp 2017-01-06 20:23:25 UTC (rev 210451)
@@ -77,8 +77,6 @@
void ScriptExecutable::installCode(VM& vm, CodeBlock* genericCodeBlock, CodeType codeType, CodeSpecializationKind kind)
{
- ASSERT(vm.heap.isDeferred());
-
if (genericCodeBlock)
CODEBLOCK_LOG_EVENT(genericCodeBlock, "installCode", ());
