Title: [270794] trunk/Source
Revision
270794
Author
tzaga...@apple.com
Date
2020-12-14 11:47:36 -0800 (Mon, 14 Dec 2020)

Log Message

Move some of the work from JSLock to VMEntryScope
https://bugs.webkit.org/show_bug.cgi?id=219830

Reviewed by Mark Lam.

Source/_javascript_Core:

We move several things from JSLock to VMEntryScope that could only be observed after we entered the VM:
- WasmThreads: only used when tiering up wasm, so VMEntryScope would have executed
- registerThreadForMachExceptionHandling: The mach exception handlers are used for:
    - sigill crash analyzer: only relevant after we enter the vm
    - wasm fault signal handler: same, we must be executing wasm and therefore VMEntryScope will have executed.
    - VMTraps: only handles faults in JIT code
- firePrimitiveGigacageEnabledIfNecessary: Only watched by the JITs

This gives is a ~10% improvement on APIBench (score change from ~36.3 to ~39.9), but as it turns out the most expensive
call is adding the current thread to the heap as this requires acquiring two locks. We can't  move this to VMEntryScope
since it's possible to use the API and GC without ever entering the VM, which would result in the current thread's stack
not being scanned. Instead, we just remember the last thread that acquired the lock and skip the call if we're seeing the
same thread again. This greatly amortizes the cost and gives us another ~10%:

CURRENT_API:        Baseline   Change
----------------------------------------
RichardsMostlyC:      62ms      32ms
RichardsMostlyObjC:  303ms     264ms
RichardsMostlySwift: 296ms     261ms
RichardsSomeC:        76ms      49ms
RichardsSomeObjC:    156ms     150ms
RichardsSomeSwift:   200ms     197ms

UPCOMING_API:       Baseline   Change
----------------------------------------
RichardsMostlyC:      19ms      19ms
RichardsMostlyObjC:  282ms     260ms
RichardsMostlySwift: 282ms     264ms
RichardsSomeC:        79ms      46ms
RichardsSomeObjC:    156ms     149ms
RichardsSomeSwift:   195ms     195ms
----------------------------------------
Score:             36.2211   43.3368

* runtime/JSLock.cpp:
(JSC::JSLock::didAcquireLock):
(JSC::JSLock::willReleaseLock):
* runtime/JSLock.h:
* runtime/VMEntryScope.cpp:
(JSC::VMEntryScope::VMEntryScope):
(JSC::VMEntryScope::~VMEntryScope):

Source/WTF:

Add a unique ID to threads. This uid is used in JSLock to avoid unnecessary work when the
same thread that last acquired the lock is reacquiring it.

* wtf/Threading.cpp:
* wtf/Threading.h:
(WTF::Thread::uid const):
(WTF::Thread::Thread):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (270793 => 270794)


--- trunk/Source/_javascript_Core/ChangeLog	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-12-14 19:47:36 UTC (rev 270794)
@@ -1,3 +1,52 @@
+2020-12-14  Tadeu Zagallo  <tzaga...@apple.com>
+
+        Move some of the work from JSLock to VMEntryScope
+        https://bugs.webkit.org/show_bug.cgi?id=219830
+
+        Reviewed by Mark Lam.
+
+        We move several things from JSLock to VMEntryScope that could only be observed after we entered the VM:
+        - WasmThreads: only used when tiering up wasm, so VMEntryScope would have executed
+        - registerThreadForMachExceptionHandling: The mach exception handlers are used for:
+            - sigill crash analyzer: only relevant after we enter the vm
+            - wasm fault signal handler: same, we must be executing wasm and therefore VMEntryScope will have executed.
+            - VMTraps: only handles faults in JIT code
+        - firePrimitiveGigacageEnabledIfNecessary: Only watched by the JITs
+
+        This gives is a ~10% improvement on APIBench (score change from ~36.3 to ~39.9), but as it turns out the most expensive
+        call is adding the current thread to the heap as this requires acquiring two locks. We can't  move this to VMEntryScope
+        since it's possible to use the API and GC without ever entering the VM, which would result in the current thread's stack
+        not being scanned. Instead, we just remember the last thread that acquired the lock and skip the call if we're seeing the
+        same thread again. This greatly amortizes the cost and gives us another ~10%:
+
+        CURRENT_API:        Baseline   Change
+        ----------------------------------------
+        RichardsMostlyC:      62ms      32ms
+        RichardsMostlyObjC:  303ms     264ms
+        RichardsMostlySwift: 296ms     261ms
+        RichardsSomeC:        76ms      49ms
+        RichardsSomeObjC:    156ms     150ms
+        RichardsSomeSwift:   200ms     197ms
+
+        UPCOMING_API:       Baseline   Change
+        ----------------------------------------
+        RichardsMostlyC:      19ms      19ms
+        RichardsMostlyObjC:  282ms     260ms
+        RichardsMostlySwift: 282ms     264ms
+        RichardsSomeC:        79ms      46ms
+        RichardsSomeObjC:    156ms     149ms
+        RichardsSomeSwift:   195ms     195ms
+        ----------------------------------------
+        Score:             36.2211   43.3368
+
+        * runtime/JSLock.cpp:
+        (JSC::JSLock::didAcquireLock):
+        (JSC::JSLock::willReleaseLock):
+        * runtime/JSLock.h:
+        * runtime/VMEntryScope.cpp:
+        (JSC::VMEntryScope::VMEntryScope):
+        (JSC::VMEntryScope::~VMEntryScope):
+
 2020-12-14  Robin Morisset  <rmoris...@apple.com>
 
         Minor cleanup of BigInts

Modified: trunk/Source/_javascript_Core/runtime/JSLock.cpp (270793 => 270794)


--- trunk/Source/_javascript_Core/runtime/JSLock.cpp	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/_javascript_Core/runtime/JSLock.cpp	2020-12-14 19:47:36 UTC (rev 270794)
@@ -25,8 +25,6 @@
 #include "JSGlobalObject.h"
 #include "MachineStackMarker.h"
 #include "SamplingProfiler.h"
-#include "WasmCapabilities.h"
-#include "WasmMachineThreads.h"
 #include <wtf/StackPointer.h>
 #include <wtf/Threading.h>
 #include <wtf/threads/Signals.h>
@@ -148,28 +146,23 @@
     void* p = currentStackPointer();
     m_vm->setStackPointerAtVMEntry(p);
 
-    if (m_vm->heap.machineThreads().addCurrentThread()) {
-        if (isKernTCSMAvailable())
-            enableKernTCSM();
+    if (thread.uid() != m_lastOwnerThread) {
+        m_lastOwnerThread = thread.uid();
+        if (m_vm->heap.machineThreads().addCurrentThread()) {
+            if (isKernTCSMAvailable())
+                enableKernTCSM();
+        }
     }
 
-#if ENABLE(WEBASSEMBLY)
-    if (Wasm::isSupported())
-        Wasm::startTrackingCurrentThread();
-#endif
-
-#if HAVE(MACH_EXCEPTIONS)
-    registerThreadForMachExceptionHandling(Thread::current());
-#endif
-
     // Note: everything below must come after addCurrentThread().
     m_vm->traps().notifyGrabAllLocks();
-    
-    m_vm->firePrimitiveGigacageEnabledIfNecessary();
 
 #if ENABLE(SAMPLING_PROFILER)
-    if (SamplingProfiler* samplingProfiler = m_vm->samplingProfiler())
-        samplingProfiler->noticeJSLockAcquisition();
+    {
+        SamplingProfiler* samplingProfiler = m_vm->samplingProfiler();
+        if (UNLIKELY(samplingProfiler))
+            samplingProfiler->noticeJSLockAcquisition();
+    }
 #endif
 }
 

Modified: trunk/Source/_javascript_Core/runtime/JSLock.h (270793 => 270794)


--- trunk/Source/_javascript_Core/runtime/JSLock.h	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/_javascript_Core/runtime/JSLock.h	2020-12-14 19:47:36 UTC (rev 270794)
@@ -145,10 +145,11 @@
     // different thread, and an optional is vulnerable to races.
     // See https://bugs.webkit.org/show_bug.cgi?id=169042#c6
     bool m_hasOwnerThread { false };
+    bool m_shouldReleaseHeapAccess;
     RefPtr<Thread> m_ownerThread;
     intptr_t m_lockCount;
     unsigned m_lockDropDepth;
-    bool m_shouldReleaseHeapAccess;
+    uint32_t m_lastOwnerThread { 0 };
     VM* m_vm;
     AtomStringTable* m_entryAtomStringTable; 
 };

Modified: trunk/Source/_javascript_Core/runtime/VMEntryScope.cpp (270793 => 270794)


--- trunk/Source/_javascript_Core/runtime/VMEntryScope.cpp	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/_javascript_Core/runtime/VMEntryScope.cpp	2020-12-14 19:47:36 UTC (rev 270794)
@@ -29,6 +29,8 @@
 #include "Options.h"
 #include "SamplingProfiler.h"
 #include "VM.h"
+#include "WasmCapabilities.h"
+#include "WasmMachineThreads.h"
 #include "Watchdog.h"
 #include <wtf/SystemTracing.h>
 
@@ -41,6 +43,17 @@
     if (!vm.entryScope) {
         vm.entryScope = this;
 
+#if ENABLE(WEBASSEMBLY)
+        if (Wasm::isSupported())
+            Wasm::startTrackingCurrentThread();
+#endif
+
+#if HAVE(MACH_EXCEPTIONS)
+        registerThreadForMachExceptionHandling(Thread::current());
+#endif
+
+        vm.firePrimitiveGigacageEnabledIfNecessary();
+
         // Reset the date cache between JS invocations to force the VM to
         // observe time zone changes.
         // FIXME: We should clear it only when we know the timezone has been changed.
@@ -51,10 +64,13 @@
             vm.watchdog()->enteredVM();
 
 #if ENABLE(SAMPLING_PROFILER)
-        if (SamplingProfiler* samplingProfiler = vm.samplingProfiler())
-            samplingProfiler->noticeVMEntry();
+        {
+            SamplingProfiler* samplingProfiler = vm.samplingProfiler();
+            if (UNLIKELY(samplingProfiler))
+                samplingProfiler->noticeVMEntry();
+        }
 #endif
-        if (Options::useTracePoints())
+        if (UNLIKELY(Options::useTracePoints()))
             tracePoint(VMEntryScopeStart);
     }
 

Modified: trunk/Source/WTF/ChangeLog (270793 => 270794)


--- trunk/Source/WTF/ChangeLog	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/WTF/ChangeLog	2020-12-14 19:47:36 UTC (rev 270794)
@@ -1,3 +1,18 @@
+2020-12-14  Tadeu Zagallo  <tzaga...@apple.com>
+
+        Move some of the work from JSLock to VMEntryScope
+        https://bugs.webkit.org/show_bug.cgi?id=219830
+
+        Reviewed by Mark Lam.
+
+        Add a unique ID to threads. This uid is used in JSLock to avoid unnecessary work when the
+        same thread that last acquired the lock is reacquiring it.
+
+        * wtf/Threading.cpp:
+        * wtf/Threading.h:
+        (WTF::Thread::uid const):
+        (WTF::Thread::Thread):
+
 2020-12-13  Yusuke Suzuki  <ysuz...@apple.com>
 
         [JSC] Introduce vmEntryCustomAccessor and vmEntryHostFunction for JITCage

Modified: trunk/Source/WTF/wtf/Threading.cpp (270793 => 270794)


--- trunk/Source/WTF/wtf/Threading.cpp	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/WTF/wtf/Threading.cpp	2020-12-14 19:47:36 UTC (rev 270794)
@@ -64,6 +64,8 @@
 #endif
 }
 
+std::atomic<uint32_t> Thread::s_uid { 0 };
+
 struct Thread::NewThreadContext : public ThreadSafeRefCounted<NewThreadContext> {
 public:
     NewThreadContext(const char* name, Function<void()>&& entryPoint, Ref<Thread>&& thread)

Modified: trunk/Source/WTF/wtf/Threading.h (270793 => 270794)


--- trunk/Source/WTF/wtf/Threading.h	2020-12-14 19:39:53 UTC (rev 270793)
+++ trunk/Source/WTF/wtf/Threading.h	2020-12-14 19:47:36 UTC (rev 270794)
@@ -92,6 +92,7 @@
 };
 
 class Thread : public ThreadSafeRefCounted<Thread> {
+    static std::atomic<uint32_t> s_uid;
 public:
     friend class ThreadGroup;
     friend WTF_EXPORT_PRIVATE void initialize();
@@ -111,6 +112,8 @@
 
     WTF_EXPORT_PRIVATE unsigned numberOfThreadGroups();
 
+    uint32_t uid() const { return m_uid; }
+
 #if OS(WINDOWS)
     // Returns ThreadIdentifier directly. It is useful if the user only cares about identity
     // of threads. At that time, users should know that holding this ThreadIdentifier does not ensure
@@ -328,6 +331,7 @@
     StackBounds m_stack { StackBounds::emptyBounds() };
     HashMap<ThreadGroup*, std::weak_ptr<ThreadGroup>> m_threadGroupMap;
     PlatformThreadHandle m_handle;
+    uint32_t m_uid;
 #if OS(WINDOWS)
     ThreadIdentifier m_id { 0 };
 #elif OS(DARWIN)
@@ -362,6 +366,7 @@
     , m_isDestroyedOnce(false)
     , m_isCompilationThread(false)
     , m_gcThreadType(static_cast<unsigned>(GCThreadType::None))
+    , m_uid(++s_uid)
 {
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to