CodeBlockSet::deleteUnmarkedAndUnreferenced can be a little more efficient
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Nov 2017 04:48:52 +0000 (04:48 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Nov 2017 04:48:52 +0000 (04:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180108

Reviewed by Saam Barati.

This was creating a vector of things to remove and then removing them. I think I remember writing
this code, and I did that because at the time we did not have removeAllMatching, which is
definitely better. This is a minuscule optimization for Speedometer. I wanted to land this
obvious improvement before I did more fundamental things to this code.

* heap/CodeBlockSet.cpp:
(JSC::CodeBlockSet::deleteUnmarkedAndUnreferenced):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/CodeBlockSet.cpp

index 78661e8..7f163a8 100644 (file)
@@ -1,3 +1,18 @@
+2017-11-28  Filip Pizlo  <fpizlo@apple.com>
+
+        CodeBlockSet::deleteUnmarkedAndUnreferenced can be a little more efficient
+        https://bugs.webkit.org/show_bug.cgi?id=180108
+
+        Reviewed by Saam Barati.
+        
+        This was creating a vector of things to remove and then removing them. I think I remember writing
+        this code, and I did that because at the time we did not have removeAllMatching, which is
+        definitely better. This is a minuscule optimization for Speedometer. I wanted to land this
+        obvious improvement before I did more fundamental things to this code.
+
+        * heap/CodeBlockSet.cpp:
+        (JSC::CodeBlockSet::deleteUnmarkedAndUnreferenced):
+
 2017-11-29  Filip Pizlo  <fpizlo@apple.com>
 
         GC should support isoheaps
index 137c593..ae79e79 100644 (file)
@@ -28,6 +28,7 @@
 
 #include "CodeBlock.h"
 #include "JSCInlines.h"
+#include "SuperSampler.h"
 #include <wtf/CommaPrinter.h>
 
 namespace JSC {
@@ -74,19 +75,26 @@ void CodeBlockSet::lastChanceToFinalize(VM& vm)
 void CodeBlockSet::deleteUnmarkedAndUnreferenced(VM& vm, CollectionScope scope)
 {
     LockHolder locker(&m_lock);
-    Vector<CodeBlock*> unmarked;
+    
+    // Destroying a CodeBlock takes about 1us on average in Speedometer. Full collections in Speedometer
+    // usually have ~2000 CodeBlocks to process. The time it takes to process the whole list varies a
+    // lot. In one extreme case I saw 18ms (on my fast MBP).
+    //
+    // FIXME: use Subspace instead of HashSet and adopt Subspace-based constraint solving. This may
+    // remove the need to eagerly destruct CodeBlocks.
+    // https://bugs.webkit.org/show_bug.cgi?id=180089
+    //
+    // FIXME: make CodeBlock::~CodeBlock a lot faster. It seems insane for that to take 1us or more.
+    // https://bugs.webkit.org/show_bug.cgi?id=180109
     
     auto consider = [&] (HashSet<CodeBlock*>& set) {
-        for (CodeBlock* codeBlock : set) {
-            if (Heap::isMarked(codeBlock))
-                continue;;
-            unmarked.append(codeBlock);
-        }
-        for (CodeBlock* codeBlock : unmarked) {
-            codeBlock->structure(vm)->classInfo()->methodTable.destroy(codeBlock);
-            set.remove(codeBlock);
-        }
-        unmarked.shrink(0);
+        set.removeIf(
+            [&] (CodeBlock* codeBlock) -> bool {
+                if (Heap::isMarked(codeBlock))
+                    return false;
+                codeBlock->structure(vm)->classInfo()->methodTable.destroy(codeBlock);
+                return true;
+            });
     };
 
     switch (scope) {