Investigate JIT deoptimization sequence in context of BG compiler (or additional isolate) triggering deoptimization #45213

mkustermann · 2021-03-05T13:44:50Z

Since efa1e18 our background compiler is no longer bailing out on class finalization. Instead it will get mutators to a safepoint and then possibly deoptimize dependent code and mutator stack's frames executing such code. (/cc @aam @rmacnak-google )

We should look though whether this is actually working as intended.

One concern that comes to mind is the following: When BG compiler will stop mutators it uses the normal safepointing mechanism. That means the mutator will be at any safepoint in the runtime. If the top-of-stack function is going to be deoptimized - as a result of invalidating dependent code - we need to be actually deoptimize frames on the stack.

Right now some IR instructions can emit code that ends up calling the runtime - and possibly hit a safepoint there - without being marked as deoptimizable. Though if the deoptimization information is missing we have a problem.

For example:

We can go to runtime to throw an exception:

class ShiftUint32OpInstr : public ShiftIntegerOpInstr {
  virtual bool ComputeCanDeoptimize() const { return false; }
  virtual bool MayThrow() const { return true; }
}

We can go to runtime to allocate memory

class AllocateContextInstr : public TemplateAllocation<0, NoThrow> {
  virtual bool ComputeCanDeoptimize() const { return false; }
}

(side note: strange that AllocateContextInstr is marked as NoThrow - since we probably can throw OutOfMemoryException)

So it seems that we should consider any IR instruction that can end up in runtime as possibly needing deoptimization info.

/cc @mraleph @alexmarkov

The text was updated successfully, but these errors were encountered:

mraleph · 2021-03-08T10:55:06Z

I think it is a fair assessment - now any instruction that does a non-leaf call needs to be a lazy-deopt aware.

This tells the compiler that boxing the result of Utf8ScanInstr will always fit in a Smi and the boxing instruction can therefore never go to runtime (and e.g. cause a lazy-deopt). Noticed as part of #45213 TEST=Existing test suite. Change-Id: I30712ec50378c7e0f70e1a5d95a2390b4f5d8ebc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/190481 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Aske Simon Christensen <askesc@google.com>

This simplifies the code and makes it easier for later work related to lazy-deopt of the instruction when returning from runtime call. Issue #45213 TEST=Existing test suite. Change-Id: I72e72554bdc306a9490a1460927d1dd650bfbe7d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191040 Reviewed-by: Tess Strickland <sstrickl@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

… ia32 This simplifies the code and makes it easier for later work related to lazy-deopt of the instruction when returning from runtime call. Issue #45213 TEST=Existing test suite. Change-Id: I056baac434532d3ed034ba488252ed0ff77eb488 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191041 Reviewed-by: Tess Strickland <sstrickl@google.com>

…-slow-path and --no-inline-alloc This will allow future tests to force allocations to go to runtime. This will be used to test deoptimization on allocation slow paths. Issue #45213 TEST=Existing test suite. Change-Id: I0d552c2b313147ce08c5fbb22761a400388abe7b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191043 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Tess Strickland <sstrickl@google.com>

…extending when decoding Will be used to encode deopt-id (which can be -1) into bit field. Issue #45213 TEST=Newly added vm/cc/BitFields_SignedField. Change-Id: I537820d813a191006388b8bfc08a3bac6f23f79f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191360 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Tess Strickland <sstrickl@google.com>

…l stub instead Having pushed arguments inside generated code makes deopt environments incorrect, so we'll move all runtime calls out of IL instructions into stubs. Issue #45213 TEST=Existing test suite. Change-Id: I843ba1ba626c3d704d75d4a7088752100ddfb906 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/191900 Reviewed-by: Tess Strickland <sstrickl@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

Simplifies work for adding deopt info to this IR instruction. Issue #45213 TEST=Existing test suite. Change-Id: Ia0936647f8f025397131b6d89764885a79cba9c7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192185 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Martin Kustermann <kustermann@google.com>

Generally speaking, the optimizing compiler should be able to use (deopt-id, deopt-environ) of any IR instruction and use it as an eager deopt target (e.g. optimizing compiler might insert TestCidsInstr with eager (deopt-id, deopt-environ) from AssertAssignable). Currently we prune the environment eagerly during SSA construction for some instructions This pruning breaks the above mechanism, since the deopt-environ isn't usable as eager deopt target. (It is effectively changing the environment on the IR instruction to be a lazy-deopt environment). This CL makes the [Environment] represent both the eager deopt target as well as the lazy deopt target. It distinguishes the two by remembering how many slots the eager deopt target needs to be pruned to come to the lazy deopt target. The SSA construction will populate this information. Effectively we move the deopt env pruning from SSA construction to the place when we need it (e.g. inlining, emitting after-call metadata). Issue #45213 TEST=Refactoring of existing code. Change-Id: I6c2a117b33f35764e556372484e4beaa294b708d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192141 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

…ctions This ensures the following IR instructions use a correctly pruned lazy-deopt environment: * AssertBoolInstr * AssertSubtypeInstr * InstantiateTypeInstr * InstantiateTypeArgumentsInstr Issue #45213 TEST=New vm/dart_2/isolates/deopt/*_test. Change-Id: I3d44909f0933c5df109d69e0a61f2d9d7dcc1247 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192184 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

…wing Allocation instructions can end up going to runtime - and in rare cases - throw a pre-allocated OutOfMemory exception. We have to mark allocation IR instructions therefore as potentially throwing. We make this CL separately from follow-up related changes, to see possible effects on benchmarks. Issue #45213 TEST=Existing tests. Will add validation of correct deopt environment in future CL. Change-Id: Ic3ed5c277c222f8432f5f72da18d32182e566dac Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192683 Reviewed-by: Vyacheslav Egorov <vegorov@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

An allocation can - in the slow path - go to runtime which can cause a pre-allocated OutOfMemory exception to be thrown. It therefore requires correct deopt information. Issue #45213 TEST=Existing test suite. A follow-up CL will add actual deoptimization tests for those instructions. Change-Id: Iaf44904b82ec021919352483f58e9e8fed473955 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192685 Reviewed-by: Vyacheslav Egorov <vegorov@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

… deopt environment Since allocation instructions can throw OOM exceptions, they require deopt information. In general it's rather hard to test that this deopt information is correct. In order to test this, this CL makes those IR instructions support lazy deopt and add tests that exercise this deopt sequence, thereby ensuring the deoptimization environment is correct. Issue #45213 TEST=New vm/dart_2/isolates/deopt/*_test. Change-Id: I6a02dcf5a0c47636f1f0aa4cd8cc0d2b4f032ca0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/192687 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

As a pre-requisite of partitioning our safepoints into "gc only safepoints" and "lazy-deopt'able safepoints" we'll let mutators keep track of whether a runtime call into the VM is deoptable. A mutator is lazy-deopt'able except for one particular case: When the mutator calls into the VM from a place where no deopt-id/deopt-env gets recorded (e.g. StoreInstanceField's box allocation goes to runtime). If a mutator is in the VM in a non-lazy-deopt'able state, it cannot call out to the embedder, transition from "vm" to "native" state, ... A future change will make new non-lazy-deopt'able runtime entries taking advantage of this CL. Issue #45213 TEST=Existing test suite & tests in future CLs. Change-Id: I30dbee02dc839b2ba9632b20ffa233aff4d58f64 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196922 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com> Reviewed-by: Ryan Macnak <rmacnak@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

…fepoint operations" This extends the existing safepoint operation mechanism by allowing to perform two different operations: * "gc safepoint operations": All mutators are stopped at places where it's safe to GC. It therefore requires stackmaps to be available for all optimized mutator frames. * "deopt safepoint operations": All mutators are stopped at places where it's safe to GC, but also safe to lazy-deopt mutator frames. It therefore requires deopt-id/deopt-info to be available for all optimized mutator frames. Mutators can be asked to block for any of those two safepoint operations. If a mutator is at a place where its safe to GC it will respond to "gc safepoint operations" requests, if a mutator is additionally at a place where it's also safe to lazy-deopt it will respond to "deopt safepoint operation" requests. Depending on how the runtime was entered (which is tracked via the [Thread::runtime_call_deopt_ability_] value) - the mutator might participate in both or only in gc safepoint operations. During the start of a "deopt safepoint operation", the safepoint handler will request all threads to stop at a "deopt safepoint". Some threads might first want to initiate their own "gc safepoint operation" (e.g. due to allocation failure) before they reach a "deopt safepoint". We do allow this by letting the safepoint handler own a "deopt safepoint operation" but still participate in other thread's "gc safepoint operation" requests until all mutators are checked into places where it's safe to lazy-deopt at which point the "deopt safepoint operation" also owns a "gc safepoint operation". In order to facilitate this, the Thread's safepoint_state will be extended to consist of the following bits: * AtSafepoint * SafepointRequested * AtDeoptSafepoint * DeoptSafepointRequested * BlockedForSafepoint Issue #45213 TEST=vm/cc/SafepointOperation_* Change-Id: Icdc2827718f6780818f99b829a5e806d6bb5b130 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196927 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com> Reviewed-by: Ryan Macnak <rmacnak@google.com>

…e entry The optimizing compiler can insert box instructions at various places during the optimization pipeline. There is no guarantee that a box instruction will get a proper (deopt-id, deopt-env) assigned to it - in many places it will not. Furthermore StoreInstanceField's into "unboxed fields" might cause box allocations to happen. Those stores might also not have deopt information associated with them. In general we require stackmaps when allocation slow paths go to runtime and cause GC. If GC throws OOM we theoretically would also need deopt information in order to populate correct catch-entry state. Though OOM is uncommon and the VM could decide to ignore the top-frame - if it's missing deopt information - since box allocations aren't something users are in control of (as opposed to user-allocated objects inside try/catch). => This CL makes box allocations go to a runtime entry that doesn't support lazy-deopt. While being in those runtime entries the mutators will also not participate in "deopt safepoint operation" requests. Issue #45213 TEST=Existing test suite. Change-Id: I1b61f77e3166da82efad08bb49bc1756576d220c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196928 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

…:kResultReg Slightly related to Issue #45213 TEST=Existing test coverage. Change-Id: I59953c9734b1e8cdba13a03c5734d084fc333130 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/196929 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Tess Strickland <sstrickl@google.com>

…int operation scope" The "RunWithMutatorsStopped" wrapper is used in places where the invoked callback can cause deoptimization of code. It should therefore ensure it's running with a "deopt safpoint operation scope" to ensure mutators are stopped at well-defined places that allow lazy-deopt. Issue #45213 TEST=Existing code base, will add "fuzzer"-like test to nightly/weekend builders. Change-Id: Icb9a4183c13fab0f084e481c10dfc56a0308126a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197162 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Ryan Macnak <rmacnak@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

To avoid accidental code that calls stubs without emitting proper (deopt-id, deopt-env) metadata, we enforce that it's always present for stub calls and add another GenerateNonLazyDeoptableStubCall for cases where it's intentionally omitted. The environment has in many cases been still emitted before, due to the usage of `pending_deoptimization_env` though we make code pass it explicitly (just as the deopt-id). We may want to consider deprecating this `pending_deoptimization_env`. Issue #45213 TEST=Existing test suite. Change-Id: I93f1d5ba4d74da5f9afa4b526ad57b9d032ca99e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/197164 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Vyacheslav Egorov <vegorov@google.com>

…ntry This is a follow-up to [0] which did the same for all other boxable objects (Mint/Double/Float32x4/Float64x2). [0] https://dart-review.googlesource.com/c/sdk/+/196928 Issue #45213 TEST=Existing test suite. Change-Id: I1736e9dce38435cf3d10587dd9201c41086dd51d Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199041 Reviewed-by: Daco Harkes <dacoharkes@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

Doing any runtime call inside an `<XXX>Instr::EmitNativeCode()` is dangerous because doing so almost always would require adjusting the lazy-deopt environment to ensure the continuation in unoptimized code would have the right stack state when returning from a runtime call. => Going to runtime should almost always happen via a stub call. To discourage using runtime calls inside `EmitNativeCode()` functions we remove the `FlowGraphCompiler::GenerateRuntimeCall()` helper method. The remaining runtime calls used in `EmitNativeCode()`s fall into three categories: * StackOverFlowInstr: In this particular case the deopt environment wouldn't need to be modified. * leaf runtime calls: Those do not need any metadata (neither deopt nor stackmaps) - since leaf runtime entries cannot safepoint * unopt-only runtime calls: Those do not need any metadata (neither deopt nor stackmaps) - since all slots in unoptimized frames are GC-able and unoptimized frames cannot be lazy-deoptimized. We add assertions for the latter to cases with a comment to make it clear. Issue #45213 TEST=Only adds assertions. Change-Id: Idb8badfbe65fff55585959338129405c71908d25 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199042 Reviewed-by: Alexander Markov <alexmarkov@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

…ods. Slightly related to #45213 TEST=Existing test suite. Change-Id: I63572d4fda838984ebb6010ef39c2cf3bd2b2958 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/200643 Reviewed-by: Vyacheslav Egorov <vegorov@google.com> Commit-Queue: Martin Kustermann <kustermann@google.com>

mkustermann · 2021-06-01T11:12:30Z

As part of addressing this issue, I've added a new test has the following failures:

vm/dart_2/deopt/restart_call_on_deopt_regress_46070_test RuntimeError (expected Pass)
vm/dart_2/deopt/restart_call_on_deopt_regress_46070_test Crash (expected Pass)

on configurations

dartk-win-debug-x64
dartk-win-product-x64
dartk-win-release-ia32
dartk-win-release-x64

…ginning of IR instruction If an IR instruction lazy-deopts it usually continues in unoptimized code in the same IR instruction after-call. Though in certain situations we want to continue before-call in unoptimized code. Two cases relevant in this CL: * An instruction gets LICMed: If it lazy-deopts it will continue at the Goto instruction outside the loop. * A recognized method which got it's InstanceCall replaced by several IR instructions. If any of them (except the last one) lazy-deopts it should re-try the call in unoptimized code (e.g. []=) In order to faciliate this we add a bit to the [Environment] which encodes whether the continuation point in unoptimized code is before-call - if so, we issue corresponding metadata. Issue #45213 Issue #46070 TEST=runtime/tests/vm/dart{,_2}/regress_46070_test.dart Change-Id: Ib824081768a2fd6293751a8fe09753e0d8155c87 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/200644 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com> Reviewed-by: Slava Egorov <vegorov@google.com>

…mization Right now RegExps get compiled to code like this: v1 <- Constant(<igoto offset table (int32 array)>) v2 <- InstanceCall([], ...) v3 <- InstanceCall([], v1, v2) IndirectGoto(v3) The [InstanceCall]s can go to runtime. When returning from runtime we can cause a lazy-deopt. The re-materialization of the unoptimized frame works correct. Though the "v3" variable will contain a PC-relative offset (telling the indirect goto how much to jump forward/backward in the optimized code). Obviously this PC-relative offset is specific to the optimized code. When we continue in unoptimized code, the indirect goto has to jump a different distance. The fix this CL makes is to avoid calling the `InstanceCall([], ...)` and instead encapsulate everything inside the IndirectGoto: v1 <- InstanceCall([], ...) IndirectGoto(v1 /*<- this is an index instead of an offset now*/) This ensures in unoptimized code no deopt safepoint can happen between looking up the offset in the constant offset array and the actual indirect jump. For the future: Maybe we should consider emitting a literal table at the end of the instruction stream instead of using a TypedDataInt32Array. Issue #45213 Closes #46399 TEST=vm/dart{,_2}/deopt/indirect_goto_regress_46399_test Change-Id: I81dc570ea997b3f477dd3f950a8fea03776eee0c Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204204 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Slava Egorov <vegorov@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>

…cializer The call specializer replaces implicit setter calls with AssertAssignable+StoreInstanceField. If the former lazy-deopts it will deopt to after-call and therefore the StoreInstanceField will not be performed. This CL makes use of newly added infrastructure that can mark such instructions as lazy-deopt to before-call, therefore causing a re-try of the instance call. Fixes #46446 Issue #45213 TEST=vm/dart{,_2}/deopt/restart_call_on_deopt_regress_46446_test Change-Id: I460e9a3c86b89a1ae127145024ace2d3d860d0af Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204721 Commit-Queue: Martin Kustermann <kustermann@google.com> Reviewed-by: Slava Egorov <vegorov@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>

mkustermann · 2021-06-28T11:25:09Z

To sumarize the work that has landed to make sharing JITed code safe in context of deoptimization:

Deopt pruning We have added support for deopt environment pruning, to ensure moved/hoisted/... instructions that lazy-deopt will have the correct environment and also ensure consumed inputs have been removed from the lazy deopt-env
Outline runtime calls We moved all runtime calls out of optimized EmitNative into stubs using register ABI - thereby ensuring that the default lazy-deopt environment is correct (no pushed arguments to runtime call on the stack when returning via lazy-deopt)
Added missing deopt info We have added (deopt-id, env) to some instructions that were missing it (e.g. some allocation instructions, ...)
Deopt Safepoint Operations: We added support for non lazy-deopt runtime calls to ensure some runtime calls can safely cause GC without checking into deoptimization safepoints (this is used in boxing allocation slow paths to avoid causing deopt in places where we don't have deopt env)
LazyDeopt to before-call We have added support for marking instructions as lazy-deopt to before-call (thereby re-trying the call). This is needed in inlining and call specialization where the replaced instructions could cause deopt.

So for now all known issues related to JIT + deoptimization have been fixed, therefore I'm going to close this issue.

mkustermann added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Mar 5, 2021

mkustermann self-assigned this Mar 10, 2021

mkustermann closed this as completed Jun 28, 2021

mkustermann mentioned this issue Jun 28, 2021

Lightweight Isolates & Faster isolate communication #36097

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Investigate JIT deoptimization sequence in context of BG compiler (or additional isolate) triggering deoptimization #45213

Investigate JIT deoptimization sequence in context of BG compiler (or additional isolate) triggering deoptimization #45213

mkustermann commented Mar 5, 2021 •

edited

Loading

mraleph commented Mar 8, 2021

mkustermann commented Jun 1, 2021

mkustermann commented Jun 28, 2021 •

edited

Loading

Investigate JIT deoptimization sequence in context of BG compiler (or additional isolate) triggering deoptimization #45213

Investigate JIT deoptimization sequence in context of BG compiler (or additional isolate) triggering deoptimization #45213

Comments

mkustermann commented Mar 5, 2021 • edited Loading

mraleph commented Mar 8, 2021

mkustermann commented Jun 1, 2021

mkustermann commented Jun 28, 2021 • edited Loading

mkustermann commented Mar 5, 2021 •

edited

Loading

mkustermann commented Jun 28, 2021 •

edited

Loading