[kernel] Delete most of the AST

[kernel] Delete most of the AST

Now basically only Program (with ~no children) persists.

BUG=
R=kmillikin@google.com

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

[jensj, 2017-06-09 08:46:31]

jensj@google.com changed reviewers:
+ kmillikin@google.com

[jensj, 2017-06-09 08:46:31]

This is NOT ready for commit. Service tests fail. But any comments would be
appreciated.

Notice that I've added a "table of content" with fixed-size integers at the end
of the binary so there's "random access" to the libraries.

[jensj, 2017-06-12 07:24:20]

Service tests are now passing.
The list of debugable positions is generated lazily, i.e. if it's not used we
don't generate it at all, when we first want it (or a position of yield points,
also for debugging) we generate it by reading the entire binary, caching the
result.

Whether that can be made smarter, I don't know. We do this for one script at a
time, i.e. we only need the debugable positions for a single file, but I think
that - because of mixins and whatnot - the data from that file can basically be
in every or any library.

[jensj, 2017-06-20 07:23:01]

Description was changed from

==========
[kernel] Delete most on the AST

Now basically only Program (with ~no children) persits.

BUG=
==========

to

==========
[kernel] Delete most of the AST

Now basically only Program (with ~no children) persists.

BUG=
==========

[jensj, 2017-06-21 12:11:54]

As a side note I might add that in precompilation mode every "real"
tokenposition is translated into a tokenposition with the value of the line it
corresponds to (when creating the .S file with dart_bootstrap). When needing a
stacktrace in precompiled mode it can thus just use the tokenposition as the
linenumber.

Because -cdartkp currently doesn't work at  all (see fyi waterfall) I cannot
really test this change with it, but my guess would be that it's no worse than
before.

[jensj, 2017-06-22 08:21:27]

Mind all the rebases.

While the end-of-file-TOC is probably not ideal either I think it's currently
the only way to go. Once the blob is split and put into vm memory etc we can
probably do something else.

[jensj, 2017-06-23 07:49:56]

https://github.com/dart-lang/sdk/commit/8b7af73daafedacf0f239d17096703420937af69

The get main closure thingy hack is no longer needed :)

[jensj, 2017-06-29 09:49:02]

ping.

[Kevin Millikin (Google), 2017-06-29 12:27:37]

LGTM to land it, with comments addressed.

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/binary.md#n...
pkg/kernel/binary.md:135: UInt32 mainMethodReference; // This is a
ProcedureReference with a fixed-size integer.
Is there any reason to have two different indicators of the mainMethod?

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_from_binary.dart:275: throw "Expected number of
libraries to match!";
Even if you're just throwing a string and not some structured exception, make it
informative.  Something like:

'Malformed binary: the table of contents indicates there are
$numberOfLibrariesCheck libraries but the binary contains $numberOfLibraries.'

Perhaps we can also throw if there is anything else (add a method to the reader
that reports if it is at the end of the stream).

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_to_binary.dart:78: void writeMagicWord(int value) {
Why replace readMagicWord with readUInt32 but not similarly for writeMagicWord?

I like the reading and writing code to parallel each other as much as possible.

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_to_binary.dart:197: writeFixedProgramFooter(program,
program.libraries);
Adopt a uniform terminology.  In the comments this is described as a "table of
contents".  I think of it as an "index" actually, because the table of contents
usually occurs at the beginning.

If it's actually a fixed program footer, use that terminology in the comments. 
If it's actually a table of contents, use that terminology in the identifier
names.

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/bootstrap.cc
File runtime/vm/bootstrap.cc (right):

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/bootstrap.c...
runtime/vm/bootstrap.cc:338: const String& kernel_name =
reader.DartSymbolForLib(j);
DartSymbolForLib ==> LibraryUri or something like that.

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:3778: Array& yields =
Array::Handle(script.yield_positions());
It can still be const, can't it?

[jensj, 2017-06-30 05:51:17]

Thanks.

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/binary.md#n...
pkg/kernel/binary.md:135: UInt32 mainMethodReference; // This is a
ProcedureReference with a fixed-size integer.
On 2017/06/29 12:27:36, Kevin Millikin (Google) wrote:
> Is there any reason to have two different indicators of the mainMethod?

None other than I didn't want to change the existing one.

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_from_binary.dart:275: throw "Expected number of
libraries to match!";
On 2017/06/29 12:27:36, Kevin Millikin (Google) wrote:
> Even if you're just throwing a string and not some structured exception, make
it
> informative.  Something like:
> 
> 'Malformed binary: the table of contents indicates there are
> $numberOfLibrariesCheck libraries but the binary contains $numberOfLibraries.'
> 
> Perhaps we can also throw if there is anything else (add a method to the
reader
> that reports if it is at the end of the stream).

I'm not sure about throwing if we're not at the end --- readProgram indicates
that we can have several programs in the same dill file, though I'm not sure
when that's applicable?

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_to_binary.dart:78: void writeMagicWord(int value) {
On 2017/06/29 12:27:36, Kevin Millikin (Google) wrote:
> Why replace readMagicWord with readUInt32 but not similarly for
writeMagicWord?
> 
> I like the reading and writing code to parallel each other as much as
possible.

Done.

https://codereview.chromium.org/2931773005/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_to_binary.dart:197: writeFixedProgramFooter(program,
program.libraries);
On 2017/06/29 12:27:36, Kevin Millikin (Google) wrote:
> Adopt a uniform terminology.  In the comments this is described as a "table of
> contents".  I think of it as an "index" actually, because the table of
contents
> usually occurs at the beginning.
> 
> If it's actually a fixed program footer, use that terminology in the comments.

> If it's actually a table of contents, use that terminology in the identifier
> names.

I have uniformly renamed to "program index".

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/bootstrap.cc
File runtime/vm/bootstrap.cc (right):

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/bootstrap.c...
runtime/vm/bootstrap.cc:338: const String& kernel_name =
reader.DartSymbolForLib(j);
On 2017/06/29 12:27:36, Kevin Millikin (Google) wrote:
> DartSymbolForLib ==> LibraryUri or something like that.

Done.

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2931773005/diff/140001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:3778: Array& yields =
Array::Handle(script.yield_positions());
On 2017/06/29 12:27:36, Kevin Millikin (Google) wrote:
> It can still be const, can't it?

Yes.
In fact, this file no longer needs to change at all.

[jensj, 2017-06-30 05:55:14]

Description was changed from

==========
[kernel] Delete most of the AST

Now basically only Program (with ~no children) persists.

BUG=
==========

to

==========
[kernel] Delete most of the AST

Now basically only Program (with ~no children) persists.

BUG=
R=kmillikin@google.com

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

[jensj, 2017-06-30 05:55:15]

Committed patchset #9 (id:160001) manually as
bbb463ac20b954d93f9cf65cab43277b7742efa2 (presubmit successful).