WTF should make it super easy to do ARM concurrency tricks
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Mar 2017 17:40:10 +0000 (17:40 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Mar 2017 17:40:10 +0000 (17:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169300

Reviewed by Mark Lam.

Source/JavaScriptCore:

This changes a bunch of GC hot paths to use new concurrency APIs that lead to optimal
code on both x86 (fully leverage TSO, transactions become CAS loops) and ARM (use
dependency chains for fencing, transactions become LL/SC loops). While inspecting the
machine code, I found other opportunities for improvement, like inlining the "am I
marked" part of the marking functions.

* heap/Heap.cpp:
(JSC::Heap::setGCDidJIT):
* heap/HeapInlines.h:
(JSC::Heap::testAndSetMarked):
* heap/LargeAllocation.h:
(JSC::LargeAllocation::isMarked):
(JSC::LargeAllocation::isMarkedConcurrently):
(JSC::LargeAllocation::aboutToMark):
(JSC::LargeAllocation::testAndSetMarked):
* heap/MarkedBlock.h:
(JSC::MarkedBlock::areMarksStaleWithDependency):
(JSC::MarkedBlock::aboutToMark):
(JSC::MarkedBlock::isMarkedConcurrently):
(JSC::MarkedBlock::isMarked):
(JSC::MarkedBlock::testAndSetMarked):
* heap/SlotVisitor.cpp:
(JSC::SlotVisitor::appendSlow):
(JSC::SlotVisitor::appendHiddenSlow):
(JSC::SlotVisitor::appendHiddenSlowImpl):
(JSC::SlotVisitor::setMarkedAndAppendToMarkStack):
(JSC::SlotVisitor::appendUnbarriered): Deleted.
(JSC::SlotVisitor::appendHidden): Deleted.
* heap/SlotVisitor.h:
* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::appendUnbarriered):
(JSC::SlotVisitor::appendHidden):
(JSC::SlotVisitor::append):
(JSC::SlotVisitor::appendValues):
(JSC::SlotVisitor::appendValuesHidden):
* runtime/CustomGetterSetter.cpp:
* runtime/JSObject.cpp:
(JSC::JSObject::visitButterflyImpl):
* runtime/JSObject.h:

Source/WTF:

This adds Atomic<>::loadLink and Atomic<>::storeCond, available only when HAVE(LL_SC).

It abstracts loadLink/storeCond behind prepare/attempt. You can write prepare/attempt
loops whenever your loop fits into the least common denominator of LL/SC and CAS.

This modifies Atomic<>::transaction to use prepare/attempt. So, if you write your loop
using Atomic<>::transaction, then you get LL/SC for free.

Depending on the kind of transaction you are doing, you may not want to perform an LL
until you have a chance to just load the current value. Atomic<>::transaction() assumes
that you do not care to have any ordering guarantees in that case. If you think that
the transaction has a good chance of aborting this way, you want
Atomic<>::transaction() to first do a plain load. But if you don't think that such an
abort is likely, then you want to go straight to the LL. The API supports this concept
via TransactionAbortLikelihood.

Additionally, this redoes the depend/consume API to be dead simple. Dependency is
unsigned. You get a dependency on a loaded value by just saying
dependency(loadedValue). You consume the dependency by using it as a bonus index to
some pointer dereference. This is made easy with the consume<T*>(ptr, dependency)
helper. In those cases where you want to pass around both a computed value and a
dependency, there's DependencyWith<T>. But you won't need it in most cases. The loaded
value or any value computed from the loaded value is a fine input to dependency()!

This change updates a bunch of hot paths to use the new APIs. Using transaction() gives
us optimal LL/SC loops for object marking and lock acquisition.

This change also updates a bunch of hot paths to use dependency()/consume().

This is a significant Octane/splay speed-up on ARM.

* wtf/Atomics.h:
(WTF::hasFence):
(WTF::Atomic::prepare):
(WTF::Atomic::attempt):
(WTF::Atomic::transaction):
(WTF::Atomic::transactionRelaxed):
(WTF::nullDependency):
(WTF::dependency):
(WTF::DependencyWith::DependencyWith):
(WTF::dependencyWith):
(WTF::consume):
(WTF::Atomic::tryTransactionRelaxed): Deleted.
(WTF::Atomic::tryTransaction): Deleted.
(WTF::zeroWithConsumeDependency): Deleted.
(WTF::consumeLoad): Deleted.
* wtf/Bitmap.h:
(WTF::WordType>::get):
(WTF::WordType>::concurrentTestAndSet):
(WTF::WordType>::concurrentTestAndClear):
* wtf/LockAlgorithm.h:
(WTF::LockAlgorithm::lockFast):
(WTF::LockAlgorithm::unlockFast):
(WTF::LockAlgorithm::unlockSlow):
* wtf/Platform.h:

Tools:

This vastly simplifies the consume API. The new API is thoroughly tested by being used
in the GC's guts. I think that unit tests are a pain to maintain, so we shouldn't have
them unless we are legitimately worried about coverage. We're not in this case.

* TestWebKitAPI/CMakeLists.txt:
* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WTF/Consume.cpp: Removed.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@213645 268f45cc-cd09-0410-ab3c-d52691b4dbfc

20 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/HeapInlines.h
Source/JavaScriptCore/heap/LargeAllocation.h
Source/JavaScriptCore/heap/MarkedBlock.h
Source/JavaScriptCore/heap/SlotVisitor.cpp
Source/JavaScriptCore/heap/SlotVisitor.h
Source/JavaScriptCore/heap/SlotVisitorInlines.h
Source/JavaScriptCore/runtime/CustomGetterSetter.cpp
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/JSObject.h
Source/WTF/ChangeLog
Source/WTF/wtf/Atomics.h
Source/WTF/wtf/Bitmap.h
Source/WTF/wtf/LockAlgorithm.h
Source/WTF/wtf/Platform.h
Tools/ChangeLog
Tools/TestWebKitAPI/CMakeLists.txt
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WTF/Consume.cpp [deleted file]

index 05f45d9..d9c31cf 100644 (file)
@@ -1,3 +1,50 @@
+2017-03-07  Filip Pizlo  <fpizlo@apple.com>
+
+        WTF should make it super easy to do ARM concurrency tricks
+        https://bugs.webkit.org/show_bug.cgi?id=169300
+
+        Reviewed by Mark Lam.
+        
+        This changes a bunch of GC hot paths to use new concurrency APIs that lead to optimal
+        code on both x86 (fully leverage TSO, transactions become CAS loops) and ARM (use
+        dependency chains for fencing, transactions become LL/SC loops). While inspecting the
+        machine code, I found other opportunities for improvement, like inlining the "am I
+        marked" part of the marking functions.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::setGCDidJIT):
+        * heap/HeapInlines.h:
+        (JSC::Heap::testAndSetMarked):
+        * heap/LargeAllocation.h:
+        (JSC::LargeAllocation::isMarked):
+        (JSC::LargeAllocation::isMarkedConcurrently):
+        (JSC::LargeAllocation::aboutToMark):
+        (JSC::LargeAllocation::testAndSetMarked):
+        * heap/MarkedBlock.h:
+        (JSC::MarkedBlock::areMarksStaleWithDependency):
+        (JSC::MarkedBlock::aboutToMark):
+        (JSC::MarkedBlock::isMarkedConcurrently):
+        (JSC::MarkedBlock::isMarked):
+        (JSC::MarkedBlock::testAndSetMarked):
+        * heap/SlotVisitor.cpp:
+        (JSC::SlotVisitor::appendSlow):
+        (JSC::SlotVisitor::appendHiddenSlow):
+        (JSC::SlotVisitor::appendHiddenSlowImpl):
+        (JSC::SlotVisitor::setMarkedAndAppendToMarkStack):
+        (JSC::SlotVisitor::appendUnbarriered): Deleted.
+        (JSC::SlotVisitor::appendHidden): Deleted.
+        * heap/SlotVisitor.h:
+        * heap/SlotVisitorInlines.h:
+        (JSC::SlotVisitor::appendUnbarriered):
+        (JSC::SlotVisitor::appendHidden):
+        (JSC::SlotVisitor::append):
+        (JSC::SlotVisitor::appendValues):
+        (JSC::SlotVisitor::appendValuesHidden):
+        * runtime/CustomGetterSetter.cpp:
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::visitButterflyImpl):
+        * runtime/JSObject.h:
+
 2017-03-08  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [GTK] JSC test stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler crashing on GTK bot
index 5c24518..88c0475 100644 (file)
@@ -1853,9 +1853,10 @@ void Heap::handleNeedFinalize()
 void Heap::setGCDidJIT()
 {
     m_worldState.transaction(
-        [&] (unsigned& state) {
+        [&] (unsigned& state) -> bool {
             RELEASE_ASSERT(state & stoppedBit);
             state |= gcDidJITBit;
+            return true;
         });
 }
 
index 21de028..8a05e38 100644 (file)
@@ -93,8 +93,8 @@ ALWAYS_INLINE bool Heap::testAndSetMarked(HeapVersion markingVersion, const void
     if (cell->isLargeAllocation())
         return cell->largeAllocation().testAndSetMarked();
     MarkedBlock& block = cell->markedBlock();
-    block.aboutToMark(markingVersion);
-    return block.testAndSetMarked(cell);
+    Dependency dependency = block.aboutToMark(markingVersion);
+    return block.testAndSetMarked(cell, dependency);
 }
 
 ALWAYS_INLINE size_t Heap::cellSize(const void* rawCell)
index 528575b..dfd18f3 100644 (file)
@@ -74,8 +74,9 @@ public:
     
     bool isNewlyAllocated() const { return m_isNewlyAllocated; }
     ALWAYS_INLINE bool isMarked() { return m_isMarked.load(std::memory_order_relaxed); }
-    ALWAYS_INLINE bool isMarked(HeapCell*) { return m_isMarked.load(std::memory_order_relaxed); }
-    ALWAYS_INLINE bool isMarkedConcurrently(HeapVersion, HeapCell*) { return m_isMarked.load(std::memory_order_relaxed); }
+    ALWAYS_INLINE bool isMarked(HeapCell*) { return isMarked(); }
+    ALWAYS_INLINE bool isMarked(HeapCell*, Dependency) { return isMarked(); }
+    ALWAYS_INLINE bool isMarkedConcurrently(HeapVersion, HeapCell*) { return isMarked(); }
     bool isLive() { return isMarked() || isNewlyAllocated(); }
     
     bool hasValidCell() const { return m_hasValidCell; }
@@ -109,7 +110,7 @@ public:
     
     const AllocatorAttributes& attributes() const { return m_attributes; }
     
-    void aboutToMark(HeapVersion) { }
+    Dependency aboutToMark(HeapVersion) { return nullDependency(); }
     
     ALWAYS_INLINE bool testAndSetMarked()
     {
@@ -120,7 +121,7 @@ public:
             return true;
         return m_isMarked.compareExchangeStrong(false, true);
     }
-    ALWAYS_INLINE bool testAndSetMarked(HeapCell*) { return testAndSetMarked(); }
+    ALWAYS_INLINE bool testAndSetMarked(HeapCell*, Dependency, TransactionAbortLikelihood = TransactionAbortLikelihood::Likely) { return testAndSetMarked(); }
     void clearMarked() { m_isMarked.store(false); }
     
     void noteMarked() { }
index b105941..22a7ba7 100644 (file)
@@ -258,7 +258,8 @@ public:
     bool isMarked(const void*);
     bool isMarked(HeapVersion markingVersion, const void*);
     bool isMarkedConcurrently(HeapVersion markingVersion, const void*);
-    bool testAndSetMarked(const void*);
+    bool isMarked(const void*, Dependency);
+    bool testAndSetMarked(const void*, Dependency, TransactionAbortLikelihood = TransactionAbortLikelihood::Likely);
         
     bool isAtom(const void*);
     void clearMarked(const void*);
@@ -278,15 +279,15 @@ public:
 
     JS_EXPORT_PRIVATE bool areMarksStale();
     bool areMarksStale(HeapVersion markingVersion);
-    struct MarksWithDependency {
-        bool areStale;
-        ConsumeDependency dependency;
-    };
-    MarksWithDependency areMarksStaleWithDependency(HeapVersion markingVersion);
+    DependencyWith<bool> areMarksStaleWithDependency(HeapVersion markingVersion);
     
-    void aboutToMark(HeapVersion markingVersion);
+    Dependency aboutToMark(HeapVersion markingVersion);
         
-    void assertMarksNotStale();
+#if ASSERT_DISABLED
+    void assertMarksNotStale() { }
+#else
+    JS_EXPORT_PRIVATE void assertMarksNotStale();
+#endif
         
     bool needsDestruction() const { return m_needsDestruction; }
     
@@ -306,7 +307,7 @@ private:
     MarkedBlock(VM&, Handle&);
     Atom* atoms();
         
-    void aboutToMarkSlow(HeapVersion markingVersion);
+    JS_EXPORT_PRIVATE void aboutToMarkSlow(HeapVersion markingVersion);
     void clearHasAnyMarked();
     
     void noteMarkedSlow();
@@ -491,27 +492,19 @@ inline bool MarkedBlock::areMarksStale(HeapVersion markingVersion)
     return markingVersion != m_markingVersion;
 }
 
-ALWAYS_INLINE MarkedBlock::MarksWithDependency MarkedBlock::areMarksStaleWithDependency(HeapVersion markingVersion)
+ALWAYS_INLINE DependencyWith<bool> MarkedBlock::areMarksStaleWithDependency(HeapVersion markingVersion)
 {
-    auto consumed = consumeLoad(&m_markingVersion);
-    MarksWithDependency ret;
-    ret.areStale = consumed.value != markingVersion;
-    ret.dependency = consumed.dependency;
-    return ret;
+    HeapVersion version = m_markingVersion;
+    return dependencyWith(dependency(version), version != markingVersion);
 }
 
-inline void MarkedBlock::aboutToMark(HeapVersion markingVersion)
+inline Dependency MarkedBlock::aboutToMark(HeapVersion markingVersion)
 {
-    if (UNLIKELY(areMarksStale(markingVersion)))
+    auto result = areMarksStaleWithDependency(markingVersion);
+    if (UNLIKELY(result.value))
         aboutToMarkSlow(markingVersion);
-    WTF::loadLoadFence();
-}
-
-#if ASSERT_DISABLED
-inline void MarkedBlock::assertMarksNotStale()
-{
+    return result.dependency;
 }
-#endif // ASSERT_DISABLED
 
 inline void MarkedBlock::Handle::assertMarksNotStale()
 {
@@ -530,16 +523,22 @@ inline bool MarkedBlock::isMarked(HeapVersion markingVersion, const void* p)
 
 inline bool MarkedBlock::isMarkedConcurrently(HeapVersion markingVersion, const void* p)
 {
-    auto marksWithDependency = areMarksStaleWithDependency(markingVersion);
-    if (marksWithDependency.areStale)
+    auto result = areMarksStaleWithDependency(markingVersion);
+    if (result.value)
         return false;
-    return m_marks.get(atomNumber(p) + marksWithDependency.dependency);
+    return m_marks.get(atomNumber(p), result.dependency);
+}
+
+inline bool MarkedBlock::isMarked(const void* p, Dependency dependency)
+{
+    assertMarksNotStale();
+    return m_marks.get(atomNumber(p), dependency);
 }
 
-inline bool MarkedBlock::testAndSetMarked(const void* p)
+inline bool MarkedBlock::testAndSetMarked(const void* p, Dependency dependency, TransactionAbortLikelihood abortLikelihood)
 {
     assertMarksNotStale();
-    return m_marks.concurrentTestAndSet(atomNumber(p));
+    return m_marks.concurrentTestAndSet(atomNumber(p), dependency, abortLikelihood);
 }
 
 inline bool MarkedBlock::Handle::isNewlyAllocated(const void* p)
index f0260fc..62fe98f 100644 (file)
@@ -222,49 +222,37 @@ void SlotVisitor::appendJSCellOrAuxiliary(HeapCell* heapCell)
     } }
 }
 
-void SlotVisitor::appendUnbarriered(JSValue value)
+void SlotVisitor::appendSlow(JSCell* cell, Dependency dependency)
 {
-    if (!value || !value.isCell())
-        return;
-
     if (UNLIKELY(m_heapSnapshotBuilder))
-        m_heapSnapshotBuilder->appendEdge(m_currentCell, value.asCell());
-
-    setMarkedAndAppendToMarkStack(value.asCell());
+        m_heapSnapshotBuilder->appendEdge(m_currentCell, cell);
+    
+    appendHiddenSlowImpl(cell, dependency);
 }
 
-void SlotVisitor::appendHidden(JSValue value)
+void SlotVisitor::appendHiddenSlow(JSCell* cell, Dependency dependency)
 {
-    if (!value || !value.isCell())
-        return;
-
-    setMarkedAndAppendToMarkStack(value.asCell());
+    appendHiddenSlowImpl(cell, dependency);
 }
 
-void SlotVisitor::setMarkedAndAppendToMarkStack(JSCell* cell)
+ALWAYS_INLINE void SlotVisitor::appendHiddenSlowImpl(JSCell* cell, Dependency dependency)
 {
-    SuperSamplerScope superSamplerScope(false);
-    
     ASSERT(!m_isCheckingForDefaultMarkViolation);
-    if (!cell)
-        return;
 
 #if ENABLE(GC_VALIDATION)
     validate(cell);
 #endif
     
     if (cell->isLargeAllocation())
-        setMarkedAndAppendToMarkStack(cell->largeAllocation(), cell);
+        setMarkedAndAppendToMarkStack(cell->largeAllocation(), cell, dependency);
     else
-        setMarkedAndAppendToMarkStack(cell->markedBlock(), cell);
+        setMarkedAndAppendToMarkStack(cell->markedBlock(), cell, dependency);
 }
 
 template<typename ContainerType>
-ALWAYS_INLINE void SlotVisitor::setMarkedAndAppendToMarkStack(ContainerType& container, JSCell* cell)
+ALWAYS_INLINE void SlotVisitor::setMarkedAndAppendToMarkStack(ContainerType& container, JSCell* cell, Dependency dependency)
 {
-    container.aboutToMark(m_markingVersion);
-    
-    if (container.testAndSetMarked(cell))
+    if (container.testAndSetMarked(cell, dependency, TransactionAbortLikelihood::Unlikely))
         return;
     
     ASSERT(cell->structure());
index 83479af..37e2a9a 100644 (file)
@@ -175,11 +175,14 @@ private:
     
     void appendJSCellOrAuxiliary(HeapCell*);
     void appendHidden(JSValue);
+    void appendHidden(JSCell*);
 
-    JS_EXPORT_PRIVATE void setMarkedAndAppendToMarkStack(JSCell*);
+    JS_EXPORT_PRIVATE void appendSlow(JSCell*, Dependency);
+    JS_EXPORT_PRIVATE void appendHiddenSlow(JSCell*, Dependency);
+    void appendHiddenSlowImpl(JSCell*, Dependency);
     
     template<typename ContainerType>
-    void setMarkedAndAppendToMarkStack(ContainerType&, JSCell*);
+    void setMarkedAndAppendToMarkStack(ContainerType&, JSCell*, Dependency);
     
     void appendToMarkStack(JSCell*);
     
index 06475a0..3d31964 100644 (file)
 
 namespace JSC {
 
-inline void SlotVisitor::appendUnbarriered(JSValue* slot, size_t count)
+ALWAYS_INLINE void SlotVisitor::appendUnbarriered(JSValue* slot, size_t count)
 {
     for (size_t i = count; i--;)
         appendUnbarriered(slot[i]);
 }
 
-inline void SlotVisitor::appendUnbarriered(JSCell* cell)
+ALWAYS_INLINE void SlotVisitor::appendUnbarriered(JSCell* cell)
 {
-    appendUnbarriered(JSValue(cell));
+    // This needs to be written in a very specific way to ensure that it gets inlined
+    // properly. In particular, it appears that using templates here breaks ALWAYS_INLINE.
+    
+    if (!cell)
+        return;
+    
+    Dependency dependency;
+    if (UNLIKELY(cell->isLargeAllocation())) {
+        dependency = nullDependency();
+        if (LIKELY(cell->largeAllocation().isMarked())) {
+            if (LIKELY(!m_heapSnapshotBuilder))
+                return;
+        }
+    } else {
+        MarkedBlock& block = cell->markedBlock();
+        dependency = block.aboutToMark(m_markingVersion);
+        if (LIKELY(block.isMarked(cell, dependency))) {
+            if (LIKELY(!m_heapSnapshotBuilder))
+                return;
+        }
+    }
+    
+    appendSlow(cell, dependency);
+}
+
+ALWAYS_INLINE void SlotVisitor::appendUnbarriered(JSValue value)
+{
+    if (value.isCell())
+        appendUnbarriered(value.asCell());
+}
+
+ALWAYS_INLINE void SlotVisitor::appendHidden(JSValue value)
+{
+    if (value.isCell())
+        appendHidden(value.asCell());
+}
+
+ALWAYS_INLINE void SlotVisitor::appendHidden(JSCell* cell)
+{
+    // This needs to be written in a very specific way to ensure that it gets inlined
+    // properly. In particular, it appears that using templates here breaks ALWAYS_INLINE.
+    
+    if (!cell)
+        return;
+    
+    Dependency dependency;
+    if (UNLIKELY(cell->isLargeAllocation())) {
+        dependency = nullDependency();
+        if (LIKELY(cell->largeAllocation().isMarked()))
+            return;
+    } else {
+        MarkedBlock& block = cell->markedBlock();
+        dependency = block.aboutToMark(m_markingVersion);
+        if (LIKELY(block.isMarked(cell, dependency)))
+            return;
+    }
+    
+    appendHiddenSlow(cell, dependency);
 }
 
 template<typename T>
-inline void SlotVisitor::append(const Weak<T>& weak)
+ALWAYS_INLINE void SlotVisitor::append(const Weak<T>& weak)
 {
     appendUnbarriered(weak.get());
 }
 
 template<typename T>
-inline void SlotVisitor::append(const WriteBarrierBase<T>& slot)
+ALWAYS_INLINE void SlotVisitor::append(const WriteBarrierBase<T>& slot)
 {
     appendUnbarriered(slot.get());
 }
 
 template<typename T>
-inline void SlotVisitor::appendHidden(const WriteBarrierBase<T>& slot)
+ALWAYS_INLINE void SlotVisitor::appendHidden(const WriteBarrierBase<T>& slot)
 {
     appendHidden(slot.get());
 }
 
 template<typename Iterator>
-inline void SlotVisitor::append(Iterator begin, Iterator end)
+ALWAYS_INLINE void SlotVisitor::append(Iterator begin, Iterator end)
 {
     for (auto it = begin; it != end; ++it)
         append(*it);
 }
 
-inline void SlotVisitor::appendValues(const WriteBarrierBase<Unknown>* barriers, size_t count)
+ALWAYS_INLINE void SlotVisitor::appendValues(const WriteBarrierBase<Unknown>* barriers, size_t count)
 {
     for (size_t i = 0; i < count; ++i)
         append(barriers[i]);
 }
 
-inline void SlotVisitor::appendValuesHidden(const WriteBarrierBase<Unknown>* barriers, size_t count)
+ALWAYS_INLINE void SlotVisitor::appendValuesHidden(const WriteBarrierBase<Unknown>* barriers, size_t count)
 {
     for (size_t i = 0; i < count; ++i)
         appendHidden(barriers[i]);
index dea45c4..b80067b 100644 (file)
@@ -26,9 +26,7 @@
 #include "config.h"
 #include "CustomGetterSetter.h"
 
-#include "JSCJSValueInlines.h"
-#include "JSObject.h"
-#include "SlotVisitorInlines.h"
+#include "JSCInlines.h"
 #include <wtf/Assertions.h>
 
 namespace JSC {
index 77967e0..2ad3818 100644 (file)
@@ -382,6 +382,7 @@ ALWAYS_INLINE Structure* JSObject::visitButterflyImpl(SlotVisitor& visitor)
     structure = vm.getStructure(structureID);
     lastOffset = structure->lastOffset();
     IndexingType indexingType = structure->indexingType();
+    Dependency indexingTypeDependency = dependency(indexingType);
     Locker<JSCell> locker(NoLockingNecessary);
     switch (indexingType) {
     case ALL_CONTIGUOUS_INDEXING_TYPES:
@@ -395,14 +396,13 @@ ALWAYS_INLINE Structure* JSObject::visitButterflyImpl(SlotVisitor& visitor)
     default:
         break;
     }
-    WTF::loadLoadFence();
-    butterfly = this->butterfly();
+    butterfly = consume(this, indexingTypeDependency)->butterfly();
+    Dependency butterflyDependency = dependency(butterfly);
     if (!butterfly)
         return structure;
-    WTF::loadLoadFence();
-    if (this->structureID() != structureID)
+    if (consume(this, butterflyDependency)->structureID() != structureID)
         return nullptr;
-    if (structure->lastOffset() != lastOffset)
+    if (consume(structure, butterflyDependency)->lastOffset() != lastOffset)
         return nullptr;
     
     markAuxiliaryAndVisitOutOfLineProperties(visitor, butterfly, structure, lastOffset);
index 7e4e80e..fb7881d 100644 (file)
@@ -664,7 +664,7 @@ public:
         
     const Butterfly* butterfly() const { return m_butterfly.get(); }
     Butterfly* butterfly() { return m_butterfly.get(); }
-        
+    
     ConstPropertyStorage outOfLineStorage() const { return m_butterfly.get()->propertyStorage(); }
     PropertyStorage outOfLineStorage() { return m_butterfly.get()->propertyStorage(); }
 
index 380394f..7a4325e 100644 (file)
@@ -1,3 +1,66 @@
+2017-03-07  Filip Pizlo  <fpizlo@apple.com>
+
+        WTF should make it super easy to do ARM concurrency tricks
+        https://bugs.webkit.org/show_bug.cgi?id=169300
+
+        Reviewed by Mark Lam.
+        
+        This adds Atomic<>::loadLink and Atomic<>::storeCond, available only when HAVE(LL_SC).
+        
+        It abstracts loadLink/storeCond behind prepare/attempt. You can write prepare/attempt
+        loops whenever your loop fits into the least common denominator of LL/SC and CAS.
+        
+        This modifies Atomic<>::transaction to use prepare/attempt. So, if you write your loop
+        using Atomic<>::transaction, then you get LL/SC for free.
+
+        Depending on the kind of transaction you are doing, you may not want to perform an LL
+        until you have a chance to just load the current value. Atomic<>::transaction() assumes
+        that you do not care to have any ordering guarantees in that case. If you think that
+        the transaction has a good chance of aborting this way, you want
+        Atomic<>::transaction() to first do a plain load. But if you don't think that such an
+        abort is likely, then you want to go straight to the LL. The API supports this concept
+        via TransactionAbortLikelihood.
+        
+        Additionally, this redoes the depend/consume API to be dead simple. Dependency is
+        unsigned. You get a dependency on a loaded value by just saying
+        dependency(loadedValue). You consume the dependency by using it as a bonus index to
+        some pointer dereference. This is made easy with the consume<T*>(ptr, dependency)
+        helper. In those cases where you want to pass around both a computed value and a
+        dependency, there's DependencyWith<T>. But you won't need it in most cases. The loaded
+        value or any value computed from the loaded value is a fine input to dependency()!
+        
+        This change updates a bunch of hot paths to use the new APIs. Using transaction() gives
+        us optimal LL/SC loops for object marking and lock acquisition.
+        
+        This change also updates a bunch of hot paths to use dependency()/consume().
+        
+        This is a significant Octane/splay speed-up on ARM.
+
+        * wtf/Atomics.h:
+        (WTF::hasFence):
+        (WTF::Atomic::prepare):
+        (WTF::Atomic::attempt):
+        (WTF::Atomic::transaction):
+        (WTF::Atomic::transactionRelaxed):
+        (WTF::nullDependency):
+        (WTF::dependency):
+        (WTF::DependencyWith::DependencyWith):
+        (WTF::dependencyWith):
+        (WTF::consume):
+        (WTF::Atomic::tryTransactionRelaxed): Deleted.
+        (WTF::Atomic::tryTransaction): Deleted.
+        (WTF::zeroWithConsumeDependency): Deleted.
+        (WTF::consumeLoad): Deleted.
+        * wtf/Bitmap.h:
+        (WTF::WordType>::get):
+        (WTF::WordType>::concurrentTestAndSet):
+        (WTF::WordType>::concurrentTestAndClear):
+        * wtf/LockAlgorithm.h:
+        (WTF::LockAlgorithm::lockFast):
+        (WTF::LockAlgorithm::unlockFast):
+        (WTF::LockAlgorithm::unlockSlow):
+        * wtf/Platform.h:
+
 2017-03-08  Anders Carlsson  <andersca@apple.com>
 
         Simplify the PaymentCoordinator interface
index 4a8ee48..e4a3283 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2007-2008, 2010, 2012-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2007-2017 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Justin Haygood (jhaygood@reaktix.com)
  *
  * Redistribution and use in source and binary forms, with or without
@@ -40,6 +40,16 @@ extern "C" void _ReadWriteBarrier(void);
 
 namespace WTF {
 
+ALWAYS_INLINE bool hasFence(std::memory_order order)
+{
+    return order != std::memory_order_relaxed;
+}
+
+enum class TransactionAbortLikelihood {
+    Unlikely,
+    Likely
+};
+    
 // Atomic wraps around std::atomic with the sole purpose of making the compare_exchange
 // operations not alter the expected value. This is more in line with how we typically
 // use CAS in our code.
@@ -107,39 +117,129 @@ struct Atomic {
     
     ALWAYS_INLINE T exchange(T newValue, std::memory_order order = std::memory_order_seq_cst) { return value.exchange(newValue, order); }
     
-    template<typename Func>
-    ALWAYS_INLINE bool tryTransactionRelaxed(const Func& func)
+#if HAVE(LL_SC)
+    ALWAYS_INLINE T loadLink(std::memory_order order = std::memory_order_seq_cst);
+    ALWAYS_INLINE bool storeCond(T value,  std::memory_order order = std::memory_order_seq_cst);
+#endif // HAVE(LL_SC)
+
+    ALWAYS_INLINE T prepare(std::memory_order order = std::memory_order_seq_cst)
     {
-        T oldValue = load(std::memory_order_relaxed);
-        T newValue = oldValue;
-        func(newValue);
-        return compareExchangeWeakRelaxed(oldValue, newValue);
+#if HAVE(LL_SC)
+        return loadLink(order);
+#else
+        UNUSED_PARAM(order);
+        return load(std::memory_order_relaxed);
+#endif
     }
+    
+#if HAVE(LL_SC)
+    static const bool prepareIsFast = false;
+#else
+    static const bool prepareIsFast = true;
+#endif
 
-    template<typename Func>
-    ALWAYS_INLINE void transactionRelaxed(const Func& func)
+    ALWAYS_INLINE bool attempt(T oldValue, T newValue, std::memory_order order = std::memory_order_seq_cst)
     {
-        while (!tryTransationRelaxed(func)) { }
+#if HAVE(LL_SC)
+        UNUSED_PARAM(oldValue);
+        return storeCond(newValue, order);
+#else
+        return compareExchangeWeak(oldValue, newValue, order);
+#endif
     }
 
     template<typename Func>
-    ALWAYS_INLINE bool tryTransaction(const Func& func)
+    ALWAYS_INLINE bool transaction(const Func& func, std::memory_order order = std::memory_order_seq_cst, TransactionAbortLikelihood abortLikelihood = TransactionAbortLikelihood::Likely)
     {
-        T oldValue = load(std::memory_order_relaxed);
-        T newValue = oldValue;
-        func(newValue);
-        return compareExchangeWeak(oldValue, newValue);
+        // If preparing is not fast then we want to skip the loop when func would fail.
+        if (!prepareIsFast && abortLikelihood == TransactionAbortLikelihood::Likely) {
+            T oldValue = load(std::memory_order_relaxed);
+            // Note: many funcs will constant-fold to true, which will kill all of this code.
+            if (!func(oldValue))
+                return false;
+        }
+        for (;;) {
+            T oldValue = prepare(order);
+            T newValue = oldValue;
+            if (!func(newValue))
+                return false;
+            if (attempt(oldValue, newValue, order))
+                return true;
+        }
     }
 
     template<typename Func>
-    ALWAYS_INLINE void transaction(const Func& func)
+    ALWAYS_INLINE bool transactionRelaxed(const Func& func, TransactionAbortLikelihood abortLikelihood = TransactionAbortLikelihood::Likely)
     {
-        while (!tryTransaction(func)) { }
+        return transaction(func, std::memory_order_relaxed, abortLikelihood);
     }
 
     std::atomic<T> value;
 };
 
+#if CPU(ARM64)
+#define DEFINE_LL_SC(width, modifier, suffix)   \
+    template<> \
+    ALWAYS_INLINE uint ## width ## _t Atomic<uint ## width ##_t>::loadLink(std::memory_order order) \
+    { \
+        int ## width ## _t result; \
+        if (hasFence(order)) { \
+            asm volatile ( \
+                "ldaxr" suffix " %" modifier "0, [%1]" \
+                : "=r"(result) \
+                : "r"(this) \
+                : "memory"); \
+        } else { \
+            asm ( \
+                "ldxr" suffix " %" modifier "0, [%1]" \
+                : "=r"(result) \
+                : "r"(this) \
+                : "memory"); \
+        } \
+        return result; \
+    } \
+    \
+    template<> \
+    ALWAYS_INLINE bool Atomic<uint ## width ## _t>::storeCond(uint ## width ## _t value, std::memory_order order) \
+    { \
+        bool result; \
+        if (hasFence(order)) { \
+            asm volatile ( \
+                "stlxr" suffix " %w0, %" modifier "1, [%2]" \
+                : "=&r"(result) \
+                : "r"(value), "r"(this) \
+                : "memory"); \
+        } else { \
+            asm ( \
+                "stxr" suffix " %w0, %" modifier "1, [%2]" \
+                : "=&r"(result) \
+                : "r"(value), "r"(this) \
+                : "memory"); \
+        } \
+        return !result; \
+    } \
+    \
+    template<> \
+    ALWAYS_INLINE int ## width ## _t Atomic<int ## width ## _t>::loadLink(std::memory_order order) \
+    { \
+        return bitwise_cast<Atomic<uint ## width ## _t>*>(this)->loadLink(order); \
+    } \
+    \
+    template<> \
+    ALWAYS_INLINE bool Atomic<int ## width ## _t>::storeCond(int ## width ## _t value, std::memory_order order) \
+    { \
+        return bitwise_cast<Atomic<uint ## width ## _t>*>(this)->storeCond(value, order); \
+    }
+
+DEFINE_LL_SC(8, "w", "b")
+DEFINE_LL_SC(16, "w", "h")
+DEFINE_LL_SC(32, "w", "")
+DEFINE_LL_SC(64, "", "")
+DEFINE_LL_SC(ptr, "", "")
+
+#undef DEFINE_LL_SC
+#endif // CPU(ARM64)
+
 template<typename T>
 inline T atomicLoad(T* location, std::memory_order order = std::memory_order_seq_cst)
 {
@@ -308,12 +408,17 @@ inline void crossModifyingCodeFence() { std::atomic_thread_fence(std::memory_ord
 
 #endif
 
-typedef size_t ConsumeDependency;
+typedef unsigned Dependency;
+
+ALWAYS_INLINE Dependency nullDependency()
+{
+    return 0;
+}
 
 template <typename T, typename std::enable_if<sizeof(T) == 8>::type* = nullptr>
-ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
+ALWAYS_INLINE Dependency dependency(T value)
 {
-    uint64_t dependency;
+    unsigned dependency;
     uint64_t copy = bitwise_cast<uint64_t>(value);
 #if CPU(ARM64)
     // Create a magical zero value through inline assembly, whose computation
@@ -325,104 +430,102 @@ ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
     // ordering. This forces weak memory order CPUs to observe `location` and
     // dependent loads in their store order without the reader using a barrier
     // or an acquire load.
-    asm volatile("eor %x[dependency], %x[in], %x[in]"
-                 : [dependency] "=r"(dependency)
-                 : [in] "r"(copy)
-                 // Lie about touching memory. Not strictly needed, but is
-                 // likely to avoid unwanted load/store motion.
-                 : "memory");
+    asm("eor %w[dependency], %w[in], %w[in]"
+        : [dependency] "=r"(dependency)
+        : [in] "r"(copy));
 #elif CPU(ARM)
-    asm volatile("eor %[dependency], %[in], %[in]"
-                 : [dependency] "=r"(dependency)
-                 : [in] "r"(copy)
-                 : "memory");
+    asm("eor %[dependency], %[in], %[in]"
+        : [dependency] "=r"(dependency)
+        : [in] "r"(copy));
 #else
     // No dependency is needed for this architecture.
     loadLoadFence();
     dependency = 0;
-    (void)copy;
+    UNUSED_PARAM(copy);
 #endif
-    return static_cast<ConsumeDependency>(dependency);
+    return dependency;
 }
 
+// FIXME: This code is almost identical to the other dependency() overload.
+// https://bugs.webkit.org/show_bug.cgi?id=169405
 template <typename T, typename std::enable_if<sizeof(T) == 4>::type* = nullptr>
-ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
+ALWAYS_INLINE Dependency dependency(T value)
 {
-    uint32_t dependency;
+    unsigned dependency;
     uint32_t copy = bitwise_cast<uint32_t>(value);
 #if CPU(ARM64)
-    asm volatile("eor %w[dependency], %w[in], %w[in]"
-                 : [dependency] "=r"(dependency)
-                 : [in] "r"(copy)
-                 : "memory");
+    asm("eor %w[dependency], %w[in], %w[in]"
+        : [dependency] "=r"(dependency)
+        : [in] "r"(copy));
 #elif CPU(ARM)
-    asm volatile("eor %[dependency], %[in], %[in]"
-                 : [dependency] "=r"(dependency)
-                 : [in] "r"(copy)
-                 : "memory");
+    asm("eor %[dependency], %[in], %[in]"
+        : [dependency] "=r"(dependency)
+        : [in] "r"(copy));
 #else
     loadLoadFence();
     dependency = 0;
-    (void)copy;
+    UNUSED_PARAM(copy);
 #endif
-    return static_cast<ConsumeDependency>(dependency);
+    return dependency;
 }
 
 template <typename T, typename std::enable_if<sizeof(T) == 2>::type* = nullptr>
-ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
+ALWAYS_INLINE Dependency dependency(T value)
 {
-    uint16_t copy = bitwise_cast<uint16_t>(value);
-    return zeroWithConsumeDependency(static_cast<size_t>(copy));
+    return dependency(static_cast<uint32_t>(value));
 }
 
 template <typename T, typename std::enable_if<sizeof(T) == 1>::type* = nullptr>
-ALWAYS_INLINE ConsumeDependency zeroWithConsumeDependency(T value)
+ALWAYS_INLINE Dependency dependency(T value)
 {
-    uint8_t copy = bitwise_cast<uint8_t>(value);
-    return zeroWithConsumeDependency(static_cast<size_t>(copy));
+    return dependency(static_cast<uint32_t>(value));
 }
 
-template <typename T>
-struct Consumed {
+template<typename T>
+struct DependencyWith {
+public:
+    DependencyWith()
+        : dependency(nullDependency())
+        , value()
+    {
+    }
+    
+    DependencyWith(Dependency dependency, const T& value)
+        : dependency(dependency)
+        , value(value)
+    {
+    }
+    
+    Dependency dependency;
     T value;
-    ConsumeDependency dependency;
 };
+    
+template<typename T>
+inline DependencyWith<T> dependencyWith(Dependency dependency, const T& value)
+{
+    return DependencyWith<T>(dependency, value);
+}
 
-// Consume load, returning the loaded `value` at `location` and a dependent-zero
-// which creates an address dependency from the `location`.
-//
-// Usage notes:
-//
-//  * Regarding control dependencies: merely branching based on `value` or
-//    `dependency` isn't sufficient to impose a dependency ordering: you must
-//    use `dependency` in the address computation of subsequent loads which
-//    should observe the store order w.r.t. `location`.
-// * Regarding memory ordering: consume load orders the `location` load with
-//   susequent dependent loads *only*. It says nothing about ordering of other
-//   loads!
-//
-// Caveat emptor.
-template <typename T>
-ALWAYS_INLINE auto consumeLoad(const T* location)
+template<typename T>
+inline T* consume(T* pointer, Dependency dependency)
 {
-    typedef typename std::remove_cv<T>::type Returned;
-    Consumed<Returned> ret { };
-    // Force the read of `location` to occur exactly once and without fusing or
-    // forwarding using volatile. This is important because the compiler could
-    // otherwise rematerialize or find equivalent loads, or simply forward from
-    // a previous one, and lose the dependency we're trying so hard to
-    // create. Prevent tearing by using an atomic, but let it move around by
-    // using relaxed. We have at least a memory fence after this which prevents
-    // the load from moving too much.
-    ret.value = reinterpret_cast<const volatile std::atomic<Returned>*>(location)->load(std::memory_order_relaxed);
-    ret.dependency = zeroWithConsumeDependency(ret.value);
-    return ret;
+#if CPU(ARM64) || CPU(ARM)
+    return bitwise_cast<T*>(bitwise_cast<char*>(pointer) + dependency);
+#else
+    UNUSED_PARAM(dependency);
+    return pointer;
+#endif
 }
 
 } // namespace WTF
 
 using WTF::Atomic;
-using WTF::ConsumeDependency;
-using WTF::consumeLoad;
+using WTF::Dependency;
+using WTF::DependencyWith;
+using WTF::TransactionAbortLikelihood;
+using WTF::consume;
+using WTF::dependency;
+using WTF::dependencyWith;
+using WTF::nullDependency;
 
 #endif // Atomics_h
index 89b2c9d..b2e7879 100644 (file)
@@ -40,13 +40,13 @@ public:
         return bitmapSize;
     }
 
-    bool get(size_t) const;
+    bool get(size_t, Dependency = nullDependency()) const;
     void set(size_t);
     void set(size_t, bool);
     bool testAndSet(size_t);
     bool testAndClear(size_t);
-    bool concurrentTestAndSet(size_t);
-    bool concurrentTestAndClear(size_t);
+    bool concurrentTestAndSet(size_t, Dependency = nullDependency(), TransactionAbortLikelihood = TransactionAbortLikelihood::Likely);
+    bool concurrentTestAndClear(size_t, Dependency = nullDependency(), TransactionAbortLikelihood = TransactionAbortLikelihood::Likely);
     size_t nextPossiblyUnset(size_t) const;
     void clear(size_t);
     void clearAll();
@@ -91,9 +91,9 @@ inline Bitmap<bitmapSize, WordType>::Bitmap()
 }
 
 template<size_t bitmapSize, typename WordType>
-inline bool Bitmap<bitmapSize, WordType>::get(size_t n) const
+inline bool Bitmap<bitmapSize, WordType>::get(size_t n, Dependency dependency) const
 {
-    return !!(bits[n / wordSize] & (one << (n % wordSize)));
+    return !!(bits[n / wordSize + dependency] & (one << (n % wordSize)));
 }
 
 template<size_t bitmapSize, typename WordType>
@@ -132,33 +132,37 @@ inline bool Bitmap<bitmapSize, WordType>::testAndClear(size_t n)
 }
 
 template<size_t bitmapSize, typename WordType>
-inline bool Bitmap<bitmapSize, WordType>::concurrentTestAndSet(size_t n)
+ALWAYS_INLINE bool Bitmap<bitmapSize, WordType>::concurrentTestAndSet(size_t n, Dependency dependency, TransactionAbortLikelihood abortLikelihood)
 {
     WordType mask = one << (n % wordSize);
     size_t index = n / wordSize;
-    WordType* wordPtr = bits.data() + index;
-    WordType oldValue;
-    do {
-        oldValue = *wordPtr;
-        if (oldValue & mask)
+    WordType* data = bits.data() + index + dependency;
+    return !bitwise_cast<Atomic<WordType>*>(data)->transactionRelaxed(
+        [&] (WordType& value) -> bool {
+            if (value & mask)
+                return false;
+            
+            value |= mask;
             return true;
-    } while (!atomicCompareExchangeWeakRelaxed(wordPtr, oldValue, static_cast<WordType>(oldValue | mask)));
-    return false;
+        },
+        abortLikelihood);
 }
 
 template<size_t bitmapSize, typename WordType>
-inline bool Bitmap<bitmapSize, WordType>::concurrentTestAndClear(size_t n)
+ALWAYS_INLINE bool Bitmap<bitmapSize, WordType>::concurrentTestAndClear(size_t n, Dependency dependency, TransactionAbortLikelihood abortLikelihood)
 {
     WordType mask = one << (n % wordSize);
     size_t index = n / wordSize;
-    WordType* wordPtr = bits.data() + index;
-    WordType oldValue;
-    do {
-        oldValue = *wordPtr;
-        if (!(oldValue & mask))
-            return false;
-    } while (!atomicCompareExchangeWeakRelaxed(wordPtr, oldValue, static_cast<WordType>(oldValue & ~mask)));
-    return true;
+    WordType* data = bits.data() + index + dependency;
+    return !bitwise_cast<Atomic<WordType>*>(data)->transactionRelaxed(
+        [&] (WordType& value) -> bool {
+            if (!(value & mask))
+                return false;
+            
+            value &= ~mask;
+            return true;
+        },
+        abortLikelihood);
 }
 
 template<size_t bitmapSize, typename WordType>
index 856788d..e347d20 100644 (file)
@@ -50,10 +50,15 @@ public:
     
     static bool lockFast(Atomic<LockType>& lock)
     {
-        LockType oldValue = lock.load(std::memory_order_relaxed);
-        if (oldValue & isHeldBit)
-            return false;
-        return lock.compareExchangeWeak(oldValue, oldValue | isHeldBit, std::memory_order_acquire);
+        return lock.transaction(
+            [&] (LockType& value) -> bool {
+                if (value & isHeldBit)
+                    return false;
+                value |= isHeldBit;
+                return true;
+            },
+            std::memory_order_acquire,
+            TransactionAbortLikelihood::Unlikely);
     }
     
     static void lock(Atomic<LockType>& lock)
@@ -80,10 +85,15 @@ public:
     
     static bool unlockFast(Atomic<LockType>& lock)
     {
-        LockType oldValue = lock.load(std::memory_order_relaxed);
-        if ((oldValue & mask) != isHeldBit)
-            return false;
-        return lock.compareExchangeWeak(oldValue, oldValue & ~isHeldBit, std::memory_order_release);
+        return lock.transaction(
+            [&] (LockType& value) -> bool {
+                if ((value & mask) != isHeldBit)
+                    return false;
+                value &= ~isHeldBit;
+                return true;
+            },
+            std::memory_order_relaxed,
+            TransactionAbortLikelihood::Unlikely);
     }
     
     static void unlock(Atomic<LockType>& lock)
@@ -206,10 +216,11 @@ public:
                     }
                     
                     lock.transaction(
-                        [&] (LockType& value) {
+                        [&] (LockType& value) -> bool {
                             value &= ~mask;
                             if (result.mayHaveMoreThreads)
                                 value |= hasParkedBit;
+                            return true;
                         });
                     return BargingOpportunity;
                 });
index 7aba048..7519062 100644 (file)
 #define ENABLE_CONCURRENT_JS 1
 #endif
 
+#if CPU(ARM64)
+#define HAVE_LL_SC 1
+#endif // CPU(ARM64)
+
 /* This controls whether B3 is built. B3 is needed for FTL JIT and WebAssembly */
 #if ENABLE(FTL_JIT) || ENABLE(WEBASSEMBLY)
 #define ENABLE_B3_JIT 1
index c9f71a7..9dde796 100644 (file)
@@ -1,3 +1,18 @@
+2017-03-08  Filip Pizlo  <fpizlo@apple.com>
+
+        WTF should make it super easy to do ARM concurrency tricks
+        https://bugs.webkit.org/show_bug.cgi?id=169300
+
+        Reviewed by Mark Lam.
+        
+        This vastly simplifies the consume API. The new API is thoroughly tested by being used
+        in the GC's guts. I think that unit tests are a pain to maintain, so we shouldn't have
+        them unless we are legitimately worried about coverage. We're not in this case.
+
+        * TestWebKitAPI/CMakeLists.txt:
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WTF/Consume.cpp: Removed.
+
 2017-03-08  Srinivasan Vijayaraghavan  <svijayaraghavan@apple.com>
 
         Fix error/warning duplication in JSON bindings results
index de2699e..72a1fcd 100644 (file)
@@ -42,7 +42,6 @@ set(TestWTF_SOURCES
     ${TESTWEBKITAPI_DIR}/TestsController.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/AtomicString.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/BloomFilter.cpp
-    ${TESTWEBKITAPI_DIR}/Tests/WTF/Consume.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/CrossThreadTask.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/CString.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/CheckedArithmeticOperations.cpp
index 5540d88..1a994b7 100644 (file)
                AD57AC211DA7465B00FF1BDE /* DidRemoveFrameFromHiearchyInPageCache.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AD57AC1F1DA7464D00FF1BDE /* DidRemoveFrameFromHiearchyInPageCache.cpp */; };
                AD57AC221DA7466E00FF1BDE /* many-iframes.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = AD57AC1D1DA7463800FF1BDE /* many-iframes.html */; };
                AD7C434D1DD2A54E0026888B /* Expected.cpp in Sources */ = {isa = PBXBuildFile; fileRef = AD7C434C1DD2A5470026888B /* Expected.cpp */; };
-               ADCEBBA61D9AE229002E283A /* Consume.cpp in Sources */ = {isa = PBXBuildFile; fileRef = ADCEBBA51D99D4CF002E283A /* Consume.cpp */; };
                B55AD1D5179F3B3000AC1494 /* PreventImageLoadWithAutoResizing_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = B55AD1D3179F3ABF00AC1494 /* PreventImageLoadWithAutoResizing_Bundle.cpp */; };
                B55F11B71517D03300915916 /* attributedStringCustomFont.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = B55F11B01517A2C400915916 /* attributedStringCustomFont.html */; };
                B55F11BE15191A0600915916 /* Ahem.ttf in Copy Resources */ = {isa = PBXBuildFile; fileRef = B55F11B9151916E600915916 /* Ahem.ttf */; };
                AD57AC1E1DA7464D00FF1BDE /* DidRemoveFrameFromHiearchyInPageCache_Bundle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DidRemoveFrameFromHiearchyInPageCache_Bundle.cpp; sourceTree = "<group>"; };
                AD57AC1F1DA7464D00FF1BDE /* DidRemoveFrameFromHiearchyInPageCache.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = DidRemoveFrameFromHiearchyInPageCache.cpp; sourceTree = "<group>"; };
                AD7C434C1DD2A5470026888B /* Expected.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Expected.cpp; sourceTree = "<group>"; };
-               ADCEBBA51D99D4CF002E283A /* Consume.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Consume.cpp; sourceTree = "<group>"; };
                B4039F9C15E6D8B3007255D6 /* MathExtras.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = MathExtras.cpp; sourceTree = "<group>"; };
                B55AD1D1179F336600AC1494 /* PreventImageLoadWithAutoResizing.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; name = PreventImageLoadWithAutoResizing.mm; path = WebKit2ObjC/PreventImageLoadWithAutoResizing.mm; sourceTree = "<group>"; };
                B55AD1D3179F3ABF00AC1494 /* PreventImageLoadWithAutoResizing_Bundle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = PreventImageLoadWithAutoResizing_Bundle.cpp; path = WebKit2ObjC/PreventImageLoadWithAutoResizing_Bundle.cpp; sourceTree = "<group>"; };
                                E40019301ACE9B5C001B0A2A /* BloomFilter.cpp */,
                                A7A966DA140ECCC8005EF9B4 /* CheckedArithmeticOperations.cpp */,
                                0FEAE3671B7D19CB00CE17F2 /* Condition.cpp */,
-                               ADCEBBA51D99D4CF002E283A /* Consume.cpp */,
                                51714EB91D087416004723C4 /* CrossThreadTask.cpp */,
                                26A2C72E15E2E73C005B1A14 /* CString.cpp */,
                                7AA021BA1AB09EA70052953F /* DateMath.cpp */,
                                7C83DEFE1D0A590C00FEBCF3 /* NakedPtr.cpp in Sources */,
                                531C1D8E1DF8EF72006E979F /* LEBDecoder.cpp in Sources */,
                                7C83DF011D0A590C00FEBCF3 /* Optional.cpp in Sources */,
-                               ADCEBBA61D9AE229002E283A /* Consume.cpp in Sources */,
                                AD7C434D1DD2A54E0026888B /* Expected.cpp in Sources */,
                                7C83DF021D0A590C00FEBCF3 /* OSObjectPtr.cpp in Sources */,
                                7C83DF591D0A590C00FEBCF3 /* ParkingLot.cpp in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WTF/Consume.cpp b/Tools/TestWebKitAPI/Tests/WTF/Consume.cpp
deleted file mode 100644 (file)
index e1a5d7a..0000000
+++ /dev/null
@@ -1,141 +0,0 @@
-/*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- *    notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- *    notice, this list of conditions and the following disclaimer in the
- *    documentation and/or other materials provided with the distribution.
- *
- * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
- * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
- * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
- * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
- * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
- * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
- * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-#include "config.h"
-
-#include <thread>
-#include <wtf/Atomics.h>
-
-template <typename T>
-NEVER_INLINE auto testConsume(const T* location)
-{
-    WTF::compilerFence(); // Paranoid testing.
-    auto ret = WTF::consumeLoad(location);
-    WTF::compilerFence(); // Paranoid testing.
-    return ret;
-}
-
-namespace TestWebKitAPI {
-
-TEST(WTF, Consumei8)
-{
-    uint8_t i8 = 42;
-    auto i8_consumed = testConsume(&i8);
-    ASSERT_EQ(i8_consumed.value, 42u);
-    ASSERT_EQ(i8_consumed.dependency, 0u);
-}
-
-TEST(WTF, Consumei16)
-{
-    uint16_t i16 = 42;
-    auto i16_consumed = testConsume(&i16);
-    ASSERT_EQ(i16_consumed.value, 42u);
-    ASSERT_EQ(i16_consumed.dependency, 0u);
-}
-
-TEST(WTF, Consumei32)
-{
-    uint32_t i32 = 42;
-    auto i32_consumed = testConsume(&i32);
-    ASSERT_EQ(i32_consumed.value, 42u);
-    ASSERT_EQ(i32_consumed.dependency, 0u);
-}
-
-TEST(WTF, Consumei64)
-{
-    uint64_t i64 = 42;
-    auto i64_consumed = testConsume(&i64);
-    ASSERT_EQ(i64_consumed.value, 42u);
-    ASSERT_EQ(i64_consumed.dependency, 0u);
-}
-
-TEST(WTF, Consumef32)
-{
-    float f32 = 42.f;
-    auto f32_consumed = testConsume(&f32);
-    ASSERT_EQ(f32_consumed.value, 42.f);
-    ASSERT_EQ(f32_consumed.dependency, 0u);
-}
-
-TEST(WTF, Consumef64)
-{
-    double f64 = 42.;
-    auto f64_consumed = testConsume(&f64);
-    ASSERT_EQ(f64_consumed.value, 42.);
-    ASSERT_EQ(f64_consumed.dependency, 0u);
-}
-
-static int* global;
-
-TEST(WTF, ConsumeGlobalPtr)
-{
-    auto* ptr = &global;
-    auto ptr_consumed = testConsume(&ptr);
-    ASSERT_EQ(ptr_consumed.value, &global);
-    ASSERT_EQ(ptr_consumed.dependency, 0u);
-}
-
-static int* globalArray[128];
-
-TEST(WTF, ConsumeGlobalArrayPtr)
-{
-    auto* ptr = &globalArray[64];
-    auto ptr_consumed = testConsume(&ptr);
-    ASSERT_EQ(ptr_consumed.value, &globalArray[64]);
-    ASSERT_EQ(ptr_consumed.dependency, 0u);
-}
-
-TEST(WTF, ConsumeStackPtr)
-{
-    char* hello = nullptr;
-    auto* stack = &hello;
-    auto stack_consumed = testConsume(&stack);
-    ASSERT_EQ(stack_consumed.value, &hello);
-    ASSERT_EQ(stack_consumed.dependency, 0u);
-}
-
-TEST(WTF, ConsumeWithThread)
-{
-    bool ready = false;
-    constexpr size_t num = 1024;
-    uint32_t* vec = new uint32_t[num];
-    std::thread t([&]() {
-        for (size_t i = 0; i != num; ++i)
-            vec[i] = i * 2;
-        WTF::storeStoreFence();
-        ready = true;
-    });
-    do {
-        auto stack_consumed = testConsume(&ready);
-        if (stack_consumed.value) {
-            for (size_t i = 0; i != num; ++i)
-                ASSERT_EQ(vec[i + stack_consumed.dependency], i * 2);
-            break;
-        }
-    } while (true);
-    t.join();
-    delete[] vec;
-}
-}