OSR entry: delay outer-loop compilation when at inner-loop
authorjfbastien@apple.com <jfbastien@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Feb 2017 01:26:00 +0000 (01:26 +0000)
committerjfbastien@apple.com <jfbastien@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Feb 2017 01:26:00 +0000 (01:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167149

Reviewed by Filip Pizlo.

r211224 was reverted because it caused a massive kraken/ai-astar
regression. This patch instead does the minimally-disruptive
change to fix the original bug as described below, but omits extra
tuning and refactoring which I had before. I'll commit tuning and
refactoring separately, if this sticks. This patch is therefore
very minimal, and layers carefully on top of the complex
spaghetti-logic. The only change it makes is that it uses triggers
to indicate to outer loops that they should compile, which fixes
the immediate bug and seems roughly perf neutral (maybe a small
gain on kraken sometimes, other times a small regression as would
be expected from compiling later).

As of https://bugs.webkit.org/show_bug.cgi?id=155217 OSR
compilation can be kicked off for an entry into an outer-loop,
while executing an inner-loop. This is desirable because often the
codegen from an inner-entry isn't as good as the codegen from an
outer-entry, but execution from an inner-loop is often pretty hot
and likely to kick off compilation. This approach provided nice
speedups on Kraken because we'd select to enter to the outer-loop
very reliably, which reduces variability (the inner-loop was
selected roughly 1/5 times from my unscientific measurements).

When compilation starts we take a snapshot of the JSValues at the
current execution state using OSR's recovery mechanism. These
values are passed to the compiler and are used as way to perform
type profiling, and could be used to observe cell types as well as
to perform predictions such as through constant propagation.

It's therefore desired to enter from the outer-loop when we can,
but we need to be executing from that location to capture the
right JSValues, otherwise we're confusing the compiler and giving
it inaccurate JSValues which can lead it to predict the wrong
things, leading to suboptimal code or recompilation due to
misprediction, or in super-corner-cases a crash.

These effects are pretty hard to measure: Fil points out that
marsalis-osr-entry really needs mustHandleValues (the JSValues
from the point of execution) because right now it just happens to
correctly guess int32. I tried removing mustHandleValues entirely
and saw no slowdowns, but our benchmarks probably aren't
sufficient to reliably find issues, sometimes because we happen to
have sufficient mitigations.

DFG tier-up was added here:
https://bugs.webkit.org/show_bug.cgi?id=112838

* dfg/DFGOperations.cpp:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp

index 6184862..70ad6ae 100644 (file)
@@ -1,3 +1,58 @@
+2017-01-31  JF Bastien  <jfbastien@apple.com>
+
+        OSR entry: delay outer-loop compilation when at inner-loop
+        https://bugs.webkit.org/show_bug.cgi?id=167149
+
+        Reviewed by Filip Pizlo.
+
+        r211224 was reverted because it caused a massive kraken/ai-astar
+        regression. This patch instead does the minimally-disruptive
+        change to fix the original bug as described below, but omits extra
+        tuning and refactoring which I had before. I'll commit tuning and
+        refactoring separately, if this sticks. This patch is therefore
+        very minimal, and layers carefully on top of the complex
+        spaghetti-logic. The only change it makes is that it uses triggers
+        to indicate to outer loops that they should compile, which fixes
+        the immediate bug and seems roughly perf neutral (maybe a small
+        gain on kraken sometimes, other times a small regression as would
+        be expected from compiling later).
+
+        As of https://bugs.webkit.org/show_bug.cgi?id=155217 OSR
+        compilation can be kicked off for an entry into an outer-loop,
+        while executing an inner-loop. This is desirable because often the
+        codegen from an inner-entry isn't as good as the codegen from an
+        outer-entry, but execution from an inner-loop is often pretty hot
+        and likely to kick off compilation. This approach provided nice
+        speedups on Kraken because we'd select to enter to the outer-loop
+        very reliably, which reduces variability (the inner-loop was
+        selected roughly 1/5 times from my unscientific measurements).
+
+        When compilation starts we take a snapshot of the JSValues at the
+        current execution state using OSR's recovery mechanism. These
+        values are passed to the compiler and are used as way to perform
+        type profiling, and could be used to observe cell types as well as
+        to perform predictions such as through constant propagation.
+
+        It's therefore desired to enter from the outer-loop when we can,
+        but we need to be executing from that location to capture the
+        right JSValues, otherwise we're confusing the compiler and giving
+        it inaccurate JSValues which can lead it to predict the wrong
+        things, leading to suboptimal code or recompilation due to
+        misprediction, or in super-corner-cases a crash.
+
+        These effects are pretty hard to measure: Fil points out that
+        marsalis-osr-entry really needs mustHandleValues (the JSValues
+        from the point of execution) because right now it just happens to
+        correctly guess int32. I tried removing mustHandleValues entirely
+        and saw no slowdowns, but our benchmarks probably aren't
+        sufficient to reliably find issues, sometimes because we happen to
+        have sufficient mitigations.
+
+        DFG tier-up was added here:
+        https://bugs.webkit.org/show_bug.cgi?id=112838
+
+        * dfg/DFGOperations.cpp:
+
 2017-01-31  Filip Pizlo  <fpizlo@apple.com>
 
         The mutator should be able to perform increments of GC work
index dc54aec..8a2395d 100644 (file)
@@ -2351,6 +2351,19 @@ static char* tierUpCommon(ExecState* exec, unsigned originBytecodeIndex, unsigne
         worklistState = Worklist::NotKnown;
 
     JITCode* jitCode = codeBlock->jitCode()->dfg();
+
+    bool triggeredSlowPath = false;
+    auto tierUpEntryTriggers = jitCode->tierUpEntryTriggers.find(osrEntryBytecodeIndex);
+    if (tierUpEntryTriggers != jitCode->tierUpEntryTriggers.end()) {
+        if (tierUpEntryTriggers->value == 1) {
+            // We were asked to enter as soon as possible. Unset this trigger so we don't continually enter.
+            if (Options::verboseOSR())
+                dataLog("EntryTrigger for ", *codeBlock, " forced slow-path.\n");
+            triggeredSlowPath = true;
+            tierUpEntryTriggers->value = 0;
+        }
+    }
+
     if (worklistState == Worklist::Compiling) {
         CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("still compiling"));
         jitCode->setOptimizationThresholdBasedOnCompilationResult(
@@ -2366,8 +2379,12 @@ static char* tierUpCommon(ExecState* exec, unsigned originBytecodeIndex, unsigne
         return nullptr;
     }
 
+    // The following is only true for triggerTierUpNowInLoop, which can never
+    // be an OSR entry.
+    bool canOSRFromHere = originBytecodeIndex == osrEntryBytecodeIndex;
+
     // If we can OSR Enter, do it right away.
-    if (originBytecodeIndex == osrEntryBytecodeIndex) {
+    if (canOSRFromHere) {
         unsigned streamIndex = jitCode->bytecodeIndexToStreamIndex.get(originBytecodeIndex);
         if (CodeBlock* entryBlock = jitCode->osrEntryBlock()) {
             if (void* address = FTL::prepareOSREntry(exec, codeBlock, entryBlock, originBytecodeIndex, streamIndex)) {
@@ -2381,10 +2398,10 @@ static char* tierUpCommon(ExecState* exec, unsigned originBytecodeIndex, unsigne
     // - If we do have an FTL code block, then try to enter for a while.
     // - If we couldn't enter for a while, then trigger OSR entry.
 
-    if (!shouldTriggerFTLCompile(codeBlock, jitCode))
+    if (!shouldTriggerFTLCompile(codeBlock, jitCode) && !triggeredSlowPath)
         return nullptr;
 
-    if (!jitCode->neverExecutedEntry) {
+    if (!jitCode->neverExecutedEntry && !triggeredSlowPath) {
         triggerFTLReplacementCompile(vm, codeBlock, jitCode);
 
         if (!codeBlock->hasOptimizedReplacement())
@@ -2430,13 +2447,28 @@ static char* tierUpCommon(ExecState* exec, unsigned originBytecodeIndex, unsigne
         return nullptr;
     }
 
+    if (!canOSRFromHere) {
+        // We can't OSR from here, or even start a compilation because doing so
+        // calls jitCode->reconstruct which would get the wrong state.
+        if (Options::verboseOSR())
+            dataLog("Non-OSR-able bc#", originBytecodeIndex, " in ", *codeBlock, " setting parent loop's trigger and backing off.\n");
+        jitCode->tierUpEntryTriggers.set(osrEntryBytecodeIndex, 1);
+        jitCode->dontOptimizeAnytimeSoon(codeBlock);
+        return nullptr;
+    }
+
     unsigned streamIndex = jitCode->bytecodeIndexToStreamIndex.get(osrEntryBytecodeIndex);
-    auto tierUpHierarchyEntry = jitCode->tierUpInLoopHierarchy.find(osrEntryBytecodeIndex);
-    if (tierUpHierarchyEntry != jitCode->tierUpInLoopHierarchy.end()) {
-        for (unsigned osrEntryCandidate : tierUpHierarchyEntry->value) {
-            if (jitCode->tierUpEntrySeen.contains(osrEntryCandidate)) {
-                osrEntryBytecodeIndex = osrEntryCandidate;
-                streamIndex = jitCode->bytecodeIndexToStreamIndex.get(osrEntryBytecodeIndex);
+
+    if (!triggeredSlowPath) {
+        auto tierUpHierarchyEntry = jitCode->tierUpInLoopHierarchy.find(osrEntryBytecodeIndex);
+        if (tierUpHierarchyEntry != jitCode->tierUpInLoopHierarchy.end()) {
+            for (unsigned osrEntryCandidate : tierUpHierarchyEntry->value) {
+                if (jitCode->tierUpEntrySeen.contains(osrEntryCandidate)) {
+                    // Ask an enclosing loop to compile, instead of doing so here.
+                    jitCode->tierUpEntryTriggers.set(osrEntryCandidate, 1);
+                    jitCode->dontOptimizeAnytimeSoon(codeBlock);
+                    return nullptr;
+                }
             }
         }
     }