WebAssembly: Don't tier up the same function twice

kmiller68 · kmiller68 · commit 2c6dd13653d8 · 2017-04-27T22:39:17.000Z
https://bugs.webkit.org/show_bug.cgi?id=171397 Reviewed by Filip Pizlo. Because we don't CAS the tier up count on function entry/loop backedge and we use the least significant to indicate whether or not tier up has already started we could see the following: Threads A and B are running count in memory is (0): A: load tier up count (0) B: load tier up count (0) A: decrement count to -2 and see we need to check for tier up (0) A: store -2 to count (-2) A: exchangeOr(1) to tier up count (-1) B: decrement count to -2 and see we need to check for tier up (-1) B: store -2 to count (-2) B: exchangeOr(1) to tier up count (-1) This would cause us to tier up the same function twice, which we would rather avoid. * wasm/WasmB3IRGenerator.cpp: (JSC::Wasm::B3IRGenerator::emitTierUpCheck): * wasm/WasmTierUpCount.h: (JSC::Wasm::TierUpCount::TierUpCount): (JSC::Wasm::TierUpCount::loopDecrement): (JSC::Wasm::TierUpCount::functionEntryDecrement): (JSC::Wasm::TierUpCount::shouldStartTierUp): Canonical link: https://commits.webkit.org/188292@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@215898 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,33 @@
+2017-04-27  Keith Miller  <keith_miller@apple.com>
+
+        WebAssembly: Don't tier up the same function twice
+        https://bugs.webkit.org/show_bug.cgi?id=171397
+
+        Reviewed by Filip Pizlo.
+
+        Because we don't CAS the tier up count on function entry/loop backedge and we use the least significant to indicate whether or not tier up has already started we could see the following:
+
+        Threads A and B are running count in memory is (0):
+
+        A: load tier up count (0)
+        B: load tier up count (0)
+        A: decrement count to -2 and see we need to check for tier up (0)
+        A: store -2 to count (-2)
+        A: exchangeOr(1) to tier up count (-1)
+        B: decrement count to -2 and see we need to check for tier up (-1)
+        B: store -2 to count (-2)
+        B: exchangeOr(1) to tier up count (-1)
+
+        This would cause us to tier up the same function twice, which we would rather avoid.
+
+        * wasm/WasmB3IRGenerator.cpp:
+        (JSC::Wasm::B3IRGenerator::emitTierUpCheck):
+        * wasm/WasmTierUpCount.h:
+        (JSC::Wasm::TierUpCount::TierUpCount):
+        (JSC::Wasm::TierUpCount::loopDecrement):
+        (JSC::Wasm::TierUpCount::functionEntryDecrement):
+        (JSC::Wasm::TierUpCount::shouldStartTierUp):
+
 2017-04-27  Keith Miller  <keith_miller@apple.com>
 
         REGRESSION (r215843): ASSERTION FAILED: !m_completionTasks[0].first in  JSC::Wasm::Plan::tryRemoveVMAndCancelIfLast(JSC::VM &)
diff --git a/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp b/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
@@ -834,7 +834,6 @@ BasicBlock* B3IRGenerator::emitTierUpCheck(BasicBlock* entry, uint32_t decrement
     if (!m_tierUp)
         return entry;
 
-    ASSERT(!(decrementCount % 2));
     // FIXME: Make this a patchpoint.
     BasicBlock* continuation = m_proc.addBlock();
 
diff --git a/Source/JavaScriptCore/wasm/WasmTierUpCount.h b/Source/JavaScriptCore/wasm/WasmTierUpCount.h
@@ -42,28 +42,30 @@ class TierUpCount {
     WTF_MAKE_NONCOPYABLE(TierUpCount);
 public:
     TierUpCount()
-        : m_count(Options::webAssemblyOMGTierUpCount() * 2)
+        : m_count(Options::webAssemblyOMGTierUpCount())
+        , m_tierUpStarted(false)
     {
     }
 
     TierUpCount(TierUpCount&& other)
     {
-        ASSERT(other.m_count == Options::webAssemblyOMGTierUpCount() * 2);
+        ASSERT(other.m_count == Options::webAssemblyOMGTierUpCount());
         m_count = other.m_count;
     }
 
-    static uint32_t loopDecrement() { return Options::webAssemblyLoopDecrement() * 2; }
-    static uint32_t functionEntryDecrement() { return Options::webAssemblyFunctionEntryDecrement() * 2; }
+    static uint32_t loopDecrement() { return Options::webAssemblyLoopDecrement(); }
+    static uint32_t functionEntryDecrement() { return Options::webAssemblyFunctionEntryDecrement(); }
 
     bool shouldStartTierUp()
     {
-        return !(WTF::atomicExchangeOr(&m_count, 1) & 1);
+        return !m_tierUpStarted.exchange(true);
     }
 
     int32_t count() { return bitwise_cast<int32_t>(m_count); }
 
 private:
     uint32_t m_count;
+    Atomic<bool> m_tierUpStarted;
 };
     
 } } // namespace JSC::Wasm

Original file line number	Diff line number	Diff line change
`@@ -42,28 +42,30 @@ class TierUpCount {`
`42`	`42`	`WTF_MAKE_NONCOPYABLE(TierUpCount);`
`43`	`43`	`public:`
`44`	`44`	`TierUpCount()`
`45`		`- : m_count(Options::webAssemblyOMGTierUpCount() * 2)`
	`45`	`+ : m_count(Options::webAssemblyOMGTierUpCount())`
	`46`	`+ , m_tierUpStarted(false)`
`46`	`47`	`{`
`47`	`48`	`}`
`48`	`49`
`49`	`50`	`TierUpCount(TierUpCount&& other)`
`50`	`51`	`{`
`51`		`- ASSERT(other.m_count == Options::webAssemblyOMGTierUpCount() * 2);`
	`52`	`+ ASSERT(other.m_count == Options::webAssemblyOMGTierUpCount());`
`52`	`53`	`m_count = other.m_count;`
`53`	`54`	`}`
`54`	`55`
`55`		`- static uint32_t loopDecrement() { return Options::webAssemblyLoopDecrement() * 2; }`
`56`		`- static uint32_t functionEntryDecrement() { return Options::webAssemblyFunctionEntryDecrement() * 2; }`
	`56`	`+ static uint32_t loopDecrement() { return Options::webAssemblyLoopDecrement(); }`
	`57`	`+ static uint32_t functionEntryDecrement() { return Options::webAssemblyFunctionEntryDecrement(); }`
`57`	`58`
`58`	`59`	`bool shouldStartTierUp()`
`59`	`60`	`{`
`60`		`- return !(WTF::atomicExchangeOr(&m_count, 1) & 1);`
	`61`	`+ return !m_tierUpStarted.exchange(true);`
`61`	`62`	`}`
`62`	`63`
`63`	`64`	`int32_t count() { return bitwise_cast<int32_t>(m_count); }`
`64`	`65`
`65`	`66`	`private:`
`66`	`67`	`uint32_t m_count;`
	`68`	`+ Atomic<bool> m_tierUpStarted;`
`67`	`69`	`};`
`68`	`70`
`69`	`71`	`} } // namespace JSC::Wasm`