[webkit-changes] [185161] trunk/Source/JavaScriptCore

Wed, 03 Jun 2015 13:08:39 -0700

Title: [185161] trunk/Source/_javascript_Core
Revision
185161
Author
fpi...@apple.com
Date
2015-06-03 13:08:01 -0700 (Wed, 03 Jun 2015)

Log Message

CallLinkStatus should return takesSlowPath if the GC often cleared the IC
https://bugs.webkit.org/show_bug.cgi?id=145502

Reviewed by Geoffrey Garen.
        
CallLinkInfo now remembers when it has been cleared by GC. This has some safeguards for when
a call gets cleared by GC only because we hadn't converted it into a closure call; in that
case the GC will just tell us that it should be a closure call. The DFG will not optimize
a call that was cleared by GC, and the DFG will always prefer a closure call if the GC told
us that the specific callee was dead but the executable wasn't.
        
This guards us from some scenarios that came up in Speedometer. It's neutral on the pure JS
benchmarks, most likely just because those benchmarks aren't real enough to have interesting
GC of code.

* bytecode/CallLinkInfo.cpp:
(JSC::CallLinkInfo::visitWeak):
(JSC::CallLinkInfo::dummy):
* bytecode/CallLinkInfo.h:
(JSC::CallLinkInfo::CallLinkInfo):
* bytecode/CallLinkStatus.cpp:
(JSC::CallLinkStatus::computeFromCallLinkInfo):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (185160 => 185161)


--- trunk/Source/_javascript_Core/ChangeLog	2015-06-03 20:04:00 UTC (rev 185160)
+++ trunk/Source/_javascript_Core/ChangeLog	2015-06-03 20:08:01 UTC (rev 185161)
@@ -1,3 +1,28 @@
+2015-06-01  Filip Pizlo  <fpi...@apple.com>
+
+        CallLinkStatus should return takesSlowPath if the GC often cleared the IC
+        https://bugs.webkit.org/show_bug.cgi?id=145502
+
+        Reviewed by Geoffrey Garen.
+        
+        CallLinkInfo now remembers when it has been cleared by GC. This has some safeguards for when
+        a call gets cleared by GC only because we hadn't converted it into a closure call; in that
+        case the GC will just tell us that it should be a closure call. The DFG will not optimize
+        a call that was cleared by GC, and the DFG will always prefer a closure call if the GC told
+        us that the specific callee was dead but the executable wasn't.
+        
+        This guards us from some scenarios that came up in Speedometer. It's neutral on the pure JS
+        benchmarks, most likely just because those benchmarks aren't real enough to have interesting
+        GC of code.
+
+        * bytecode/CallLinkInfo.cpp:
+        (JSC::CallLinkInfo::visitWeak):
+        (JSC::CallLinkInfo::dummy):
+        * bytecode/CallLinkInfo.h:
+        (JSC::CallLinkInfo::CallLinkInfo):
+        * bytecode/CallLinkStatus.cpp:
+        (JSC::CallLinkStatus::computeFromCallLinkInfo):
+
 2015-06-02  Filip Pizlo  <fpi...@apple.com>
 
         GetById and PutById profiling should be more precise about it takes slow path

Modified: trunk/Source/_javascript_Core/bytecode/CallLinkInfo.cpp (185160 => 185161)


--- trunk/Source/_javascript_Core/bytecode/CallLinkInfo.cpp	2015-06-03 20:04:00 UTC (rev 185160)
+++ trunk/Source/_javascript_Core/bytecode/CallLinkInfo.cpp	2015-06-03 20:08:01 UTC (rev 185161)
@@ -58,6 +58,13 @@
 
 void CallLinkInfo::visitWeak(RepatchBuffer& repatchBuffer)
 {
+    auto handleSpecificCallee = [&] (JSFunction* callee) {
+        if (Heap::isMarked(callee->executable()))
+            hasSeenClosure = true;
+        else
+            clearedByGC = true;
+    };
+    
     if (isLinked()) {
         if (stub) {
             if (!stub->visitWeak(repatchBuffer)) {
@@ -68,6 +75,7 @@
                         ".\n");
                 }
                 unlink(repatchBuffer);
+                clearedByGC = true;
             }
         } else if (!Heap::isMarked(callee.get())) {
             if (Options::verboseOSR()) {
@@ -77,11 +85,14 @@
                     callee.get()->executable()->hashFor(specializationKind()),
                     ").\n");
             }
+            handleSpecificCallee(callee.get());
             unlink(repatchBuffer);
         }
     }
-    if (!!lastSeenCallee && !Heap::isMarked(lastSeenCallee.get()))
+    if (!!lastSeenCallee && !Heap::isMarked(lastSeenCallee.get())) {
+        handleSpecificCallee(lastSeenCallee.get());
         lastSeenCallee.clear();
+    }
 }
 
 CallLinkInfo& CallLinkInfo::dummy()

Modified: trunk/Source/_javascript_Core/bytecode/CallLinkInfo.h (185160 => 185161)


--- trunk/Source/_javascript_Core/bytecode/CallLinkInfo.h	2015-06-03 20:04:00 UTC (rev 185160)
+++ trunk/Source/_javascript_Core/bytecode/CallLinkInfo.h	2015-06-03 20:08:01 UTC (rev 185161)
@@ -59,6 +59,7 @@
         : isFTL(false)
         , hasSeenShouldRepatch(false)
         , hasSeenClosure(false)
+        , clearedByGC(false)
         , callType(None)
         , maxNumArguments(0)
         , slowPathCount(0)
@@ -92,7 +93,8 @@
     bool isFTL : 1;
     bool hasSeenShouldRepatch : 1;
     bool hasSeenClosure : 1;
-    unsigned callType : 5; // CallType
+    bool clearedByGC : 1;
+    unsigned callType : 4; // CallType
     unsigned calleeGPR : 8;
     uint8_t maxNumArguments; // Only used for varargs calls.
     uint32_t slowPathCount;

Modified: trunk/Source/_javascript_Core/bytecode/CallLinkStatus.cpp (185160 => 185161)


--- trunk/Source/_javascript_Core/bytecode/CallLinkStatus.cpp	2015-06-03 20:04:00 UTC (rev 185160)
+++ trunk/Source/_javascript_Core/bytecode/CallLinkStatus.cpp	2015-06-03 20:08:01 UTC (rev 185161)
@@ -135,6 +135,9 @@
 CallLinkStatus CallLinkStatus::computeFromCallLinkInfo(
     const ConcurrentJITLocker&, CallLinkInfo& callLinkInfo)
 {
+    if (callLinkInfo.clearedByGC)
+        return takesSlowPath();
+    
     // Note that despite requiring that the locker is held, this code is racy with respect
     // to the CallLinkInfo: it may get cleared while this code runs! This is because
     // CallLinkInfo::unlink() may be called from a different CodeBlock than the one that owns
@@ -148,11 +151,6 @@
     // that is still marginally valid (i.e. the pointers ain't stale). This kind of raciness
     // is probably OK for now.
     
-    // FIXME: If the GC often clears this call, we should probably treat it like it always takes the
-    // slow path. We could be smart about this; for example if we cleared a specific callee but the
-    // despecified executable was alive then we could note that separately.
-    // https://bugs.webkit.org/show_bug.cgi?id=145502
-    
     // PolymorphicCallStubRoutine is a GCAwareJITStubRoutine, so if non-null, it will stay alive
     // until next GC even if the CallLinkInfo is concurrently cleared. Also, the variants list is
     // never mutated after the PolymorphicCallStubRoutine is instantiated. We have some conservative

Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (185160 => 185161)


--- trunk/Source/_javascript_Core/jit/Repatch.cpp	2015-06-03 20:04:00 UTC (rev 185160)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp	2015-06-03 20:08:01 UTC (rev 185161)
@@ -1745,6 +1745,9 @@
         }
     }
     
+    if (isClosureCall)
+        callLinkInfo.hasSeenClosure = true;
+    
     Vector<PolymorphicCallCase> callCases;
     
     // Figure out what our cases are.

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to