Changeset 181456 in webkit

Timestamp:

Mar 12, 2015, 3:14:32 PM (11 years ago)

Author:

mark.lam@apple.com

Message:

Use std::atomic for CodeBlock::m_visitAggregateHasBeenCalled.
<​https://webkit.org/b/142640>

Reviewed by Mark Hahnenberg.

We used to spin our own compare and swap on a uint8_t. Now that we can
use C++11, let's use std::atomic instead.

• bytecode/CodeBlock.cpp:

(JSC::CodeBlock::visitAggregate):

• The CAS here needs std::memory_order_acquire ordering because it requires lock acquisition semantics to visit the CodeBlock.

• bytecode/CodeBlock.h:

(JSC::CodeBlockSet::mark):

• heap/CodeBlockSet.cpp:

(JSC::CodeBlockSet::clearMarksForFullCollection):
(JSC::CodeBlockSet::clearMarksForEdenCollection):

• These can go with relaxed ordering because they are all done before the GC starts parallel marking.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/CodeBlock.cpp (modified) (1 diff)
• bytecode/CodeBlock.h (modified) (2 diffs)
• heap/CodeBlockSet.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-03-12  Mark Lam  <mark.lam@apple.com>
+
+        Use std::atomic for CodeBlock::m_visitAggregateHasBeenCalled.
+        <https://webkit.org/b/142640>
+
+        Reviewed by Mark Hahnenberg.
+
+        We used to spin our own compare and swap on a uint8_t.  Now that we can
+        use C++11, let's use std::atomic instead.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::visitAggregate):
+        - The CAS here needs std::memory_order_acquire ordering because it
+          requires lock acquisition semantics to visit the CodeBlock.
+
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlockSet::mark):
+        * heap/CodeBlockSet.cpp:
+        (JSC::CodeBlockSet::clearMarksForFullCollection):
+        (JSC::CodeBlockSet::clearMarksForEdenCollection):
+        - These can go with relaxed ordering because they are all done before
+          the GC starts parallel marking.
+
 2015-03-12  Csaba Osztrogonác  <ossy@webkit.org>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -2196 +2196 @@
 #if ENABLE(PARALLEL_GC)
     // I may be asked to scan myself more than once, and it may even happen concurrently.
-    // To this end, use a CAS loop to check if I've been called already. Only one thread
-    // may proceed past this point - whichever one wins the CAS race.
-    unsigned oldValue;
-    do {
-        oldValue = m_visitAggregateHasBeenCalled;
-        if (oldValue) {
-            // Looks like someone else won! Return immediately to ensure that we don't
-            // trace the same CodeBlock concurrently. Doing so is hazardous since we will
-            // be mutating the state of ValueProfiles, which contain JSValues, which can
-            // have word-tearing on 32-bit, leading to awesome timing-dependent crashes
-            // that are nearly impossible to track down.
-            
-            // Also note that it must be safe to return early as soon as we see the
-            // value true (well, (unsigned)1), since once a GC thread is in this method
-            // and has won the CAS race (i.e. was responsible for setting the value true)
-            // it will definitely complete the rest of this method before declaring
-            // termination.
-            return;
-        }
-    } while (!WTF::weakCompareAndSwap(&m_visitAggregateHasBeenCalled, 0, 1));
+    // To this end, use an atomic operation to check (and set) if I've been called already.
+    // Only one thread may proceed past this point - whichever one wins the atomic set race.
+    bool expected = false;
+    bool setByMe = m_visitAggregateHasBeenCalled.compare_exchange_strong(expected, true, std::memory_order_acquire);
+    if (!setByMe)
+        return;
 #endif // ENABLE(PARALLEL_GC)
     

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -1066 +1066 @@
     bool m_needsActivation;
     bool m_mayBeExecuting;
-    uint8_t m_visitAggregateHasBeenCalled;
+    std::atomic<bool> m_visitAggregateHasBeenCalled;
 
     RefPtr<SourceProvider> m_source;
@@ -1302 +1302 @@
     codeBlock->m_mayBeExecuting = true;
     // We might not have cleared the marks for this CodeBlock, but we need to visit it.
-    codeBlock->m_visitAggregateHasBeenCalled = false;
+    codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
 #if ENABLE(GGC)
     m_currentlyExecuting.append(codeBlock);

trunk/Source/JavaScriptCore/heap/CodeBlockSet.cpp

@@ -66 +66 @@
     for (CodeBlock* codeBlock : m_oldCodeBlocks) {
         codeBlock->m_mayBeExecuting = false;
-        codeBlock->m_visitAggregateHasBeenCalled = false;
+        codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
     }
 
@@ -83 +83 @@
         executable->forEachCodeBlock([](CodeBlock* codeBlock) {
             codeBlock->m_mayBeExecuting = false;
-            codeBlock->m_visitAggregateHasBeenCalled = false;
+            codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
         });
     }