Avoid numerous type allocations by caching the canonical type of a class, which

Avoid numerous type allocations by caching the canonical type of a class, which
is used as the receiver type for non-static methods of this class, be it a
generic, non-generic, or signature class.

R=srdjan@google.com

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

[regis, 2015-08-21 01:52:35]

regis@google.com changed reviewers:
+ asiva@google.com, iposva@google.com, srdjan@google.com

[srdjan, 2015-08-21 05:02:21]

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

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3617: if (!types.IsNull() && types.Length() > 0) {
Add parenthesis.

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3626: if ((NumTypeArguments() == 0) && !IsSignatureClass())
{
Should we factor out this test? Only if we can come up with a descriptive name
that is shorter than this test :-).

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3629: set_canonical_types(type);
Can we clear it ever, i.e., type -> null ? If it is the same, maybe skip writing
it? Is that check worth it?

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:1039: // type parameters declared by this class if it is
generic, e.g. Map<K, V>.
Add comment: returns Type::null() if ...

[regis, 2015-08-21 15:38:22]

Thanks!

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

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3617: if (!types.IsNull() && types.Length() > 0) {
On 2015/08/21 05:02:20, srdjan wrote:
> Add parenthesis.

Done.

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3626: if ((NumTypeArguments() == 0) && !IsSignatureClass())
{
On 2015/08/21 05:02:20, srdjan wrote:
> Should we factor out this test? Only if we can come up with a descriptive name
> that is shorter than this test :-).

I cannot think of a good name :-)
In any case, I think it is only needed here and in the setter above. I will try
to remove it from other places where I do not think it is necessary.

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3629: set_canonical_types(type);
On 2015/08/21 05:02:20, srdjan wrote:
> Can we clear it ever, i.e., type -> null ? If it is the same, maybe skip
writing
> it? Is that check worth it?

No, we cannot clear it. However, from the time we notice it was null and start
computing it, it may have been set via recursion.
I think it would be bad if it set to a different type than the one we just
calculated.

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1309613002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:1039: // type parameters declared by this class if it is
generic, e.g. Map<K, V>.
On 2015/08/21 05:02:21, srdjan wrote:
> Add comment: returns Type::null() if ...

Done.

[srdjan, 2015-08-21 18:16:04]

lgtm

https://codereview.chromium.org/1309613002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1309613002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1869: RawType* Class::SignatureType() const {
Maybe we should grab the zone here and use it in the many Handle allocations.

[regis, 2015-08-21 20:33:28]

Thanks!

https://codereview.chromium.org/1309613002/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1309613002/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1869: RawType* Class::SignatureType() const {
On 2015/08/21 18:16:04, srdjan wrote:
> Maybe we should grab the zone here and use it in the many Handle allocations.

OK. I'll do it in the next revision, which I have started already.

[regis, 2015-08-21 20:37:17]

Committed patchset #2 (id:20001) manually as
26031ab6b00022d96669db1f9c4370251ca1e389 (presubmit successful).