[JSC] Less contended MetaAllocator
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Aug 2019 20:33:14 +0000 (20:33 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Aug 2019 20:33:14 +0000 (20:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200278

Reviewed by Mark Lam.

Source/JavaScriptCore:

The profiler result of JetStream2/bomb-workers shows that we are having contention under MetaAllocator::currentStatistics.
This function is called in ExecutableAllocator::memoryPressureMultiplier, and it is called from ExecutableCounter's threshold
calculation. But MetaAllocator::currentStatistics takes a global lock inside MetaAllocator and causes contention. However,
we do not need to have a lock actually: clients of MetaAllocator::currentStatistics typically use bytesReserved and bytesAllocated
information. However, since our executable allocator is fixed-sized, bytesReserved is always the fixed size. So just reading bytesAllocated
racily is enough.

This patch attempts to reduce the contention by the following two things.

1. Read bytesAllocated racily instead of calling MetaAllocator::currentStatistics. Then ExecutableCounter does not need to take a lock.
2. page lifetime management APIs of MetaAllocator should take a second `count` parameter to batch the system calls.

* jit/ExecutableAllocator.cpp:
(JSC::ExecutableAllocator::underMemoryPressure):
(JSC::ExecutableAllocator::memoryPressureMultiplier):
(JSC::ExecutableAllocator::allocate):
(JSC::FixedVMPoolExecutableAllocator::FixedVMPoolExecutableAllocator): Deleted.
(JSC::FixedVMPoolExecutableAllocator::memoryStart): Deleted.
(JSC::FixedVMPoolExecutableAllocator::memoryEnd): Deleted.
(JSC::FixedVMPoolExecutableAllocator::isJITPC): Deleted.
(JSC::FixedVMPoolExecutableAllocator::initializeSeparatedWXHeaps): Deleted.
(JSC::FixedVMPoolExecutableAllocator::jitWriteThunkGenerator): Deleted.
(JSC::FixedVMPoolExecutableAllocator::genericWriteToJITRegion): Deleted.

Source/WTF:

* wtf/MetaAllocator.cpp:
(WTF::MetaAllocator::incrementPageOccupancy):
(WTF::MetaAllocator::decrementPageOccupancy):
* wtf/MetaAllocator.h:

Tools:

Update the interface.

* TestWebKitAPI/Tests/WTF/MetaAllocator.cpp:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/ExecutableAllocator.cpp
Source/WTF/ChangeLog
Source/WTF/wtf/MetaAllocator.cpp
Source/WTF/wtf/MetaAllocator.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/MetaAllocator.cpp

index 530036f..f8e30cf 100644 (file)
@@ -1,5 +1,36 @@
 2019-08-14  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] Less contended MetaAllocator
+        https://bugs.webkit.org/show_bug.cgi?id=200278
+
+        Reviewed by Mark Lam.
+
+        The profiler result of JetStream2/bomb-workers shows that we are having contention under MetaAllocator::currentStatistics.
+        This function is called in ExecutableAllocator::memoryPressureMultiplier, and it is called from ExecutableCounter's threshold
+        calculation. But MetaAllocator::currentStatistics takes a global lock inside MetaAllocator and causes contention. However,
+        we do not need to have a lock actually: clients of MetaAllocator::currentStatistics typically use bytesReserved and bytesAllocated
+        information. However, since our executable allocator is fixed-sized, bytesReserved is always the fixed size. So just reading bytesAllocated
+        racily is enough.
+
+        This patch attempts to reduce the contention by the following two things.
+
+        1. Read bytesAllocated racily instead of calling MetaAllocator::currentStatistics. Then ExecutableCounter does not need to take a lock.
+        2. page lifetime management APIs of MetaAllocator should take a second `count` parameter to batch the system calls.
+
+        * jit/ExecutableAllocator.cpp:
+        (JSC::ExecutableAllocator::underMemoryPressure):
+        (JSC::ExecutableAllocator::memoryPressureMultiplier):
+        (JSC::ExecutableAllocator::allocate):
+        (JSC::FixedVMPoolExecutableAllocator::FixedVMPoolExecutableAllocator): Deleted.
+        (JSC::FixedVMPoolExecutableAllocator::memoryStart): Deleted.
+        (JSC::FixedVMPoolExecutableAllocator::memoryEnd): Deleted.
+        (JSC::FixedVMPoolExecutableAllocator::isJITPC): Deleted.
+        (JSC::FixedVMPoolExecutableAllocator::initializeSeparatedWXHeaps): Deleted.
+        (JSC::FixedVMPoolExecutableAllocator::jitWriteThunkGenerator): Deleted.
+        (JSC::FixedVMPoolExecutableAllocator::genericWriteToJITRegion): Deleted.
+
+2019-08-14  Yusuke Suzuki  <ysuzuki@apple.com>
+
         [JSC] Make PAC jump and return more explicit
         https://bugs.webkit.org/show_bug.cgi?id=200703
 
index b11de5e..36d7ccc 100644 (file)
@@ -154,7 +154,7 @@ void ExecutableAllocator::setJITEnabled(bool enabled)
 #endif
 }
 
-class FixedVMPoolExecutableAllocator : public MetaAllocator {
+class FixedVMPoolExecutableAllocator final : public MetaAllocator {
     WTF_MAKE_FAST_ALLOCATED;
 public:
     FixedVMPoolExecutableAllocator()
@@ -208,6 +208,8 @@ public:
 
             addFreshFreeSpace(reservationBase, reservationSize);
 
+            ASSERT(bytesReserved() == reservationSize); // Since our executable memory is fixed-sized, bytesReserved is never changed after initialization.
+
             void* reservationEnd = reinterpret_cast<uint8_t*>(reservationBase) + reservationSize;
 
             m_memoryStart = MacroAssemblerCodePtr<ExecutableMemoryPtrTag>(tagCodePtr<ExecutableMemoryPtrTag>(reservationBase));
@@ -228,20 +230,21 @@ protected:
         return nullptr;
     }
 
-    void notifyNeedPage(void* page) override
+    void notifyNeedPage(void* page, size_t count) override
     {
 #if USE(MADV_FREE_FOR_JIT_MEMORY)
         UNUSED_PARAM(page);
+        UNUSED_PARAM(count);
 #else
-        m_reservation.commit(page, pageSize());
+        m_reservation.commit(page, pageSize() * count);
 #endif
     }
 
-    void notifyPageIsFree(void* page) override
+    void notifyPageIsFree(void* page, size_t count) override
     {
 #if USE(MADV_FREE_FOR_JIT_MEMORY)
         for (;;) {
-            int result = madvise(page, pageSize(), MADV_FREE);
+            int result = madvise(page, pageSize() * count, MADV_FREE);
             if (!result)
                 return;
             ASSERT(result == -1);
@@ -251,7 +254,7 @@ protected:
             }
         }
 #else
-        m_reservation.decommit(page, pageSize());
+        m_reservation.decommit(page, pageSize() * count);
 #endif
     }
 
@@ -431,19 +434,17 @@ bool ExecutableAllocator::underMemoryPressure()
 {
     if (!allocator)
         return Base::underMemoryPressure();
-    MetaAllocator::Statistics statistics = allocator->currentStatistics();
-    return statistics.bytesAllocated > statistics.bytesReserved / 2;
+    return allocator->bytesAllocated() > allocator->bytesReserved() / 2;
 }
 
 double ExecutableAllocator::memoryPressureMultiplier(size_t addedMemoryUsage)
 {
     if (!allocator)
         return Base::memoryPressureMultiplier(addedMemoryUsage);
-    MetaAllocator::Statistics statistics = allocator->currentStatistics();
-    ASSERT(statistics.bytesAllocated <= statistics.bytesReserved);
-    size_t bytesAllocated = statistics.bytesAllocated + addedMemoryUsage;
+    ASSERT(allocator->bytesAllocated() <= allocator->bytesReserved());
+    size_t bytesAllocated = allocator->bytesAllocated() + addedMemoryUsage;
     size_t bytesAvailable = static_cast<size_t>(
-        statistics.bytesReserved * (1 - executablePoolReservationFraction));
+        allocator->bytesReserved() * (1 - executablePoolReservationFraction));
     if (bytesAllocated >= bytesAvailable)
         bytesAllocated = bytesAvailable;
     double result = 1.0;
@@ -475,10 +476,9 @@ RefPtr<ExecutableMemoryHandle> ExecutableAllocator::allocate(size_t sizeInBytes,
 
     if (effort == JITCompilationCanFail) {
         // Don't allow allocations if we are down to reserve.
-        MetaAllocator::Statistics statistics = allocator->currentStatistics();
-        size_t bytesAllocated = statistics.bytesAllocated + sizeInBytes;
+        size_t bytesAllocated = allocator->bytesAllocated() + sizeInBytes;
         size_t bytesAvailable = static_cast<size_t>(
-            statistics.bytesReserved * (1 - executablePoolReservationFraction));
+            allocator->bytesReserved() * (1 - executablePoolReservationFraction));
         if (bytesAllocated > bytesAvailable) {
             if (Options::logExecutableAllocation())
                 dataLog("Allocation failed because bytes allocated ", bytesAllocated,  " > ", bytesAvailable, " bytes available.\n");
index 4d0dfb8..83ebc49 100644 (file)
@@ -1,3 +1,15 @@
+2019-08-14  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Less contended MetaAllocator
+        https://bugs.webkit.org/show_bug.cgi?id=200278
+
+        Reviewed by Mark Lam.
+
+        * wtf/MetaAllocator.cpp:
+        (WTF::MetaAllocator::incrementPageOccupancy):
+        (WTF::MetaAllocator::decrementPageOccupancy):
+        * wtf/MetaAllocator.h:
+
 2019-08-14  Samuel GroƟ  <saelo@google.com>
 
         [JSCOnly] JSCOnly port doesn't build on macOS
index c3b6937..11f3cbc 100644 (file)
@@ -397,22 +397,46 @@ void MetaAllocator::incrementPageOccupancy(void* address, size_t sizeInBytes)
 {
     uintptr_t firstPage = reinterpret_cast<uintptr_t>(address) >> m_logPageSize;
     uintptr_t lastPage = (reinterpret_cast<uintptr_t>(address) + sizeInBytes - 1) >> m_logPageSize;
+
+    uintptr_t currentPageStart = 0;
+    size_t count = 0;
+    auto flushNeedPages = [&] {
+        if (!currentPageStart)
+            return;
+        notifyNeedPage(reinterpret_cast<void*>(currentPageStart << m_logPageSize), count);
+        currentPageStart = 0;
+        count = 0;
+    };
     
     for (uintptr_t page = firstPage; page <= lastPage; ++page) {
-        HashMap<uintptr_t, size_t>::iterator iter = m_pageOccupancyMap.find(page);
-        if (iter == m_pageOccupancyMap.end()) {
-            m_pageOccupancyMap.add(page, 1);
+        auto result = m_pageOccupancyMap.add(page, 1);
+        if (result.isNewEntry) {
             m_bytesCommitted += m_pageSize;
-            notifyNeedPage(reinterpret_cast<void*>(page << m_logPageSize));
-        } else
-            iter->value++;
+            if (!currentPageStart)
+                currentPageStart = page;
+            ++count;
+        } else {
+            result.iterator->value++;
+            flushNeedPages();
+        }
     }
+    flushNeedPages();
 }
 
 void MetaAllocator::decrementPageOccupancy(void* address, size_t sizeInBytes)
 {
     uintptr_t firstPage = reinterpret_cast<uintptr_t>(address) >> m_logPageSize;
     uintptr_t lastPage = (reinterpret_cast<uintptr_t>(address) + sizeInBytes - 1) >> m_logPageSize;
+
+    uintptr_t currentPageStart = 0;
+    size_t count = 0;
+    auto flushFreePages = [&] {
+        if (!currentPageStart)
+            return;
+        notifyPageIsFree(reinterpret_cast<void*>(currentPageStart << m_logPageSize), count);
+        currentPageStart = 0;
+        count = 0;
+    };
     
     for (uintptr_t page = firstPage; page <= lastPage; ++page) {
         HashMap<uintptr_t, size_t>::iterator iter = m_pageOccupancyMap.find(page);
@@ -420,9 +444,13 @@ void MetaAllocator::decrementPageOccupancy(void* address, size_t sizeInBytes)
         if (!--(iter->value)) {
             m_pageOccupancyMap.remove(iter);
             m_bytesCommitted -= m_pageSize;
-            notifyPageIsFree(reinterpret_cast<void*>(page << m_logPageSize));
-        }
+            if (!currentPageStart)
+                currentPageStart = page;
+            ++count;
+        } else
+            flushFreePages();
     }
+    flushFreePages();
 }
 
 bool MetaAllocator::isInAllocatedMemory(const AbstractLocker&, void* address)
index d8885d9..2dceb8e 100644 (file)
@@ -114,10 +114,10 @@ protected:
     virtual FreeSpacePtr allocateNewSpace(size_t& numPages) = 0;
 
     // Commit a page.
-    virtual void notifyNeedPage(void* page) = 0;
+    virtual void notifyNeedPage(void* page, size_t) = 0;
     
     // Uncommit a page.
-    virtual void notifyPageIsFree(void* page) = 0;
+    virtual void notifyPageIsFree(void* page, size_t) = 0;
     
     // NOTE: none of the above methods are called during allocator
     // destruction, in part because a MetaAllocator cannot die so long
index 459f900..04afc8b 100644 (file)
@@ -1,3 +1,14 @@
+2019-08-14  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Less contended MetaAllocator
+        https://bugs.webkit.org/show_bug.cgi?id=200278
+
+        Reviewed by Mark Lam.
+
+        Update the interface.
+
+        * TestWebKitAPI/Tests/WTF/MetaAllocator.cpp:
+
 2019-08-14  Jonathan Bedard  <jbedard@apple.com>
 
         results.webkit.org: 500 errors on API endpoints don't return JSON
index ef01b3a..232f5e3 100644 (file)
@@ -105,23 +105,27 @@ public:
             }
         }
         
-        virtual void notifyNeedPage(void* page)
+        virtual void notifyNeedPage(void* page, size_t count)
         {
             // the page should be both free and unmapped.
-            EXPECT_TRUE(!m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize()));
-            for (uintptr_t address = reinterpret_cast<uintptr_t>(page); address < reinterpret_cast<uintptr_t>(page) + pageSize(); ++address)
+            for (size_t i = 0; i < count; ++i)
+                EXPECT_TRUE(!m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize() + i));
+            for (uintptr_t address = reinterpret_cast<uintptr_t>(page); address < reinterpret_cast<uintptr_t>(page) + pageSize() * count; ++address)
                 EXPECT_TRUE(!m_parent->byteState(reinterpret_cast<void*>(address)));
-            m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize()) = true;
+            for (size_t i = 0; i < count; ++i)
+                m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize() + i) = true;
         }
         
-        virtual void notifyPageIsFree(void* page)
+        virtual void notifyPageIsFree(void* page, size_t count)
         {
             // the page should be free of objects at this point, but it should still
             // be mapped.
-            EXPECT_TRUE(m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize()));
-            for (uintptr_t address = reinterpret_cast<uintptr_t>(page); address < reinterpret_cast<uintptr_t>(page) + pageSize(); ++address)
+            for (size_t i = 0; i < count; ++i)
+                EXPECT_TRUE(m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize() + i));
+            for (uintptr_t address = reinterpret_cast<uintptr_t>(page); address < reinterpret_cast<uintptr_t>(page) + pageSize() * count; ++address)
                 EXPECT_TRUE(!m_parent->byteState(reinterpret_cast<void*>(address)));
-            m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize()) = false;
+            for (size_t i = 0; i < count; ++i)
+                m_parent->pageState(reinterpret_cast<uintptr_t>(page) / pageSize() + i) = false;
         }
         
     private: