Changeset 276155 in webkit

Timestamp:

Apr 16, 2021, 12:24:22 PM (5 years ago)

Author:

keith_miller@apple.com

Message:

Before deleting a MarkedBlock we do not need to clear its m_directory pointer.
​https://bugs.webkit.org/show_bug.cgi?id=224677

Reviewed by Yusuke Suzuki.

Right now when we are about to free a MarkedBlock we clear the
m_directory pointer in the MarkedBlock's Handle. This has the
downside, however, of potentially paging in the footer from disk /
the compressor, which some data we have seen shows is happening.
This patch prevents this uncessary store to hopefully reduce the
number of pageins/decompressions caused by Safari web content.

• heap/BlockDirectory.cpp:

(JSC::BlockDirectory::removeBlock):
(JSC::BlockDirectory::removeBlockForDeletion):

• heap/BlockDirectory.h:
• heap/MarkedBlock.cpp:

(JSC::MarkedBlock::Handle::~Handle):

• heap/MarkedSpace.cpp:

(JSC::MarkedSpace::freeBlock):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/BlockDirectory.cpp (modified) (2 diffs)
• heap/BlockDirectory.h (modified) (1 diff)
• heap/MarkedBlock.cpp (modified) (1 diff)
• heap/MarkedSpace.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-04-16  Keith Miller  <keith_miller@apple.com>
+
+        Before deleting a MarkedBlock we do not need to clear its m_directory pointer.
+        https://bugs.webkit.org/show_bug.cgi?id=224677
+
+        Reviewed by Yusuke Suzuki.
+
+        Right now when we are about to free a MarkedBlock we clear the
+        m_directory pointer in the MarkedBlock's Handle. This has the
+        downside, however, of potentially paging in the footer from disk /
+        the compressor, which some data we have seen shows is happening.
+        This patch prevents this uncessary store to hopefully reduce the
+        number of pageins/decompressions caused by Safari web content.
+
+        * heap/BlockDirectory.cpp:
+        (JSC::BlockDirectory::removeBlock):
+        (JSC::BlockDirectory::removeBlockForDeletion):
+        * heap/BlockDirectory.h:
+        * heap/MarkedBlock.cpp:
+        (JSC::MarkedBlock::Handle::~Handle):
+        * heap/MarkedSpace.cpp:
+        (JSC::MarkedSpace::freeBlock):
+
 2021-04-16  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/heap/BlockDirectory.cpp

@@ -141 +141 @@
 }
 
-void BlockDirectory::removeBlock(MarkedBlock::Handle* block)
+void BlockDirectory::removeBlock(MarkedBlock::Handle* block, WillDeleteBlock willDelete)
 {
     ASSERT(block->directory() == this);
@@ -156 +156 @@
             vectorRef[block->index()] = false;
         });
-    
-    block->didRemoveFromDirectory();
+
+    if (willDelete == WillDeleteBlock::No)
+        block->didRemoveFromDirectory();
 }
 

trunk/Source/JavaScriptCore/heap/BlockDirectory.h

@@ -84 +84 @@
     
     void addBlock(MarkedBlock::Handle*);
-    void removeBlock(MarkedBlock::Handle*);
+    enum class WillDeleteBlock { No, Yes };
+    // If WillDeleteBlock::Yes is passed then the block will be left in an invalid state. We do this, however, to avoid potentially paging in / decompressing old blocks to update their handle just before freeing them.
+    void removeBlock(MarkedBlock::Handle*, WillDeleteBlock = WillDeleteBlock::No);
 
     bool isPagedOut(MonotonicTime deadline);

trunk/Source/JavaScriptCore/heap/MarkedBlock.cpp

@@ -77 +77 @@
             dataLog("MarkedBlock Balance: ", balance, "\n");
     }
-    removeFromDirectory();
+    m_directory->removeBlock(this, BlockDirectory::WillDeleteBlock::Yes);
     m_block->~MarkedBlock();
     m_alignedMemoryAllocator->freeAlignedMemory(m_block);

trunk/Source/JavaScriptCore/heap/MarkedSpace.cpp

@@ -376 +376 @@
 void MarkedSpace::freeBlock(MarkedBlock::Handle* block)
 {
-    block->directory()->removeBlock(block);
     m_capacity -= MarkedBlock::blockSize;
     m_blocks.remove(&block->block());