Canonicalize types.

Canonicalize types.
Committed: https://code.google.com/p/dart/source/detail?r=1045

[srdjan, 2011-11-01 20:51:43]

LGTM with comments and questions

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:455: super_type = super_type.Canonicalize();
Do you need to finalize the super_type that you may actually be throwing away?
Maybe call FinalizeType after getting the type's canonical version?

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:470: super_type_arg =
super_type_arg.Canonicalize();
I assume you cannot ask for proper canonical version until you have instantiated
it?

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:505: type_argument = type_argument.Canonicalize();
Ditto here, Finalize after Canonicalize or is there something that happens that
you need from finalization?

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:36: // Set the error message on failure.
What if no failure? errmsg.IsNull or empty?

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:37: static RawType*
FinalizeAndCanonicalizeType(const Type& type, String& errmsg);
String* instead of String& (reference passed with with const, use pointer for
results)

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2055
runtime/vm/object.cc:2055: if (canonical_types.IsNull()) {
Should this be an assert (canonical types is initialized to empty array).

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2070
runtime/vm/object.cc:2070: }
I wonder if you need num_canonical_types variable. Instead you can use the
canonical_types.Length() and abort once you meet type.IsNull.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2076
runtime/vm/object.cc:2076: (old_length == 0) ? kInitialCanonicalTypesLength :
old_length * 2;
The growth (old_length * 2) seems a little excessive. Why not + kGrowthConstant

[regis, 2011-11-01 21:54:50]

Thanks!

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:455: super_type = super_type.Canonicalize();
On 2011/11/01 20:51:43, srdjan wrote:
> Do you need to finalize the super_type that you may actually be throwing away?
> Maybe call FinalizeType after getting the type's canonical version?

The finalized and canonicalized super type is written back to the class on the
following line. So this is not wasted work.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:470: super_type_arg =
super_type_arg.Canonicalize();
On 2011/11/01 20:51:43, srdjan wrote:
> I assume you cannot ask for proper canonical version until you have
instantiated
> it?

Correct. We want to canonicalize the finalized output type, with instantiated
type parameters of its super types.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:505: type_argument = type_argument.Canonicalize();
On 2011/11/01 20:51:43, srdjan wrote:
> Ditto here, Finalize after Canonicalize or is there something that happens
that
> you need from finalization?

Finalization may instantiate type arguments, so it does not make sense to
canonicalize before finalization.
I may try to canonicalize as part of finalization, instead of having two
separate calls. Would that be clearer?

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.h
File runtime/vm/class_finalizer.h (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:36: // Set the error message on failure.
On 2011/11/01 20:51:43, srdjan wrote:
> What if no failure? errmsg.IsNull or empty?

errmsg.IsNull() if no error. Added comment.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.h#ne...
runtime/vm/class_finalizer.h:37: static RawType*
FinalizeAndCanonicalizeType(const Type& type, String& errmsg);
On 2011/11/01 20:51:43, srdjan wrote:
> String* instead of String& (reference passed with with const, use pointer for
> results)

Done.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2055
runtime/vm/object.cc:2055: if (canonical_types.IsNull()) {
On 2011/11/01 20:51:43, srdjan wrote:
> Should this be an assert (canonical types is initialized to empty array).

No, this is more complicated. Classes of the VM isolate (e.g. null_class_,
void_class_, dynamic_class_) cannot point to arrays allocated in other isolates.
We would have to store these classes in the object store.
I added a TODO, in case we decide to do just that.
It may then be possible to remove all types from the object store and rely on
type canonicalization instead. I'd prefer to do this in a following cl.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2070
runtime/vm/object.cc:2070: }
On 2011/11/01 20:51:43, srdjan wrote:
> I wonder if you need num_canonical_types variable. Instead you can use the
> canonical_types.Length() and abort once you meet type.IsNull.

There is not sentinel ending the array with a null type. I duplicated the code
used for constants.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2076
runtime/vm/object.cc:2076: (old_length == 0) ? kInitialCanonicalTypesLength :
old_length * 2;
On 2011/11/01 20:51:43, srdjan wrote:
> The growth (old_length * 2) seems a little excessive. Why not +
kGrowthConstant

OK, I now add 2 elements each time the array grows.

[srdjan, 2011-11-01 22:05:14]

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:455: super_type = super_type.Canonicalize();
On 2011/11/01 21:54:50, regis wrote:
> On 2011/11/01 20:51:43, srdjan wrote:
> > Do you need to finalize the super_type that you may actually be throwing
away?
> > Maybe call FinalizeType after getting the type's canonical version?
> 
> The finalized and canonicalized super type is written back to the class on the
> following line. So this is not wasted work.

As I understand it the canonical super_type could already have been finalized,
so the finalization of 'rejected' super_type instance is wasted. However, it
seems from otehr comments that you need to finalize in order to properly search
for canonical version.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2070
runtime/vm/object.cc:2070: }
On 2011/11/01 21:54:50, regis wrote:
> On 2011/11/01 20:51:43, srdjan wrote:
> > I wonder if you need num_canonical_types variable. Instead you can use the
> > canonical_types.Length() and abort once you meet type.IsNull.
> 
> There is not sentinel ending the array with a null type. I duplicated the code
> used for constants.

You do not need a NULL sentinel as you would iterate for length of array and
break when null encountered. Grow operation fills array with null objects.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2076
runtime/vm/object.cc:2076: (old_length == 0) ? kInitialCanonicalTypesLength :
old_length * 2;
On 2011/11/01 21:54:50, regis wrote:
> On 2011/11/01 20:51:43, srdjan wrote:
> > The growth (old_length * 2) seems a little excessive. Why not +
> kGrowthConstant
> 
> OK, I now add 2 elements each time the array grows. 

If you add only one, you can get rid of a field in class and make the algorithm
simpler :-).

[regis, 2011-11-01 22:28:53]

Thanks for the additional feedback. I'll send you an another cl addressing it.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/class_finalizer.cc#n...
runtime/vm/class_finalizer.cc:455: super_type = super_type.Canonicalize();
On 2011/11/01 22:05:14, srdjan wrote:
> On 2011/11/01 21:54:50, regis wrote:
> > On 2011/11/01 20:51:43, srdjan wrote:
> > > Do you need to finalize the super_type that you may actually be throwing
> away?
> > > Maybe call FinalizeType after getting the type's canonical version?
> > 
> > The finalized and canonicalized super type is written back to the class on
the
> > following line. So this is not wasted work.
> 
> As I understand it the canonical super_type could already have been finalized,
> so the finalization of 'rejected' super_type instance is wasted. However, it
> seems from otehr comments that you need to finalize in order to properly
search
> for canonical version.

If the super_type was already finalized, the call to FinalizeType returns
immediately. In that case, the only wasted work is to canonicalize again. This
is why I will try to combine finalization with canonicalization.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2070
runtime/vm/object.cc:2070: }
On 2011/11/01 22:05:14, srdjan wrote:
> On 2011/11/01 21:54:50, regis wrote:
> > On 2011/11/01 20:51:43, srdjan wrote:
> > > I wonder if you need num_canonical_types variable. Instead you can use the
> > > canonical_types.Length() and abort once you meet type.IsNull.
> > 
> > There is not sentinel ending the array with a null type. I duplicated the
code
> > used for constants.
> 
> You do not need a NULL sentinel as you would iterate for length of array and
> break when null encountered. Grow operation fills array with null objects.
> 

Ah, I now see what you mean. Clever!
I missed the fact that we can rely on the Grow operation initializing the empty
space.
I will remove the num_canonical_types_ as well as the num_contants_ field.

http://codereview.chromium.org/8372041/diff/1/runtime/vm/object.cc#newcode2076
runtime/vm/object.cc:2076: (old_length == 0) ? kInitialCanonicalTypesLength :
old_length * 2;
On 2011/11/01 22:05:14, srdjan wrote:
> On 2011/11/01 21:54:50, regis wrote:
> > On 2011/11/01 20:51:43, srdjan wrote:
> > > The growth (old_length * 2) seems a little excessive. Why not +
> > kGrowthConstant
> > 
> > OK, I now add 2 elements each time the array grows. 
> 
> If you add only one, you can get rid of a field in class and make the
algorithm
> simpler :-).

Actually, I think I can still simplify and remove the field while incrementing
by 2 :-)