Restructure the Kernel string table.

Restructure the Kernel string table.

Instead of a list of strings given by their length and UTF-8 encoding,
restructure the string table to consist of a list of string ending
offsets followed by a blob of UTF-8 encoded strings without lengths.

This is a step toward copying the string blob into the VM's heap and
building a heap-allocated structure to give random access to them.

BUG=
R=asgerf@google.com, jensj@google.com

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

[Kevin Millikin (Google), 2017-04-04 09:26:43]

kmillikin@google.com changed reviewers:
+ asgerf@google.com, jensj@google.com, kustermann@google.com

[asgerf, 2017-04-04 09:33:36]

LGTM

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

https://codereview.chromium.org/2790073004/diff/1/pkg/kernel/binary.md#newcode67
pkg/kernel/binary.md:67: UInt index; // Index into the Program's .strings.
The '.' looks out of place.

It looks like it should say either "Program's strings" or "Program.strings".

[jensj, 2017-04-04 09:42:17]

lgtm with comments.

https://codereview.chromium.org/2790073004/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2790073004/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:126: void readSourceUriTable() {
This method isn't called anymore is it? It doesn't seem to match the description
of the binary either so that's probably a good thing.

https://codereview.chromium.org/2790073004/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2790073004/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:324: intptr_t base = ReaderOffset();
One could also save the base as a class variable and return
"string_offsets_[index]" + base instead of updating it here... Probably depends
on how many strings we use whether it's worth it though...

https://codereview.chromium.org/2790073004/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:358: end_offset = ReadUInt();
Should this be "+=", or should the SkipBytes be called inside the loop?

[jensj, 2017-04-04 09:52:59]

https://codereview.chromium.org/2790073004/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2790073004/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:358: end_offset = ReadUInt();
On 2017/04/04 09:42:16, jensj wrote:
> Should this be "+=", or should the SkipBytes be called inside the loop?

Ignore that comment... The string table starts with a list of offsets, we only
want the last one so we can skip the entire payload.

[Kevin Millikin (Google), 2017-04-04 11:02:59]

Thanks for the review.

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

https://codereview.chromium.org/2790073004/diff/1/pkg/kernel/binary.md#newcode67
pkg/kernel/binary.md:67: UInt index; // Index into the Program's .strings.
On 2017/04/04 09:33:36, asgerf wrote:
> The '.' looks out of place.
> 
> It looks like it should say either "Program's strings" or "Program.strings".

I'll just say Program's strings.

I was going for something like: // Index into p.strings, where p is the
enclosing Program.

https://codereview.chromium.org/2790073004/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2790073004/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:126: void readSourceUriTable() {
On 2017/04/04 09:42:16, jensj wrote:
> This method isn't called anymore is it? It doesn't seem to match the
description
> of the binary either so that's probably a good thing.

You are right.  I've removed it.

[Kevin Millikin (Google), 2017-04-04 11:04:34]

Description was changed from

==========
Restructure the Kernel string table.

Instead of a list of strings given by their length and UTF-8 encoding,
restructure the string table to consist of a list of string ending
offsets followed by a blob of UTF-8 encoded strings without lengths.

This is a step toward copying the string blob into the VM's heap and
building a heap-allocated structure to give random access to them.

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

to

==========
Restructure the Kernel string table.

Instead of a list of strings given by their length and UTF-8 encoding,
restructure the string table to consist of a list of string ending
offsets followed by a blob of UTF-8 encoded strings without lengths.

This is a step toward copying the string blob into the VM's heap and
building a heap-allocated structure to give random access to them.

BUG=
R=asgerf@google.com, jensj@google.com

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

[Kevin Millikin (Google), 2017-04-04 11:04:35]

Committed patchset #2 (id:20001) manually as
04cb9809d10f1fa72da58a89a53c1a51a1e6f60e (presubmit successful).