Changeset 277475 in webkit

Timestamp:

May 13, 2021, 7:03:43 PM (5 years ago)

Author:

ggaren@apple.com

Message:

m_calleeSaveRegisters should not be a pointer to a pointer
​https://bugs.webkit.org/show_bug.cgi?id=225787

Reviewed by Keith Miller.

Ben found this through memory stress testing.

RegisterAtOffsetList is effectively just a pointer. unique_ptr<RegisterAtOffsetList>
is a pointer to a pointer. RegisterAtOffsetList is long-lived, so it
creates heap page fragmentation.

Worth 3MB on Ben's test.

• bytecode/CodeBlock.cpp:

(JSC::CodeBlock::setCalleeSaveRegisters):
(JSC::CodeBlock::calleeSaveRegisters const): Use a fence before setting
m_hasCalleeSaveRegisters to ensure that all writes have completed before
the struct becomes visible.

• bytecode/CodeBlock.h: Use RegisterAtOffsetList directly instead of

unique_ptr<RegisterAtOffsetList> to avoid a long-lived lonely 8 byte
allocation.

• ftl/FTLCompile.cpp:

(JSC::FTL::compile): Updated for type change.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/CodeBlock.cpp (modified) (2 diffs)
• bytecode/CodeBlock.h (modified) (3 diffs)
• ftl/FTLCompile.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-05-13  Geoffrey Garen  <ggaren@apple.com>
+
+        m_calleeSaveRegisters should not be a pointer to a pointer
+        https://bugs.webkit.org/show_bug.cgi?id=225787
+
+        Reviewed by Keith Miller.
+
+        Ben found this through memory stress testing.
+
+        RegisterAtOffsetList is effectively just a pointer. unique_ptr<RegisterAtOffsetList>
+        is a pointer to a pointer. RegisterAtOffsetList is long-lived, so it
+        creates heap page fragmentation.
+
+        Worth 3MB on Ben's test.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::setCalleeSaveRegisters):
+        (JSC::CodeBlock::calleeSaveRegisters const): Use a fence before setting
+        m_hasCalleeSaveRegisters to ensure that all writes have completed before
+        the struct becomes visible.
+
+        * bytecode/CodeBlock.h: Use RegisterAtOffsetList directly instead of 
+        unique_ptr<RegisterAtOffsetList> to avoid a long-lived lonely 8 byte
+        allocation.
+
+        * ftl/FTLCompile.cpp:
+        (JSC::FTL::compile): Updated for type change.
+
 2021-05-13  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -1791 +1791 @@
 }
 
-void CodeBlock::setCalleeSaveRegisters(RegisterSet calleeSaveRegisters)
-{
+void CodeBlock::setCalleeSaveRegisters(RegisterSet registerSet)
+{
+    auto calleeSaveRegisters = RegisterAtOffsetList(registerSet);
+
     ConcurrentJSLocker locker(m_lock);
-    ensureJITData(locker).m_calleeSaveRegisters = makeUnique<RegisterAtOffsetList>(calleeSaveRegisters);
-}
-
-void CodeBlock::setCalleeSaveRegisters(std::unique_ptr<RegisterAtOffsetList> registerAtOffsetList)
+    auto& jitData = ensureJITData(locker);
+    jitData.m_calleeSaveRegisters = WTFMove(calleeSaveRegisters);
+    WTF::storeStoreFence();
+    jitData.m_hasCalleeSaveRegisters = true;
+}
+
+void CodeBlock::setCalleeSaveRegisters(RegisterAtOffsetList&& registerAtOffsetList)
 {
     ConcurrentJSLocker locker(m_lock);
-    ensureJITData(locker).m_calleeSaveRegisters = WTFMove(registerAtOffsetList);
+    auto& jitData = ensureJITData(locker);
+    jitData.m_calleeSaveRegisters = WTFMove(registerAtOffsetList);
+    WTF::storeStoreFence();
+    jitData.m_hasCalleeSaveRegisters = true;
 }
 
@@ -2493 +2501 @@
 #if ENABLE(JIT)
     if (auto* jitData = m_jitData.get()) {
-        if (const RegisterAtOffsetList* registers = jitData->m_calleeSaveRegisters.get())
-            return registers;
+        if (jitData->m_hasCalleeSaveRegisters)
+            return &jitData->m_calleeSaveRegisters;
     }
 #endif

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -65 +65 @@
 #include "ProgramExecutable.h"
 #include "PutPropertySlot.h"
+#include "RegisterAtOffsetList.h"
 #include "ValueProfile.h"
 #include "VirtualRegister.h"
@@ -284 +285 @@
         FixedVector<StringJumpTable> m_stringSwitchJumpTables;
         std::unique_ptr<PCToCodeOriginMap> m_pcToCodeOriginMap;
-        std::unique_ptr<RegisterAtOffsetList> m_calleeSaveRegisters;
+        bool m_hasCalleeSaveRegisters { false };
+        RegisterAtOffsetList m_calleeSaveRegisters;
         JITCodeMap m_jitCodeMap;
     };
@@ -344 +346 @@
 
     void setCalleeSaveRegisters(RegisterSet);
-    void setCalleeSaveRegisters(std::unique_ptr<RegisterAtOffsetList>);
+    void setCalleeSaveRegisters(RegisterAtOffsetList&&);
 
     void setRareCaseProfiles(FixedVector<RareCaseProfile>&&);

trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp

@@ -72 +72 @@
         return;
     
-    std::unique_ptr<RegisterAtOffsetList> registerOffsets =
-        makeUnique<RegisterAtOffsetList>(state.proc->calleeSaveRegisterAtOffsetList());
+    RegisterAtOffsetList registerOffsets = state.proc->calleeSaveRegisterAtOffsetList();
     if (shouldDumpDisassembly())
-        dataLog(tierName, "Unwind info for ", CodeBlockWithJITType(codeBlock, JITType::FTLJIT), ": ", *registerOffsets, "\n");
+        dataLog(tierName, "Unwind info for ", CodeBlockWithJITType(codeBlock, JITType::FTLJIT), ": ", registerOffsets, "\n");
     codeBlock->setCalleeSaveRegisters(WTFMove(registerOffsets));
     ASSERT(!(state.proc->frameSize() % sizeof(EncodedJSValue)));