Skip to content

VM: enable OSR in block expressions #36421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
aartbik opened this issue Apr 1, 2019 · 2 comments
Closed

VM: enable OSR in block expressions #36421

aartbik opened this issue Apr 1, 2019 · 2 comments
Assignees
@aartbik
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@aartbik
Copy link
Contributor

aartbik commented Apr 1, 2019

The current VM implementation of block expressions disables OSR while inside a block expression to avoid some issues with temps on expression stack. This is okay for a first implementation, since widget loops inside control flow collections can be expected to be short running. However, for a high quality implementation, we should enable OSR even inside those blocks, since it is always possible to write a contrived long running loop. This bug tracks the progress of that work.

Probably biggest VM item left to be explored is enabling OSR points inside control flow collection for loops. Alex notes we should ensure that temporaries on the expression stack are preserved when OSR happens.

Consider a more complex expression like

var x = x.bar(42, bazz(), 'abc', [more_bazz(), for (int i = 0; i < n; i++) 2*i ]);

In this case x, 42, bazz() and 'abc' should be on the expression stack unless CFE introduces temporary variables for them. Let's:
(1) test if the above example works;
(2) dump kernel to see how CFE generates this;
(3) print flowgraph of a function compiled for OSR and check that it loads captured values correctly.
@aartbik aartbik added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Apr 1, 2019
@aartbik aartbik self-assigned this Apr 1, 2019
This was referenced Apr 1, 2019
Closed
Closed
@aartbik
Copy link
Contributor Author

aartbik commented Apr 1, 2019

Great example! Something like

@NeverInline
List<int> foo(int n) {
  var x = z.bar(42, bazz(), 'abc', [more_bazz(), for (int i = 0; i < n; i++) 2*i ]);
  return x;
}

passes without OSR in block expr

Attempting OSR for dart:core__GrowableList@0150898__grow@0150898 at id=54, count=60000
OSR OK dart:core__GrowableList@0150898__grow@0150898 at 54
Attempting OSR for file:///usr/local/google/home/ajcbik/Repo/DART/osr.dart_::_main at id=110, count=60006
OSR OK file:///usr/local/google/home/ajcbik/Repo/DART/osr.dart_::_main at 110

but when OSR inside the block expression is enable as well, it crashes (segfault in release mode, the following assertion in debug mode).

Attempting OSR for file:///usr/local/google/home/ajcbik/Repo/DART/osr.dart_::_foo at id=48, count=60002
../../runtime/platform/growable_array.h: 57: error: expected: index < length_
version=2.2.1-edge.74d5ea5785d94724cfca64752c85564cd7dcf405 (Mon Apr 1 10:50:13 2019 -0700) on "linux_x64"
thread=37215, isolate=main(0x5627d1f31200)
  [0x00005627cfc3eb3c] dart::Profiler::DumpStackTrace(void*)
  [0x00005627cfc3eb3c] dart::Profiler::DumpStackTrace(void*)
  [0x00005627cff1a682] dart::Assert::Fail(char const*, ...)
  [0x00005627cfd515b8] dart::FlowGraph::RenameRecursive(dart::BlockEntryInstr*, dart::GrowableArray<dart::Definition*>*, dart::GrowableArray<dart::PhiInstr*>*, dart::VariableLivenessAnalysis*, dart::ZoneGrowableArray<dart::Definition*>*)
  [0x00005627cfd5135e] dart::FlowGraph::RenameRecursive(dart::BlockEntryInstr*, dart::GrowableArray<dart::Definition*>*, dart::GrowableArray<dart::PhiInstr*>*, dart::VariableLivenessAnalysis*, dart::ZoneGrowableArray<dart::Definition*>*)
  [0x00005627cfd5135e] dart::FlowGraph::RenameRecursive(dart::BlockEntryInstr*, dart::GrowableArray<dart::Definition*>*, dart::GrowableArray<dart::PhiInstr*>*, dart::VariableLivenessAnalysis*, dart::ZoneGrowableArray<dart::Definition*>*)
  [0x00005627cfd50251] dart::FlowGraph::Rename(dart::GrowableArray<dart::PhiInstr*>*, dart::VariableLivenessAnalysis*, dart::ZoneGrowableArray<dart::Definition*>*)
  [0x00005627cfd4ef74] dart::FlowGraph::ComputeSSA(long, dart::ZoneGrowableArray<dart::Definition*>*)
  [0x00005627cfe16901] Unknown symbol
  [0x00005627cfe164c2] dart::CompilerPass::Run(dart::CompilerPassState*) const
  [0x00005627cfe1668a] dart::CompilerPass::RunPipeline(dart::CompilerPass::PipelineMode, dart::CompilerPassState*)
  [0x00005627cfe8dd5d] dart::CompileParsedFunctionHelper::Compile(dart::CompilationPipeline*)
  [0x00005627cfe8f19e] Unknown symbol
  [0x00005627cfe902d2] dart::Compiler::CompileOptimizedFunction(dart::Thread*, dart::Function const&, long)
  [0x00005627cfcae52b] dart::DRT_StackOverflow(dart::NativeArguments)
  [0x00007faab9781037] Unknown symbol
  [0x00007faab7ab77ce] Unknown symbol
  [0x00007faab7ab0ae5] Unknown symbol
  [0x00007faab7ab07dd] Unknown symbol
  [0x00007faab7ab05d9] Unknown symbol
  [0x00007faab7aaf935] Unknown symbol
  [0x00007faab7a8a45c] Unknown symbol
  [0x00007faab7aaf5f3] Unknown symbol
  [0x00007faab978153a] Unknown symbol
  [0x00005627cfad6733] dart::DartEntry::InvokeFunction(dart::Function const&, dart::Array const&, dart::Array const&, unsigned long)
  [0x00005627cfad9b51] dart::DartLibraryCalls::HandleMessage(dart::Object const&, dart::Instance const&)
  [0x00005627cfb1b12e] dart::IsolateMessageHandler::HandleMessage(dart::Message*)
  [0x00005627cfb5cb98] dart::MessageHandler::HandleMessages(dart::MonitorLocker*, bool, bool)
  [0x00005627cfb5d906] dart::MessageHandler::TaskCallback()
  [0x00005627cfcf782c] dart::ThreadPool::Worker::Loop()
  [0x00005627cfcf730b] dart::ThreadPool::Worker::Main(unsigned long)
  [0x00005627cfc386f5] Unknown symbol
-- End of DumpStackTrace

@a-siva @alexmarkov @mraleph

dart-bot pushed a commit that referenced this issue Apr 3, 2019
@aartbik
[vm/compiler] Fix incorrect labeling of stack slots
e64bd9a 
Rationale:
The "unoptimized code" representation is really a stack
machine that pushes and pops arguments. The intermediate
representation did not show the correct labeling for
"temporaries", in particular around push arguments.
This CL fixes that bug, making the human readable form
of the intermediate much easier to understand. Also,
it allows for proper stack depth computations.

First use:
Sharpen condition on OSR points a bit.
Simple control flow collections will now allow OSR.


#36421
#36409

Change-Id: I8be707af08462a22687b355b747ba85e42431ad5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98500
Commit-Queue: Aart Bik <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
dart-bot pushed a commit that referenced this issue Apr 3, 2019
@aartbik
[vm/bytecode] fixed typo in stack test
e9f5b85 
Rationale:
We should test for *empty* stack. As written, we basically
never generate OSR points for the bytecode path except for
the cases we can' handle yet :-)

#36421

Change-Id: Ie70a626f9fd24d63442e681ab120087f9d1b010b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98625
Reviewed-by: Alexander Markov <[email protected]>
Commit-Queue: Aart Bik <[email protected]>
dart-bot pushed a commit that referenced this issue Apr 10, 2019
@aartbik
[dart/vm] Support OSR on non-empty expression stack
9876e92 
Rationale:
This enables OSR even in the presence of block expressions,
which are introduced by the new CFE control-flow collections
and spread operations. Faster is better!

#36421
#36218
#36214

Change-Id: I363a0b0a5becbbd5743f8fb47ff24dcebc0e4d64
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/98744
Commit-Queue: Aart Bik <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>
@aartbik
Copy link
Contributor Author

aartbik commented Apr 10, 2019

OSR is now working in all cases, including control-flow collections that leave a non-empty expression stack in an OSR point (for both kernel and bytecode paths).

@aartbik aartbik closed this as completed Apr 10, 2019
dart-bot pushed a commit that referenced this issue Apr 18, 2019
@aartbik
[dart/vm] Reduce trip count of osr test
3cf3e4e 
Rationale:
Test was borderline timeout. The reduced tripcount
still triggers OSR consistently, but runs a lot faster.

#36421
#36218
#36214

Change-Id: I60917bb6bdd51dee6bd11d1fe5beede4753c2147
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/99783
Reviewed-by: Siva Annamalai <[email protected]>
Commit-Queue: Aart Bik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aartbik aartbik

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@aartbik