Proposal: Add YAML file with VM patch file information.

Add YAML file with VM patch file information.

R=vegorov@google.com

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

[ahe, 2017-02-15 14:22:33]

ahe@google.com changed reviewers:
+ asiva@google.com, vegorov@google.com, zra@google.com

[ahe, 2017-02-15 14:22:34]

Here's a starting point for a discussion.

I'm starting implementing reading Dart VM patch files in Fasta, and I'd like to
move away from having the VM's sources defined in .gypi files. 

Here's my proposal:

Let's replace the gypi files with a YAML file, for example,
runtime/vm/libraries.yaml as I've included in this CL.

Compared to JSON, YAML is easy to read and supports comments. It's easy to
convert YAML to JSON. Compared to gypi, YAML has nothing to do with GYP.

I imagine that we also generate a libraries.json file that can be easily read
from python and Dart (without requiring additional dependencies - python has
builtin support for JSON since 2.6).

[zra, 2017-02-15 16:41:42]

I think I'm missing a lot of context here, but here are my initial thoughts.

We probably have to keep around the gypi files as long as we are supporting
Dartium.

Otherwise, the GN build can be happy with whatever format the lists are in as
long as there is a Python script like this one:

https://github.com/dart-lang/sdk/blob/master/tools/gypi_to_gn.py

invoked from:

https://github.com/dart-lang/sdk/blob/master/runtime/bin/gypi_contents.gni

Invoking Dart scripts as part of build file generation is not a dependency we
want to introduce, however. Only after the build files have been generated, and
the VM has been built, can we invoke Dart scripts during the build.

[ahe, 2017-02-15 21:56:42]

On 2017/02/15 16:41:42, zra wrote:
> I think I'm missing a lot of context here, but here are my initial thoughts.
> 
> We probably have to keep around the gypi files as long as we are supporting
> Dartium.
> 
> Otherwise, the GN build can be happy with whatever format the lists are in as
> long as there is a Python script like this one:
> 
> https://github.com/dart-lang/sdk/blob/master/tools/gypi_to_gn.py
> 
> invoked from:
> 
> https://github.com/dart-lang/sdk/blob/master/runtime/bin/gypi_contents.gni
> 
> Invoking Dart scripts as part of build file generation is not a dependency we
> want to introduce, however. Only after the build files have been generated,
and
> the VM has been built, can we invoke Dart scripts during the build.

Thank you, Zach. I found those scripts and was able to use them as a basis for
the YAML file.

It's a good point about Dartium support, this is not something I had thought
about. I have experience with GYP, and I should be able to change the GYP-based
build to whatever format we settle on.

Perhaps it would be safer to pick JSON over YAML, YAML is a bit easier to read,
but JSON can be most easily read by our tools. I would really like to get away
from gypi to avoid having to create a Python dictionary parser in Dart.

[ahe, 2017-02-15 23:46:29]

And about context:

In Fasta I'd like to be able to read SDK libraries and patch files, preferably
from the same source as our existing tools use.

However, the gypi files aren't really suitable for this, and the platform files
in sdk/lib aren't super-awesome either, as they require another parser.

In Fasta, I'd really like to read the information from JSON. I chose to use YAML
as someone recently pointed out that it's easy to translate to JSON, and it
supports comments (and is easier to read and write).

I hope this is enough context, if not, please let me know.

[ahe, 2017-02-17 14:24:53]

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

[ahe, 2017-02-17 14:24:54]

+kustermann

[ahe, 2017-02-20 14:45:54]

Slava, could you take a look, I'd like to land this as I need to read the
information from somewhere.

I think we can iterate on the exact format later.

I've created this bug https://github.com/dart-lang/sdk/issues/28836 to track
that we settle on a shared format and actually remove the ones we don't use.

[zra, 2017-02-20 17:17:01]

What targets are going to require JSON files?

At least Fuchsia will not have a checked-in SDK to generate JSON files like
this. I'm also not sure that CheckedInSdkExecutable will work in the Flutter
build.

[zra, 2017-02-20 17:24:02]

Also, the generate_buildfiles.py hook should probably be last in case it's going
to depend on any files you need to generate in earlier hooks.

[Vyacheslav Egorov (Google), 2017-02-20 19:30:30]

Why don't we make fasta just ingest YAML? I understand that we might want to
minimize amount of dependencies but in reality we always have yaml checked out
into SDK repo anyway. 

Adding hooks to generate JSON out of YAML with a help of two programs (Python &
Dart) seems just a bit too convoluted compared to just ingesting YAML directly
in fasta. 

This also avoids touching DEPS and as such avoids necessity to reach any
consensus regarding the format of the file.

[ahe, 2017-02-21 09:39:04]

On 2017/02/20 19:30:30, Vyacheslav Egorov (Google) wrote:
> Why don't we make fasta just ingest YAML? I understand that we might want to
> minimize amount of dependencies but in reality we always have yaml checked out
> into SDK repo anyway. 

I don't want to add dependencies to fasta because it makes it slower. For
example, this program:

import 'dart:io';

import 'dart:convert';

import 'package:yaml/yaml.dart' show loadYaml;

main(List<String> arguments) async {
  loadYaml(await new File(arguments.single).readAsStringSync());
}

Is about 57% slower than this program:

import 'dart:io';

import 'dart:convert';

main(List<String> arguments) async {
  JSON.decode(await new File(arguments.single).readAsStringSync());
}

Part of the reason for that is that package:yaml imports other packages that I'm
deliberately trying to avoid using, for example, package:collection,
package:path, and package:charcode. These packages are designed for developer
productivity and performance in large long-running apps. Sadly, when you're
building a short-running performance sensitive program like fasta, you don't
have the luxury to use any package you'd like.

[ahe, 2017-02-21 09:50:51]

On 2017/02/20 17:17:01, zra wrote:
> What targets are going to require JSON files?

My long-term goal is to replace all the .gypi files by these files. In order to
do so, I need to provide a GYP and GN alternative, which means a Python program.
Python doesn't natively understand YAML, so I prefer to have it read JSON.

Also, dart2js, analyzer, and Fasta should be able to read either the JSON file
or the YAML file. As long as my goal is to make Fasta fast, I believe I can't
afford to read other file types than Kernel, Dart sources, and JSON.

[ahe, 2017-02-21 09:52:03]

Please take another look.

[Vyacheslav Egorov (Google), 2017-02-21 10:14:40]

We need to parse YAML file once and only if we are compiling SDK. So while 50%
might look scary, in absolute numbers it's something like 100ms.

$ time dart fast.dart input.json
dart fast.dart input.json  0.09s user 0.01s system 109% cpu 0.099 total
$ time dart slow.dart input.yaml
dart slow.dart input.yaml  0.19s user 0.04s system 116% cpu 0.193 total

Furthermore the difference is even less with a snapshot:

$ dart --snapshot=slow.app-jit --snapshot-kind=app-jit slow.dart input.yaml
$ time dart slow.app-jit input.yaml
dart slow.app-jit input.yaml  0.03s user 0.01s system 122% cpu 0.037 total
$ dart --snapshot=fast.app-jit --snapshot-kind=app-jit fast.dart input.json
$ time dart fast.app-jit input.json
dart fast.app-jit input.json  0.02s user 0.01s system 117% cpu 0.030 total

Do you think this level of overhead (paid once!) warrants having duplicated
files in the SDK source tree?

[ahe, 2017-02-21 10:22:23]

On 2017/02/21 10:14:40, Vyacheslav Egorov (Google) wrote:
> Do you think this level of overhead (paid once!) warrants having duplicated
> files in the SDK source tree?

Yes. The overhead isn't paid once, dependencies always grow. For this reason,
you need to think about the complete cost each time you consider adding a new
dependency. People have a tendency to add more and more dependencies and blame
others for startup performance.

[Vyacheslav Egorov (Google), 2017-02-21 11:48:27]

LGTM

please put reference to https://github.com/dart-lang/sdk/issues/28836 into both
YAML files so that we don't loose track of this particular bug.

[ahe, 2017-02-21 12:54:25]

On 2017/02/21 11:48:27, Vyacheslav Egorov (Google) wrote:
> LGTM
> 
> please put reference to https://github.com/dart-lang/sdk/issues/28836 into
both
> YAML files so that we don't loose track of this particular bug.

Done.

Thank you. I'd appreciate if anyone has comment or concerns, that they mention
them in the issue: https://github.com/dart-lang/sdk/issues/28836.

[ahe, 2017-02-21 12:56:34]

Description was changed from

==========
Add YAML file with VM patch file information.
==========

to

==========
Add YAML file with VM patch file information.

R=vegorov@google.com

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

[ahe, 2017-02-21 12:56:36]

Committed patchset #4 (id:60001) manually as
b13b8d872aee14e2eba75ce16845ae6d5fbe5c6d (presubmit successful).