From 5d95bd39ca21a57a0811adadd5dd14a5ae1a98a6 Mon Sep 17 00:00:00 2001 From: Jakob Linke Date: Mon, 19 Dec 2022 08:36:29 +0100 Subject: [PATCH] [maglev] Prevent lazy deopts during maglev's JumpLoop (=OSR) The problem was that synchronous Maglev OSR potentially caused code deoptimization during compilation dependency finalization; this led to a lazy deopt when returning from the call to Runtime_CompileOptimizedOSRFromMaglev. However, a lazy deopt is disallowed at this point, since a) Maglev doesn't support marking an opcode as both lazy- and eager deopt, and b) the JumpLoop opcode is already marked as eager deopt since that's how OSR is implemented under the hood. See also the comment in runtime-compiler.cc. We fix this by changing synchronous Maglev-to-Turbofan OSR behavior s.t. actual OSR compilation is triggered from Ignition (and not from Maglev). In other words, when synchronous OSR is requested: 1. trigger an eager deopt from Maglev to Ignition by returning a non-null code object from Runtime_CompileOptimizedOSRFromMaglev. 2. Ignition handles the pending OSR compile request (through osr_urgency). This CL also reverts previous partial fixes: This reverts commit 21969e8e24ba80b601a1ad3216903ed647d85a0d. This reverts commit 6bcbcfed5c206f47570eca573bb9c5df3f71ad19. Bug: chromium:1394279,v8:13585,v8:7700 Change-Id: I3d64aa39575ad806ba2623102092176ca160ef0b Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4110740 Commit-Queue: Jakob Linke Reviewed-by: Victor Gomes Cr-Commit-Position: refs/heads/main@{#84922} --- src/maglev/x64/maglev-ir-x64.cc | 7 ++--- src/runtime/runtime-compiler.cc | 27 ++++++++++++------- .../mjsunit/maglev/regress/regress-1394279.js | 21 +++++++++++++++ tools/testrunner/local/variants.py | 4 ++- 4 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 test/mjsunit/maglev/regress/regress-1394279.js diff --git a/src/maglev/x64/maglev-ir-x64.cc b/src/maglev/x64/maglev-ir-x64.cc index c94981a141..e6f1cf334a 100644 --- a/src/maglev/x64/maglev-ir-x64.cc +++ b/src/maglev/x64/maglev-ir-x64.cc @@ -3048,9 +3048,10 @@ void AttemptOnStackReplacement(MaglevAssembler* masm, // Two cases may cause us to attempt OSR, in the following order: // // 1) Presence of cached OSR Turbofan code. - // 2) The OSR urgency exceeds the current loop depth - in that case, trigger - // a Turbofan OSR compilation if in concurrent mode. - // Otherwise trigger an eager deopt and delegate OSR to Ignition. + // 2) The OSR urgency exceeds the current loop depth - in that case, call + // into runtime to trigger a Turbofan OSR compilation. A non-zero return + // value means we should deopt into Ignition which will handle all further + // necessary steps (rewriting the stack frame, jumping to OSR'd code). // // See also: InterpreterAssembler::OnStackReplacement. diff --git a/src/runtime/runtime-compiler.cc b/src/runtime/runtime-compiler.cc index 4d2330ba74..98e7733995 100644 --- a/src/runtime/runtime-compiler.cc +++ b/src/runtime/runtime-compiler.cc @@ -548,18 +548,27 @@ RUNTIME_FUNCTION(Runtime_CompileOptimizedOSRFromMaglev) { DCHECK_EQ(frame->LookupCodeT().kind(), CodeKind::MAGLEV); Handle function = handle(frame->function(), isolate); - if (V8_UNLIKELY(function->ActiveTierIsTurbofan())) { + // This path is only relevant for tests (all production configurations enable + // concurrent OSR). It's quite subtle, if interested read on: + if (V8_UNLIKELY(!isolate->concurrent_recompilation_enabled() || + !v8_flags.concurrent_osr)) { + // - Synchronous Turbofan compilation may trigger lazy deoptimization (e.g. + // through compilation dependency finalization actions). + // - Maglev (currently) disallows marking an opcode as both can_lazy_deopt + // and can_eager_deopt. + // - Maglev's JumpLoop opcode (the logical caller of this runtime function) + // is marked as can_eager_deopt since OSR'ing to Turbofan involves + // deoptimizing to Ignition under the hood. + // - Thus this runtime function *must not* trigger a lazy deopt, and + // therefore cannot trigger synchronous Turbofan compilation (see above). + // + // We solve this synchronous OSR case by bailing out early to Ignition, and + // letting it handle OSR. How do we trigger the early bailout? Returning + // any non-null Code from this function triggers the deopt in JumpLoop. return function->code(); } - if (V8_LIKELY(isolate->concurrent_recompilation_enabled() && - v8_flags.concurrent_osr && v8_flags.turbofan)) { - return CompileOptimizedOSR(isolate, function, osr_offset); - } else { - function->MarkForOptimization(isolate, CodeKind::TURBOFAN, - ConcurrencyMode::kSynchronous); - return function->code(); - } + return CompileOptimizedOSR(isolate, function, osr_offset); } RUNTIME_FUNCTION(Runtime_LogOrTraceOptimizedOSREntry) { diff --git a/test/mjsunit/maglev/regress/regress-1394279.js b/test/mjsunit/maglev/regress/regress-1394279.js new file mode 100644 index 0000000000..a299853db2 --- /dev/null +++ b/test/mjsunit/maglev/regress/regress-1394279.js @@ -0,0 +1,21 @@ +// Copyright 2022 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// Flags: --allow-natives-syntax --maglev --single-threaded + +function foo(b) { + foo.bind(); + if (b) { + %OptimizeFunctionOnNextCall(foo); + } + for (let i = 0; i < 10000; i++) {} + foo instanceof foo; +} + + %PrepareFunctionForOptimization(foo); +foo(false); +%OptimizeMaglevOnNextCall(foo); +foo(true); +foo.prototype = foo; +foo(true); diff --git a/tools/testrunner/local/variants.py b/tools/testrunner/local/variants.py index 71a9a14a9f..5b056c7169 100644 --- a/tools/testrunner/local/variants.py +++ b/tools/testrunner/local/variants.py @@ -70,7 +70,9 @@ INCOMPATIBLE_FLAGS_PER_VARIANT = { ], "nooptimization": ["--always-turbofan"], "slow_path": ["--no-force-slow-path"], - "stress_concurrent_allocation": ["--single-threaded-gc", "--predictable"], + "stress_concurrent_allocation": [ + "--single-threaded", "--single-threaded-gc", "--predictable" + ], "stress_concurrent_inlining": [ "--single-threaded", "--predictable", "--lazy-feedback-allocation", "--assert-types", "--no-concurrent-recompilation"