Skip to content

Restore batch mode compiler support for dartk and dartkp configurations #31585

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
3 tasks done
mraleph opened this issue Dec 8, 2017 · 5 comments
Closed
3 tasks done

Restore batch mode compiler support for dartk and dartkp configurations #31585

mraleph opened this issue Dec 8, 2017 · 5 comments
Assignees
@mkustermann
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P0 A serious issue requiring immediate resolution

Comments

@mraleph
Copy link
Member

mraleph commented Dec 8, 2017 •
edited by mkustermann
Loading

Currently we use kernel isolate in all dartk and non-strong dartkp modes. This allows us to compile to Kernel transparently, but leads to a lot of issues and bad performance of tests especially in simulated architectures (40x slower tests on SIMARM+dartk configuration) and precompiler mode.

Plan:

  • Restore pure batch mode support: run persistent compiler runner in background, compile to dill files on disk, pass those to the VM.
  • Switch dartkp on all architectures to this mode.
  • Switch dartk on SIM* architectures to use this mode.

Some tests that need to perform on the fly compilation (Isolate and reload tests) would continue to use --dfe for now.
@mraleph mraleph added this to the 2.0-alpha milestone Dec 8, 2017
@mraleph mraleph added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. area-kernel P0 A serious issue requiring immediate resolution labels Dec 8, 2017
@mraleph
Copy link
Member Author

mraleph commented Dec 8, 2017

/cc @alexmarkov @a-siva

@mraleph mraleph mentioned this issue Dec 8, 2017
Closed
@mkustermann
Copy link
Member

Enabled in -cdartkp --strong in 8c38a41

@alan-knight
Copy link
Contributor

The CL caused regressions, talked to Martin and reverted it in https://dart.googlesource.com/sdk/+/af0b27480a30bea4c87ab34b8d4f712319fcd3f5

@mkustermann
Copy link
Member

A re-land of that CL is in-progress and more CLs are in preparation.

whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann
Reland "Use batch mode compilation for -cdartkp --strong"
317c55f 
Preliminary testing shows, this increases performance by around 20%. The
main benefit is by re-using a warmed-up VM and not start one from
scratch for every compilation.

Going forward we can do more optimizations, e.g. reading the platform
dill file only once into memory (instead of repeatedly) ...

  => This requires us using the new state-full IKG compiler.

Issue #31585

Change-Id: I2d3448783fc118611baf4671187a897227a65a6c
Reviewed-on: https://dart-review.googlesource.com/28400
Reviewed-by: Vyacheslav Egorov <[email protected]>
whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann
Use batch mode compilation for normal -cdartkp
d1ed915 
Issue #31585

Change-Id: I70790a33cfbfbc7c2c48c6e77074f955d6de7e01
Reviewed-on: https://dart-review.googlesource.com/28280
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann
Only compile to kernel for precompilation for -cdartkp or -dartkp --s…
891e591 
…trong

Issue #31585

Change-Id: Ife12640a61513a0312fcfc3713f1a44f69c1ac9a
Reviewed-on: https://dart-review.googlesource.com/28621
Reviewed-by: Vyacheslav Egorov <[email protected]>
whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann
Add support for running simdbc64 in batch mode
a11d7a1 
This makes isolate tests fail, since we no longer run from "source" (or
rather use the kernel-isolate to to "source -> dill" for us).

The special vm/cc suite will continue to be run via the kerne-isolate, so we
have the coverage for these (which probably include reload tests).

Issue #31585

Change-Id: I51bd2f9345d650b4ff2a98aa1c8365c765e0d013
Reviewed-on: https://dart-review.googlesource.com/28722
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann @mraleph
Ensure we pass the kernel file to dart_bootstrap when using cfe (in b…
ba8014d 
…atch and non-batch mode)

Issue #31585

Change-Id: Ibca196103b868177afa0ab9e15c913bbea2474ed
Reviewed-on: https://dart-review.googlesource.com/28760
Reviewed-by: Vyacheslav Egorov <[email protected]>
whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann
Treat compile-time errors from the batch mode runner correctly
717ed65 
The batch runner uses an exitCode of 1 to signal compile-time errors,
which we need to support in the VMKernelCompilationCommandOutput.

Furthermore allow running the output of a dart -> kernel compilation on
the VM even though we expect an compile-time error (sometimes the
frontend has bugs and doesn't emit the compile-time errors and we need
to run the VM to get them)

Issue #31585

Change-Id: I8b4c34557dbf3de487247d75b02777bacbd452c1
Reviewed-on: https://dart-review.googlesource.com/28642
Reviewed-by: Martin Kustermann <[email protected]>
whesse pushed a commit that referenced this issue Dec 12, 2017
@mkustermann
Update status file after changing -cdartkp to use batch mode for kern…
962c18b 
…el compilation

Issue #31585

Change-Id: I0e4c57f57efa39375728f6a718fb675c6f97cd94
Reviewed-on: https://dart-review.googlesource.com/28820
Reviewed-by: Martin Kustermann <[email protected]>
whesse pushed a commit that referenced this issue Dec 13, 2017
@mkustermann
[VM] Allow run_vm_tests to load like we do in normal dart
6e34fa1 
Our build files use non-appjit snapshots for kernel-service for
certain architectures, e.g. simulators.

Issue #31585

Change-Id: I5162b8fe266f97710c50c347ae20b53cd66cf75d
Reviewed-on: https://dart-review.googlesource.com/29240
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Vyacheslav Egorov <[email protected]>
whesse pushed a commit that referenced this issue Dec 14, 2017
@mkustermann
Enable dartk batch running of simarm/simarm64/simdbc
36c3f97 
Issue #31585

Change-Id: I19f9507e0f2285c6b2dd29ca5991487dfb99fdb8
Reviewed-on: https://dart-review.googlesource.com/29581
Reviewed-by: Vyacheslav Egorov <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
whesse pushed a commit that referenced this issue Dec 14, 2017
@mkustermann
Do not use pkg/vm wrapper script when running fully-linked kernel files
0b8d270 
Issue #31585

Change-Id: I5211c7d8fb16ea60509916bf525cc36ac05dd3a9
Reviewed-on: https://dart-review.googlesource.com/29591
Reviewed-by: Vyacheslav Egorov <[email protected]>
Commit-Queue: Martin Kustermann <[email protected]>
@mkustermann
Copy link
Member

So this has been working for a while, I've kept the bug open so far for the following things:

  • Make use of a new IKG api to make the batch compiler faster, there's a todo in pkg/vm/bin/gen_kernel.dart to do that (this is purely a performance optimization)
  • Make isolate tests work (currently we skip them). The issue with spawn function is tracked at Make Isolate.spawn work without loading from source in JIT mode #31698 and spawning from uri simply won't work for batch mode. The only thing we could consider is white-listing a few spawn-uri-using-tests to use the kernel isolate. But this seems to me too complicated/hackisch and we don't gain a lot from it. So I have no plans of implementing this.

Will close this issue therefore.

@kmillikin kmillikin added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel and removed legacy-area-front-end Legacy: Use area-dart-model instead. labels Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mkustermann mkustermann

Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P0 A serious issue requiring immediate resolution
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mraleph @kmillikin @alan-knight @mkustermann