[osr] Fall back to synchronous OSR on cache mismatches

If we've already cached OSR'd code for the current function but with a
different osr offset, fall back to synchronous compilation. This avoids
degenerate cases where we repeatedly spawn OSR jobs but then fail to
install them.

Drive-by: More consistent --trace-osr output.
Drive-by: Rename kCompileForOnStackReplacement to kCompileOptimizeOSR
for name consistency.
Drive-by: Add JSFunction::DebugNameCStr() for more convenient PrintF's.

Bug: v8:12161
Change-Id: I2b4a65bc9e082d85d7048a3e92ef86b07d396687
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3560431Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Jakob Linke <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79761}

src/builtins/arm/builtins-arm.cc

@@ -1824,7 +1824,7 @@ void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   ASM_CODE_COMMENT(masm);
   {
     FrameAndConstantPoolScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/arm64/builtins-arm64.cc

@@ -2077,7 +2077,7 @@ void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   ASM_CODE_COMMENT(masm);
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/ia32/builtins-ia32.cc

@@ -2806,7 +2806,7 @@ void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   ASM_CODE_COMMENT(masm);
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   Label skip;

src/builtins/loong64/builtins-loong64.cc

@@ -1803,7 +1803,7 @@ void Generate_OSREntry(MacroAssembler* masm, Register entry_address,
 void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/mips/builtins-mips.cc

@@ -1795,7 +1795,7 @@ void Generate_OSREntry(MacroAssembler* masm, Register entry_address,
 void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/mips64/builtins-mips64.cc

@@ -1800,7 +1800,7 @@ void Generate_OSREntry(MacroAssembler* masm, Register entry_address,
 void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/ppc/builtins-ppc.cc

@@ -1673,7 +1673,7 @@ void Builtins::Generate_NotifyDeoptimized(MacroAssembler* masm) {
 void Builtins::Generate_InterpreterOnStackReplacement(MacroAssembler* masm) {
   {
     FrameAndConstantPoolScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/riscv64/builtins-riscv64.cc

@@ -1874,7 +1874,7 @@ void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   ASM_CODE_COMMENT(masm);
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/s390/builtins-s390.cc

@@ -243,7 +243,7 @@ void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   ASM_CODE_COMMENT(masm);
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   // If the code object is null, just return to the caller.

src/builtins/x64/builtins-x64.cc

@@ -2729,7 +2729,7 @@ void Generate_OSREntry(MacroAssembler* masm, Register entry_address) {
 void OnStackReplacement(MacroAssembler* masm, bool is_interpreter) {
   {
     FrameScope scope(masm, StackFrame::INTERNAL);
-    __ CallRuntime(Runtime::kCompileForOnStackReplacement);
+    __ CallRuntime(Runtime::kCompileOptimizedOSR);
   }
 
   Label skip;

src/codegen/compiler.cc

@@ -121,22 +121,25 @@ class CompilerTracer : public AllStatic {
   }
 
   static void TraceOptimizeOSR(Isolate* isolate, Handle<JSFunction> function,
-                               BytecodeOffset osr_offset) {
+                               BytecodeOffset osr_offset,
+                               ConcurrencyMode mode) {
     if (!FLAG_trace_osr) return;
     CodeTracer::Scope scope(isolate->GetCodeTracer());
-    PrintF(scope.file(), "[OSR - Started: ");
-    function->PrintName(scope.file());
-    PrintF(scope.file(), " at OSR bytecode offset %d]\n", osr_offset.ToInt());
+    PrintF(scope.file(),
+           "[OSR - started. function: %s, osr offset: %d, mode: %s]\n",
+           function->DebugNameCStr().get(), osr_offset.ToInt(), ToString(mode));
   }
 
   static void TraceOptimizeOSRUnavailable(Isolate* isolate,
                                           Handle<JSFunction> function,
-                                          BytecodeOffset osr_offset) {
+                                          BytecodeOffset osr_offset,
+                                          ConcurrencyMode mode) {
     if (!FLAG_trace_osr) return;
     CodeTracer::Scope scope(isolate->GetCodeTracer());
-    PrintF(scope.file(), "[OSR - Unavailable (failed or in progress): ");
-    function->PrintName(scope.file());
-    PrintF(scope.file(), " at OSR bytecode offset %d]\n", osr_offset.ToInt());
+    PrintF(scope.file(),
+           "[OSR - unavailable (failed or in progress). function: %s, osr "
+           "offset: %d, mode: %s]\n",
+           function->DebugNameCStr().get(), osr_offset.ToInt(), ToString(mode));
   }
 
   static void TraceCompilationStats(Isolate* isolate,
@@ -3405,12 +3408,13 @@ MaybeHandle<CodeT> Compiler::CompileOptimizedOSR(Isolate* isolate,
   Handle<BytecodeArray> bytecode(frame->GetBytecodeArray(), isolate);
   bytecode->reset_osr_urgency();
 
-  CompilerTracer::TraceOptimizeOSR(isolate, function, osr_offset);
+  CompilerTracer::TraceOptimizeOSR(isolate, function, osr_offset, mode);
   MaybeHandle<CodeT> result = GetOrCompileOptimized(
       isolate, function, mode, CodeKind::TURBOFAN, osr_offset, frame);
 
   if (result.is_null()) {
-    CompilerTracer::TraceOptimizeOSRUnavailable(isolate, function, osr_offset);
+    CompilerTracer::TraceOptimizeOSRUnavailable(isolate, function, osr_offset,
+                                                mode);
   }
 
   return result;

src/execution/tiering-manager.cc

@@ -100,19 +100,17 @@ namespace {
 
 void TraceInOptimizationQueue(JSFunction function) {
   if (FLAG_trace_opt_verbose) {
-    PrintF("[not marking function ");
-    function.PrintName();
-    PrintF(" for optimization: already queued]\n");
+    PrintF("[not marking function %s for optimization: already queued]\n",
+           function.DebugNameCStr().get());
   }
 }
 
 void TraceHeuristicOptimizationDisallowed(JSFunction function) {
   if (FLAG_trace_opt_verbose) {
-    PrintF("[not marking function ");
-    function.PrintName();
     PrintF(
-        " for optimization: marked with "
-        "%%PrepareFunctionForOptimization for manual optimization]\n");
+        "[not marking function %s for optimization: marked with "
+        "%%PrepareFunctionForOptimization for manual optimization]\n",
+        function.DebugNameCStr().get());
   }
 }
 
@@ -153,21 +151,19 @@ namespace {
 bool HaveCachedOSRCodeForCurrentBytecodeOffset(UnoptimizedFrame* frame,
                                                int* osr_urgency_out) {
   JSFunction function = frame->function();
-  BytecodeArray bytecode = frame->GetBytecodeArray();
-  const int bytecode_offset = frame->GetBytecodeOffset();
-  if (V8_UNLIKELY(function.shared().osr_code_cache_state() != kNotCached)) {
-    OSROptimizedCodeCache cache = function.native_context().osr_code_cache();
-    interpreter::BytecodeArrayIterator iterator(
-        handle(bytecode, frame->isolate()));
-    for (int jump_offset : cache.GetBytecodeOffsetsFromSFI(function.shared())) {
-      iterator.SetOffset(jump_offset);
-      if (base::IsInRange(bytecode_offset, iterator.GetJumpTargetOffset(),
-                          jump_offset)) {
-        int loop_depth = iterator.GetImmediateOperand(1);
-        // `+ 1` because osr_urgency is an exclusive upper limit on the depth.
-        *osr_urgency_out = loop_depth + 1;
-        return true;
-      }
+  const int current_offset = frame->GetBytecodeOffset();
+  OSROptimizedCodeCache cache = function.native_context().osr_code_cache();
+  interpreter::BytecodeArrayIterator iterator(
+      handle(frame->GetBytecodeArray(), frame->isolate()));
+  for (BytecodeOffset osr_offset : cache.OsrOffsetsFor(function.shared())) {
+    DCHECK(!osr_offset.IsNone());
+    iterator.SetOffset(osr_offset.ToInt());
+    if (base::IsInRange(current_offset, iterator.GetJumpTargetOffset(),
+                        osr_offset.ToInt())) {
+      int loop_depth = iterator.GetImmediateOperand(1);
+      // `+ 1` because osr_urgency is an exclusive upper limit on the depth.
+      *osr_urgency_out = loop_depth + 1;
+      return true;
     }
   }
   return false;
@@ -227,14 +223,15 @@ void TrySetOsrUrgency(Isolate* isolate, JSFunction function, int osr_urgency) {
 
   // We've passed all checks - bump the OSR urgency.
 
+  BytecodeArray bytecode = shared.GetBytecodeArray(isolate);
   if (V8_UNLIKELY(FLAG_trace_osr)) {
     CodeTracer::Scope scope(isolate->GetCodeTracer());
-    PrintF(scope.file(), "[OSR - arming back edges in ");
-    function.PrintName(scope.file());
-    PrintF(scope.file(), "]\n");
+    PrintF(scope.file(),
+           "[OSR - setting osr urgency. function: %s, old urgency: %d, new "
+           "urgency: %d]\n",
+           function.DebugNameCStr().get(), bytecode.osr_urgency(), osr_urgency);
   }
 
-  BytecodeArray bytecode = shared.GetBytecodeArray(isolate);
   DCHECK_GE(osr_urgency, bytecode.osr_urgency());  // Never lower urgency here.
   bytecode.set_osr_urgency(osr_urgency);
 }
@@ -352,9 +349,8 @@ OptimizationDecision TieringManager::ShouldOptimize(JSFunction function,
     // small, optimistically optimize it now.
     return OptimizationDecision::TurbofanSmallFunction();
   } else if (FLAG_trace_opt_verbose) {
-    PrintF("[not yet optimizing ");
-    function.PrintName();
-    PrintF(", not enough ticks: %d/%d and ", ticks, ticks_for_optimization);
+    PrintF("[not yet optimizing %s, not enough ticks: %d/%d and ",
+           function.DebugNameCStr().get(), ticks, ticks_for_optimization);
     if (any_ic_changed_) {
       PrintF("ICs changed]\n");
     } else {

src/objects/js-function.cc

@@ -1110,8 +1110,12 @@ int JSFunction::ComputeInstanceSizeWithMinSlack(Isolate* isolate) {
   return initial_map().instance_size();
 }
 
+std::unique_ptr<char[]> JSFunction::DebugNameCStr() {
+  return shared().DebugNameCStr();
+}
+
 void JSFunction::PrintName(FILE* out) {
-  PrintF(out, "%s", shared().DebugNameCStr().get());
+  PrintF(out, "%s", DebugNameCStr().get());
 }
 
 namespace {

src/objects/js-function.h

@@ -300,7 +300,7 @@ class JSFunction : public TorqueGeneratedJSFunction<
                                        : JSFunction::kSizeWithoutPrototype;
   }
 
-  // Prints the name of the function using PrintF.
+  std::unique_ptr<char[]> DebugNameCStr();
   void PrintName(FILE* out = stdout);
 
   // Calculate the instance size and in-object properties count.

src/objects/osr-optimized-code-cache.cc

@@ -133,16 +133,36 @@ void OSROptimizedCodeCache::EvictDeoptimizedCode(Isolate* isolate) {
   }
 }
 
-std::vector<int> OSROptimizedCodeCache::GetBytecodeOffsetsFromSFI(
+std::vector<BytecodeOffset> OSROptimizedCodeCache::OsrOffsetsFor(
     SharedFunctionInfo shared) {
-  std::vector<int> bytecode_offsets;
   DisallowGarbageCollection gc;
+
+  const OSRCodeCacheStateOfSFI state = shared.osr_code_cache_state();
+  if (state == kNotCached) return {};
+
+  std::vector<BytecodeOffset> offsets;
   for (int index = 0; index < length(); index += kEntryLength) {
-    if (GetSFIFromEntry(index) == shared) {
-      bytecode_offsets.push_back(GetBytecodeOffsetFromEntry(index).ToInt());
-    }
+    if (GetSFIFromEntry(index) != shared) continue;
+    offsets.emplace_back(GetBytecodeOffsetFromEntry(index));
+    if (state == kCachedOnce) return offsets;
   }
-  return bytecode_offsets;
+
+  return offsets;
+}
+
+base::Optional<BytecodeOffset> OSROptimizedCodeCache::FirstOsrOffsetFor(
+    SharedFunctionInfo shared) {
+  DisallowGarbageCollection gc;
+
+  const OSRCodeCacheStateOfSFI state = shared.osr_code_cache_state();
+  if (state == kNotCached) return {};
+
+  for (int index = 0; index < length(); index += kEntryLength) {
+    if (GetSFIFromEntry(index) != shared) continue;
+    return GetBytecodeOffsetFromEntry(index);
+  }
+
+  return {};
 }
 
 int OSROptimizedCodeCache::GrowOSRCache(

src/objects/osr-optimized-code-cache.h

@@ -57,6 +57,9 @@ class V8_EXPORT OSROptimizedCodeCache : public WeakFixedArray {
   CodeT TryGet(SharedFunctionInfo shared, BytecodeOffset osr_offset,
                Isolate* isolate);
 
+  std::vector<BytecodeOffset> OsrOffsetsFor(SharedFunctionInfo shared);
+  base::Optional<BytecodeOffset> FirstOsrOffsetFor(SharedFunctionInfo shared);
+
   // Remove all code objects marked for deoptimization from OSR code cache.
   void EvictDeoptimizedCode(Isolate* isolate);
 
@@ -67,10 +70,6 @@ class V8_EXPORT OSROptimizedCodeCache : public WeakFixedArray {
   // Sets the OSR optimized code cache to an empty array.
   static void Clear(Isolate* isolate, NativeContext context);
 
-  // Returns vector of bytecode offsets corresponding to the shared function
-  // |shared|
-  std::vector<int> GetBytecodeOffsetsFromSFI(SharedFunctionInfo shared);
-
   enum OSRCodeCacheConstants {
     kSharedOffset,
     kCachedCodeOffset,

src/objects/shared-function-info.h

@@ -15,7 +15,6 @@
 #include "src/objects/function-kind.h"
 #include "src/objects/function-syntax-kind.h"
 #include "src/objects/objects.h"
-#include "src/objects/osr-optimized-code-cache.h"
 #include "src/objects/script.h"
 #include "src/objects/slots.h"
 #include "src/objects/smi.h"
@@ -42,6 +41,8 @@ class WasmCapiFunctionData;
 class WasmExportedFunctionData;
 class WasmJSFunctionData;
 
+enum OSRCodeCacheStateOfSFI : uint8_t;
+
 namespace wasm {
 struct WasmModule;
 class ValueType;

src/runtime/runtime-compiler.cc

@@ -56,9 +56,7 @@ RUNTIME_FUNCTION(Runtime_CompileLazy) {
 
 #ifdef DEBUG
   if (FLAG_trace_lazy && !sfi->is_compiled()) {
-    PrintF("[unoptimized: ");
-    function->PrintName();
-    PrintF("]\n");
+    PrintF("[unoptimized: %s]\n", function->DebugNameCStr().get());
   }
 #endif
 
@@ -225,7 +223,7 @@ RUNTIME_FUNCTION(Runtime_VerifyType) {
   return *obj;
 }
 
-RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
+RUNTIME_FUNCTION(Runtime_CompileOptimizedOSR) {
   HandleScope handle_scope(isolate);
   DCHECK_EQ(0, args.length());
   DCHECK(FLAG_use_osr);
@@ -244,21 +242,40 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
   BytecodeOffset osr_offset = BytecodeOffset(frame->GetBytecodeOffset());
   DCHECK(!osr_offset.IsNone());
 
-  // TODO(v8:12161): If cache exists with different offset: kSynchronous.
   ConcurrencyMode mode =
-      isolate->concurrent_recompilation_enabled() && FLAG_concurrent_osr
+      V8_LIKELY(isolate->concurrent_recompilation_enabled() &&
+                FLAG_concurrent_osr)
           ? ConcurrencyMode::kConcurrent
           : ConcurrencyMode::kSynchronous;
 
+  // The synchronous fallback mechanism triggers if we've already got OSR'd
+  // code for the current function but at a different OSR offset - that may
+  // indicate we're having trouble hitting the correct JumpLoop for code
+  // installation. In this case, fall back to synchronous OSR.
   Handle<JSFunction> function(frame->function(), isolate);
-  MaybeHandle<CodeT> maybe_result =
-      Compiler::CompileOptimizedOSR(isolate, function, osr_offset, frame, mode);
+  base::Optional<BytecodeOffset> cached_osr_offset =
+      function->native_context().osr_code_cache().FirstOsrOffsetFor(
+          function->shared());
+  if (cached_osr_offset.has_value() &&
+      cached_osr_offset.value() != osr_offset) {
+    if (V8_UNLIKELY(FLAG_trace_osr)) {
+      CodeTracer::Scope scope(isolate->GetCodeTracer());
+      PrintF(scope.file(),
+             "[OSR - falling back to synchronous compilation due to mismatched "
+             "cached entry. function: %s, requested: %d, cached: %d]\n",
+             function->DebugNameCStr().get(), osr_offset.ToInt(),
+             cached_osr_offset.value().ToInt());
+    }
+    mode = ConcurrencyMode::kSynchronous;
+  }
 
   Handle<CodeT> result;
-  if (!maybe_result.ToHandle(&result)) {
-    // No OSR'd code available.
-    // TODO(v8:12161): Distinguish between actual failure and scheduling a
-    // concurrent job.
+  if (!Compiler::CompileOptimizedOSR(isolate, function, osr_offset, frame, mode)
+           .ToHandle(&result)) {
+    // An empty result can mean one of two things:
+    // 1) we've started a concurrent compilation job - everything is fine.
+    // 2) synchronous compilation failed for some reason.
+
     if (!function->HasAttachedOptimizedCode()) {
       function->set_code(function->shared().GetCode(), kReleaseStore);
     }
@@ -278,9 +295,9 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
   if (FLAG_trace_osr) {
     CodeTracer::Scope scope(isolate->GetCodeTracer());
     PrintF(scope.file(),
-           "[OSR - Entry at OSR bytecode offset %d, offset %d in optimized "
-           "code]\n",
-           osr_offset.ToInt(), data.OsrPcOffset().value());
+           "[OSR - entry. function: %s, osr offset: %d, pc offset: %d]\n",
+           function->DebugNameCStr().get(), osr_offset.ToInt(),
+           data.OsrPcOffset().value());
   }
 
   if (function->feedback_vector().invocation_count() <= 1 &&
@@ -313,9 +330,10 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
     // compile for OSR again.
     if (FLAG_trace_osr) {
       CodeTracer::Scope scope(isolate->GetCodeTracer());
-      PrintF(scope.file(), "[OSR - Re-marking ");
-      function->PrintName(scope.file());
-      PrintF(scope.file(), " for non-concurrent optimization]\n");
+      PrintF(scope.file(),
+             "[OSR - forcing synchronous optimization on next entry. function: "
+             "%s]\n",
+             function->DebugNameCStr().get());
     }
     function->set_tiering_state(TieringState::kRequestTurbofan_Synchronous);
   }

src/runtime/runtime-test.cc

@@ -661,7 +661,7 @@ RUNTIME_FUNCTION(Runtime_OptimizeOsr) {
     USE(unused_result);
 
     // Finalize again to finish the queued job. The next call into
-    // CompileForOnStackReplacement will pick up the cached Code object.
+    // Runtime::kCompileOptimizedOSR will pick up the cached Code object.
     FinalizeOptimization(isolate);
   }
 

src/runtime/runtime.h

@@ -107,7 +107,7 @@ namespace internal {
   F(WeakCollectionSet, 4, 1)
 
 #define FOR_EACH_INTRINSIC_COMPILER(F, I) \
-  F(CompileForOnStackReplacement, 0, 1)   \
+  F(CompileOptimizedOSR, 0, 1)            \
   F(CompileLazy, 1, 1)                    \
   F(CompileBaseline, 1, 1)                \
   F(CompileMaglev_Concurrent, 1, 1)       \