Changeset 225315 in webkit for trunk/Source/JavaScriptCore/heap/CodeBlockSet.cpp

Timestamp:

Nov 29, 2017, 8:48:52 PM (8 years ago)

Author:

[email protected]

Message:

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):

File:

• trunk/Source/JavaScriptCore/heap/CodeBlockSet.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/heap/CodeBlockSet.cpp

@@ -29 +29 @@
 #include "CodeBlock.h"
 #include "JSCInlines.h"
+#include "SuperSampler.h"
 #include <wtf/CommaPrinter.h>
 
@@ -75 +76 @@
 {
     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;
+            });
     };