Move the Kernel canonical name table into the VM's heap

Move the Kernel canonical name table into the VM's heap

The canonical name table is copied into a typed data array in the VM's
heap.  The encoding is the same as in the binary except that the
integer indexes are fixed-size.

Canonical names are now integer indexes instead of objects allocated
in the C++ heap.

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

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

[Kevin Millikin (Google), 2017-05-02 23:03:06]

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

[jensj, 2017-05-03 06:08:17]

lgtm

[Vyacheslav Egorov (Google), 2017-05-03 06:28:48]

lgtm

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:381: intptr_t canonical_name() { return canonical_name_; }
With all these different indices I wonder if it worth actually having some
wrappers for type safety?

e.g. 

class CanonicalNameIndex {
private: 
  intptr_t index_;
};

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:63: intptr_t target = builder_->ReadUInt()
- 1;
might be worth having a helper called ReadCanonicalNameReference similar to one
in the reader. 

otherwise -1 is way too confusing.

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:422: intptr_t target = ReadUInt() - 1;
ditto

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:103: RawClass*
BuildingTranslationHelper::LookupClassByKernelClass(intptr_t klass) {
I think we *do* need need named type for these indices, otherwise it is hard to
understand whether it is a CanonicalName or maybe a String index - just by
looking at the signature.

[Kevin Millikin (Google), 2017-05-03 17:27:18]

Description was changed from

==========
Move the Kernel canonical name table into the VM's heap

The canonical name table is copied into a typed data array in the VM's
heap.  The encoding is the same as in the binary except that the
integer indexes are fixed-size.

Canonical names are now integer indexes instead of objects allocated
in the C++ heap.

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

to

==========
Move the Kernel canonical name table into the VM's heap

The canonical name table is copied into a typed data array in the VM's
heap.  The encoding is the same as in the binary except that the
integer indexes are fixed-size.

Canonical names are now integer indexes instead of objects allocated
in the C++ heap.

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

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

[Kevin Millikin (Google), 2017-05-03 17:27:20]

Committed patchset #2 (id:20001) manually as
514281b8a744222a3d1b9d196fac1b4b60b3b28e (presubmit successful).

[Kevin Millikin (Google), 2017-05-03 17:43:52]

On 2017/05/03 06:28:48, Vyacheslav Egorov (Google) wrote:
> lgtm
> 
> https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel.h
> File runtime/vm/kernel.h (right):
> 
>
https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel.h#new...
> runtime/vm/kernel.h:381: intptr_t canonical_name() { return canonical_name_; }
> With all these different indices I wonder if it worth actually having some
> wrappers for type safety?
> 
> e.g. 
> 
> class CanonicalNameIndex {
> private: 
>   intptr_t index_;
> };
> 
>
https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_binar...
> File runtime/vm/kernel_binary_flowgraph.cc (right):
> 
>
https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_binar...
> runtime/vm/kernel_binary_flowgraph.cc:63: intptr_t target =
builder_->ReadUInt()
> - 1;
> might be worth having a helper called ReadCanonicalNameReference similar to
one
> in the reader. 
> 
> otherwise -1 is way too confusing.
> 
>
https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_binar...
> runtime/vm/kernel_binary_flowgraph.cc:422: intptr_t target = ReadUInt() - 1;
> ditto
> 
>
https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_reade...
> File runtime/vm/kernel_reader.cc (right):
> 
>
https://codereview.chromium.org/2853423002/diff/20001/runtime/vm/kernel_reade...
> runtime/vm/kernel_reader.cc:103: RawClass*
> BuildingTranslationHelper::LookupClassByKernelClass(intptr_t klass) {
> I think we *do* need need named type for these indices, otherwise it is hard
to
> understand whether it is a CanonicalName or maybe a String index - just by
> looking at the signature.

Thanks for the review.

Adding classes for string indexes and canonical name indexes will definitely be
an improvement.  I will do that as a separate change.