VM: [Kernel] Switch to DFE for testing -c dartk configuration.

VM: [Kernel] Switch to DFE for testing -c dartk configuration.

R=kustermann@google.com

Committed: https://github.com/dart-lang/sdk/commit/a7ffe241771861620035758a775ed885c378381a

[Vyacheslav Egorov (Google), 2017-01-31 14:48:13]

vegorov@google.com changed reviewers:
+ kustermann@google.com

[Vyacheslav Egorov (Google), 2017-01-31 14:48:14]

PTAL

[kustermann, 2017-01-31 15:39:00]

lgtm

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:19: //           dart
runtime/tools/kernel-service.dart
This should be:


    dart runtime/tools/kernel-service.dart --batch

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:22: // environment variable. When not set (or
set to 0) an ephemeral port returned
"environment variable" aren't these -D... flags to the VM instead of environment
variables?

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:265: new ContentType("application", "json",
charset: "utf-8");
Isn't there a constant for it in dart:io?

ContentType.JSON

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:269: "patchedSdk": patchedSdk.toString(),
If these can be relative, you should Platform.currentWorkingDirectory (or
whatever it's called) as well.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:275: await rs.forEach(ds.add);
Either use something like (and make DataSink be able to accept it)

  await rw.pipe(ds);

or get rid of [DataSink] and juse say

final buffer = new BytesBuffer();
await rs.ForEach(buffer.add);
buffer.takeBytes();

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:281: client.close();
await client.close()

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:289: server.listen((HttpRequest request) async
{
For a graceful shutdown:

ProcessSignal.SIGTERM.first.then((_) {
  server.close();
});

which will gracefully shutdown the server.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:305: request.response.statusCode =
HttpStatus.INTERNAL_SERVER_ERROR;
Maybe set the statusCode to be OK in the other branch to be symmetric.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:318: return new RawReceivePort()..handler =
_processLoadRequest;
maybe use 

if () {
  ....
  return null;
} else {
  return new RawReceivePort()..handler = ...;
}

It's kind of weired that one branch returns a Future and the other one a
RawReceivePort.

https://codereview.chromium.org/2668503003/diff/20001/tests/language/language...
File tests/language/language_kernel.status (right):

https://codereview.chromium.org/2668503003/diff/20001/tests/language/language...
tests/language/language_kernel.status:47: mixin_illegal_superclass_test/03:
MissingCompileTimeError
Why do these suddendly pop up?

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:375: super._equal(other) && needsDFERunner
== other.needsDFERunner;
whenever you overwrite _equal, also overwrite _buildHashCode

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2103: 
one new line is enough

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2371: _startProcess() async {
Future

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2377: _process = await
io.Process.start(executable, arguments);
You can just use Platform.executable and 'tools/dartk_wrappers/dfe_worker' here.
The reason we have a wrapper script for dartk is different and not necessary
here.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2380: final readyMsg = await
_process.stdout.transform(UTF8.decoder).transform(new LineSplitter()).first;
long line

this will potentially crash the subprocess if it prints more than one line
(because the write causes SIGPIPE / broken pipe).

The .first will cancel the subscription after the first line.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2382: assert(data[0] == 'READY');
Consider draining _process.stderr as well (maybe just using DebugLogger or
io.stderr to report it)

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2386: // TODO(floitsch): should we try to
report the stacktrace?
floitsch :)

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2738: _terminateDFEWorkers()]);
indentation

https://codereview.chromium.org/2668503003/diff/60001/tools/dartk_wrappers/df...
File tools/dartk_wrappers/dfe_worker (right):

https://codereview.chromium.org/2668503003/diff/60001/tools/dartk_wrappers/df...
tools/dartk_wrappers/dfe_worker:1: #!/usr/bin/env bash
You can get rid of this file

[siva, 2017-01-31 16:43:40]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2017-01-31 16:43:40]

I thought the general consensus was to try and use the app snapshot to see how
fast the tests would run.

[siva, 2017-01-31 16:43:42]

I thought the general consensus was to try and use the app snapshot to see how
fast the tests would run.

[Vyacheslav Egorov (Google), 2017-01-31 17:06:55]

Thanks for the comments, Martin!

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
File runtime/tools/kernel-service.dart (right):

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:19: //           dart
runtime/tools/kernel-service.dart
On 2017/01/31 15:38:59, kustermann wrote:
> This should be:
> 
> 
>     dart runtime/tools/kernel-service.dart --batch

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:19: //           dart
runtime/tools/kernel-service.dart
On 2017/01/31 15:38:59, kustermann wrote:
> This should be:
> 
> 
>     dart runtime/tools/kernel-service.dart --batch

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:22: // environment variable. When not set (or
set to 0) an ephemeral port returned
On 2017/01/31 15:38:59, kustermann wrote:
> "environment variable" aren't these -D... flags to the VM instead of
environment
> variables?

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:265: new ContentType("application", "json",
charset: "utf-8");
On 2017/01/31 15:38:59, kustermann wrote:
> Isn't there a constant for it in dart:io?
> 
> ContentType.JSON

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:269: "patchedSdk": patchedSdk.toString(),
On 2017/01/31 15:39:00, kustermann wrote:
> If these can be relative, you should Platform.currentWorkingDirectory (or
> whatever it's called) as well.

These are all absolute.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:275: await rs.forEach(ds.add);
On 2017/01/31 15:38:59, kustermann wrote:
> Either use something like (and make DataSink be able to accept it)
> 
>   await rw.pipe(ds);
> 
> or get rid of [DataSink] and juse say
> 
> final buffer = new BytesBuffer();
> await rs.ForEach(buffer.add);
> buffer.takeBytes();

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:281: client.close();
On 2017/01/31 15:38:59, kustermann wrote:
> await client.close()

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:289: server.listen((HttpRequest request) async
{
On 2017/01/31 15:38:59, kustermann wrote:
> For a graceful shutdown:
> 
> ProcessSignal.SIGTERM.first.then((_) {
>   server.close();
> });
> 
> which will gracefully shutdown the server.

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:305: request.response.statusCode =
HttpStatus.INTERNAL_SERVER_ERROR;
On 2017/01/31 15:38:59, kustermann wrote:
> Maybe set the statusCode to be OK in the other branch to be symmetric.

Done.

https://codereview.chromium.org/2668503003/diff/20001/runtime/tools/kernel-se...
runtime/tools/kernel-service.dart:318: return new RawReceivePort()..handler =
_processLoadRequest;
On 2017/01/31 15:39:00, kustermann wrote:
> maybe use 
> 
> if () {
>   ....
>   return null;
> } else {
>   return new RawReceivePort()..handler = ...;
> }
> 
> It's kind of weired that one branch returns a Future and the other one a
> RawReceivePort.

Done.

https://codereview.chromium.org/2668503003/diff/20001/tests/language/language...
File tests/language/language_kernel.status (right):

https://codereview.chromium.org/2668503003/diff/20001/tests/language/language...
tests/language/language_kernel.status:47: mixin_illegal_superclass_test/03:
MissingCompileTimeError
On 2017/01/31 15:39:00, kustermann wrote:
> Why do these suddendly pop up?

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:375: super._equal(other) && needsDFERunner
== other.needsDFERunner;
On 2017/01/31 15:39:00, kustermann wrote:
> whenever you overwrite _equal, also overwrite _buildHashCode

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2103: 
On 2017/01/31 15:39:00, kustermann wrote:
> one new line is enough

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2371: _startProcess() async {
On 2017/01/31 15:39:00, kustermann wrote:
> Future 

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2377: _process = await
io.Process.start(executable, arguments);
On 2017/01/31 15:39:00, kustermann wrote:
> You can just use Platform.executable and 'tools/dartk_wrappers/dfe_worker'
here.
> The reason we have a wrapper script for dartk is different and not necessary
> here.

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2380: final readyMsg = await
_process.stdout.transform(UTF8.decoder).transform(new LineSplitter()).first;
On 2017/01/31 15:39:00, kustermann wrote:
> long line
> 
> this will potentially crash the subprocess if it prints more than one line
> (because the write causes SIGPIPE / broken pipe).
> 
> The .first will cancel the subscription after the first line.

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2380: final readyMsg = await
_process.stdout.transform(UTF8.decoder).transform(new LineSplitter()).first;
On 2017/01/31 15:39:00, kustermann wrote:
> long line
> 
> this will potentially crash the subprocess if it prints more than one line
> (because the write causes SIGPIPE / broken pipe).
> 
> The .first will cancel the subscription after the first line.

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2382: assert(data[0] == 'READY');
On 2017/01/31 15:39:00, kustermann wrote:
> Consider draining _process.stderr as well (maybe just using DebugLogger or
> io.stderr to report it)

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2386: // TODO(floitsch): should we try to
report the stacktrace?
On 2017/01/31 15:39:00, kustermann wrote:
> floitsch :)

Done.

https://codereview.chromium.org/2668503003/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_runner.dart:2738: _terminateDFEWorkers()]);
On 2017/01/31 15:39:00, kustermann wrote:
> indentation

Done.

[Vyacheslav Egorov (Google), 2017-01-31 17:18:55]

AppJIT snapshot is certainly an important piece that will make front-end usable
for the end users. 

However we now know that app-jit snapshot gives us only around 2x improvement
for the cycle time. This means that running test suite through --dfe will still
be around 5x slower than running test suite in batch mode dartk.

This CL minimizes the performance hit we are taking on test 
suite cycle time to 2x and makes the switch from Dartk to --dfe feasible. 

Given that some of us already spend considerable amounts of time during the day
running -c dartk configuration this CL seems crucial.

We weighted complexity of this CL (none) vs benefits (a lot of saved engineering
time) and decided to go forward with it.  

As mentioned before we will remove batch mode as soon as cycle time is within
reasonable bounds.

[Vyacheslav Egorov (Google), 2017-01-31 17:25:06]

Description was changed from

==========
VM: [Kernel] Switch to DFE for testing -c dartk configuration.
==========

to

==========
VM: [Kernel] Switch to DFE for testing -c dartk configuration.

R=kustermann@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a7ffe241771861620035758a775ed885c378381a
==========

[Vyacheslav Egorov (Google), 2017-01-31 17:25:07]

Committed patchset #5 (id:80001) manually as
a7ffe241771861620035758a775ed885c378381a (presubmit successful).

[hausner, 2017-01-31 17:27:04]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2017-01-31 17:27:04]

I still think this is the wrong approach to reduce testing time.

1) Requiring two processes to run each test makes things flaky, especially on
the bots.

2) We don't see the actual performance of the dart front end. If we experienced
it every day, we'd push harder to improve runtimes.