[kernel] Change how TypeParameterType is calculated.

[kernel] Change how TypeParameterType is calculated.

Prior to this CL we carried around information about the containing class
and member, both of which was fetched by reading out-of-line in the binary
(i.e. while reading the current member, start reading something from the
parent member etc).
It had also required the introduction of extra fields in the kernel
binary file (dill file).

This CL cleans that up, by
a) Setting type parameters on functions as needed (in kernel_reader.cc)
b) Using the VM Class and VM Function to get the required information
   (with a above the information is all available).
   (in kernel_binary_flowgraph.cc.) This means that
c) We don't have to read the binary out-of-line (for TypeParameterType
   to work at least), and that
d) We can remove the previously introduced extra fields from the
   kernel binary file (dill file).

R=dmitryas@google.com, kmillikin@google.com

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

[jensj, 2017-07-05 13:33:55]

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

[jensj, 2017-07-05 13:33:56]

So I was actually moving method bodies to the VMs heap when I ran into this
problem of the streaming jumping around to read its parents code or its classes
code etc.

This fixes some of it (though there's probably still other issues of the same
type left). It especially fixes a critique point that Kevin had about the
introduction of the binary fields that I here remove again.

It passes all tests when running locally with
./pkg/front_end/tool/fasta testing fasta_min

I will start a debug-mode test (python tools/test.py -cdartk) before I leave
today.

I've included Kevin as reviewer (despite being told to set Dima as the reviewer
for my stuff this week) as I would like his input as well. That being said,
Dima, I would also very much like your input :)

I will base my further work on putting the method bodies into the VMs heap on
this patch.

[Dmitry Stefantsov, 2017-07-07 09:23:54]

I'm not familiar with this part of VM, but I did my best to study what was
changed, and and it LGTM.

I only have one small question regarding code writing conventions in VM that I
was told once.  Please, find it below.

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

https://codereview.chromium.org/2973633002/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.cc:1547: TypeArguments& class_types =
It looks like [class_types] is not changed anywhere.  Shouldn't it be `const` in
this case?

Same question for other local variables here and in other procedures.

[sivachandra, 2017-07-28 22:50:24]

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

[sivachandra, 2017-07-28 22:50:25]

A drive by comment as I am trying to understand the code in this area.

https://codereview.chromium.org/2973633002/diff/1/runtime/vm/kernel_reader.cc
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2973633002/diff/1/runtime/vm/kernel_reader.cc...
runtime/vm/kernel_reader.cc:642: if (type_parameter_count > 0) {
nit: Invert the condition perhaps?

ASSERT(type_parameter_count >= 0);
if (type_parameter_count == 0) {
  return;
}

[Kevin Millikin (Google), 2017-08-08 14:08:52]

LGTM when the comments below are addressed.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1574: if (class_types.Length() >
parameter_index) {
It doesn't really matter but I find this and the comparisons below easier to
think about if they are flipped around (parameter_index < class_types.Length()),
which is the way I think of the comparison we need to make indexing into
class_types safe.

I suggest flipping them.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1582: if
(active_class_->MemberIsFactoryProcedure()) {
It's kind of hard to reason about it, but I think that MemberIsFactoryProcedure
and MemberIsProcedure cannot both be true.  If that is the case then the code is
clearer if it's written:

if (...MemberIsFactoryProcedure()) {
  ...
} else if (...MemberIsProcedure()) {
  ...
}

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1608: bool member_is_procedure =
active_class_->MemberIsProcedure();
It seems weird to name it when there is only one use.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1625: for (TypeParameterScope* scope =
type_parameter_scope_; scope != NULL;
This code just return DynamicType unless the binary is not well-formed.  It's
confusing to use the loop, just check that the index is in range of
scope->outer_parameters_count() + scope->parameters_count().  If it is return
DynamicType and if it's not longjmp out of here.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:60: summed_outer_parameters_count_ = 0;
summed_ seems like a noise word and I wouldn't make it plural if I was using
count, so: outer_parameter_count_.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:673: set_on->set_type_parameters(type_parameters);
Having two different template instantiations of this function seems unnecessary.
 I suggest passing a const Object& and using something like:

ASSERT(set_on.IsClass() || set_on.IsFunction());
if (set_on.IsClass()) {
  Class::Cast(set_on).set_type_parameters...
} else {
  Function::Cast(set_on).set...
}

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:196: if (member == NULL) return false;
This doesn't seem safe.  I think if there is no member it's a mistake to ask if
the member is a procedure.  You might interpret "false" as something like "it's
a Field" and that is a latent bug.

Can we assert member != NULL?

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:206: if (member == NULL) return false;
Likewise, I'm not sure this is safe.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:210: intptr_t MemberTypeParameterCount(Zone* zone) {
We don't need all this code in the header file, do we?

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:217: } else if (function_kind ==
RawFunction::kMethodExtractor) {
member->IsMethodExtractor()

[jensj, 2017-08-09 08:39:47]

If there are no objections I will land soon.

https://codereview.chromium.org/2973633002/diff/1/runtime/vm/kernel_reader.cc
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2973633002/diff/1/runtime/vm/kernel_reader.cc...
runtime/vm/kernel_reader.cc:642: if (type_parameter_count > 0) {
On 2017/07/28 22:50:24, sivachandra wrote:
> nit: Invert the condition perhaps?
> 
> ASSERT(type_parameter_count >= 0);
> if (type_parameter_count == 0) {
>   return;
> }

Done.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1574: if (class_types.Length() >
parameter_index) {
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> It doesn't really matter but I find this and the comparisons below easier to
> think about if they are flipped around (parameter_index <
class_types.Length()),
> which is the way I think of the comparison we need to make indexing into
> class_types safe.
> 
> I suggest flipping them.

Done.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1582: if
(active_class_->MemberIsFactoryProcedure()) {
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> It's kind of hard to reason about it, but I think that
MemberIsFactoryProcedure
> and MemberIsProcedure cannot both be true.  If that is the case then the code
is
> clearer if it's written:
> 
> if (...MemberIsFactoryProcedure()) {
>   ...
> } else if (...MemberIsProcedure()) {
>   ...
> }

Actually, a FactoryProcedure is always a procedure:

  bool MemberIsProcedure() {
    if (member == NULL) return false;
    RawFunction::Kind function_kind = member->kind();
    return [...] ||
           member->IsFactory();
  }

  bool MemberIsFactoryProcedure() {
    if (member == NULL) return false;
    return member->IsFactory();
  }

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1608: bool member_is_procedure =
active_class_->MemberIsProcedure();
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> It seems weird to name it when there is only one use.

Fixed.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:1625: for (TypeParameterScope* scope =
type_parameter_scope_; scope != NULL;
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> This code just return DynamicType unless the binary is not well-formed.  It's
> confusing to use the loop, just check that the index is in range of
> scope->outer_parameters_count() + scope->parameters_count().  If it is return
> DynamicType and if it's not longjmp out of here.

While I'm not quite sure what you mean by longjmp out of here, I have changed it
to remove the loop.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.h:60: summed_outer_parameters_count_ = 0;
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> summed_ seems like a noise word and I wouldn't make it plural if I was using
> count, so: outer_parameter_count_.

Done.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:673: set_on->set_type_parameters(type_parameters);
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> Having two different template instantiations of this function seems
unnecessary.
>  I suggest passing a const Object& and using something like:
> 
> ASSERT(set_on.IsClass() || set_on.IsFunction());
> if (set_on.IsClass()) {
>   Class::Cast(set_on).set_type_parameters...
> } else {
>   Function::Cast(set_on).set...
> }

Done.

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

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:196: if (member == NULL) return false;
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> This doesn't seem safe.  I think if there is no member it's a mistake to ask
if
> the member is a procedure.  You might interpret "false" as something like
"it's
> a Field" and that is a latent bug.
> 
> Can we assert member != NULL?

I've added as "HasMember" function, and guarded calls with that.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:206: if (member == NULL) return false;
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> Likewise, I'm not sure this is safe.

As above.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:210: intptr_t MemberTypeParameterCount(Zone* zone) {
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> We don't need all this code in the header file, do we?

I don't really care - but no, it doesn't have to be in the header file. I've
moved it to the .cc file.

https://codereview.chromium.org/2973633002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:217: } else if (function_kind ==
RawFunction::kMethodExtractor) {
On 2017/08/08 14:08:51, Kevin Millikin (Google) wrote:
> member->IsMethodExtractor()

Done.

[jensj, 2017-08-09 09:45:05]

Description was changed from

==========
[kernel] Change how TypeParameterType is calculated.

Prior to this CL we carried around information about the containing class
and member, both of which was fetched by reading out-of-line in the binary
(i.e. while reading the current member, start reading something from the
parent member etc).
It had also required the introduction of extra fields in the kernel
binary file (dill file).

This CL cleans that up, by
a) Setting type parameters on functions as needed (in kernel_reader.cc)
b) Using the VM Class and VM Function to get the required information
   (with a above the information is all available).
   (in kernel_binary_flowgraph.cc.) This means that
c) We don't have to read the binary out-of-line (for TypeParameterType
   to work at least), and that
d) We can remove the previously introduced extra fields from the
   kernel binary file (dill file).
==========

to

==========
[kernel] Change how TypeParameterType is calculated.

Prior to this CL we carried around information about the containing class
and member, both of which was fetched by reading out-of-line in the binary
(i.e. while reading the current member, start reading something from the
parent member etc).
It had also required the introduction of extra fields in the kernel
binary file (dill file).

This CL cleans that up, by
a) Setting type parameters on functions as needed (in kernel_reader.cc)
b) Using the VM Class and VM Function to get the required information
   (with a above the information is all available).
   (in kernel_binary_flowgraph.cc.) This means that
c) We don't have to read the binary out-of-line (for TypeParameterType
   to work at least), and that
d) We can remove the previously introduced extra fields from the
   kernel binary file (dill file).

R=dmitryas@google.com, kmillikin@google.com

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

[jensj, 2017-08-09 09:45:07]

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