- Restructure the Canonicalization of instances in preparation for adding a

- Restructure Canonicalization of instances in preparation for adding a
  HashMap to store the Canonical constants of Instances/Arrays. The goal is
  to continue to use an array for Numbers (Mint, Bigint, Double) and use a
  HashMap for other Instances/Arrays.

- Check first for the array length and elements being same before checking
  if the TypeArguments are equivalent

- Removed some unused code

R=regis@google.com

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

[siva, 2016-04-06 20:57:04]

Description was changed from

==========
- Restructure the Canonicalization of instances in preparation for adding a
  HashMap to store the Canonical constants of Instances/Arrays. The goal is
  to continue to use an array for Numbers (Mint, Bigint, Double) and use a
  HashMap for other Instances/Arrays.

- Check first for the array length and elements being same before checking
  if the TypeArguments are equivalent

- Removed some unused code
==========

to

==========
- Restructure Canonicalization of instances in preparation for adding a
  HashMap to store the Canonical constants of Instances/Arrays. The goal is
  to continue to use an array for Numbers (Mint, Bigint, Double) and use a
  HashMap for other Instances/Arrays.

- Check first for the array length and elements being same before checking
  if the TypeArguments are equivalent

- Removed some unused code
==========

[siva, 2016-04-06 21:12:19]

asiva@google.com changed reviewers:
+ regis@google.com

[regis, 2016-04-06 22:02:39]

LGTM

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

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:17353: case kBigintCid: {
Why are you not calling the existing Bigint::NewCanonical as for Mint and Double
above?

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20539: for (intptr_t i = 0; i < len; i++) {
Depending on len, this loop could be much more expensive than checking the type
arguments and could be run last (possibly only if len is above a threshold, but
that requires more code).
If the average array is small, checking the type arguments last is fine.

[siva, 2016-04-06 22:26:36]

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

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:17353: case kBigintCid: {
On 2016/04/06 22:02:39, regis wrote:
> Why are you not calling the existing Bigint::NewCanonical as for Mint and
Double
> above?

Bigint::NewCanonical requires a String object which is scanned and converted
into a bigint, it would be expensive to convert the bigint object back to a
string and then call Bigint::NewCanonical.

I could have called Instance::CheckAndCanonicalize here but that is the code I
am going to change next for instance/arrays to use a HashMap and hence that code
appears to be duplicated here.

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20539: for (intptr_t i = 0; i < len; i++) {
On 2016/04/06 22:02:39, regis wrote:
> Depending on len, this loop could be much more expensive than checking the
type
> arguments and could be run last (possibly only if len is above a threshold,
but
> that requires more code).
> If the average array is small, checking the type arguments last is fine.

How many times do you think we will have a case where the length is the same and
all the objects are identical but the TypeArguments are different, I am thinking
that is a rarer case. Most of the times the length will not match or if the
length matches then some objects in the array will not match and we will bailout
sooner.

Agreed there could be a degenerate case where a 1000 length array has 999
elements that are identical and the last one is different.

[srdjan, 2016-04-06 22:32:43]

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

[srdjan, 2016-04-06 22:32:44]

DBC

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

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:14672: char* chars = OS::SCreate(Thread::Current()->zone(),
zone

[siva, 2016-04-06 22:35:33]

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

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:14672: char* chars = OS::SCreate(Thread::Current()->zone(),
On 2016/04/06 22:32:44, srdjan wrote:
> zone

Done.

[regis, 2016-04-06 22:42:44]

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

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:17353: case kBigintCid: {
On 2016/04/06 22:26:36, siva wrote:
> On 2016/04/06 22:02:39, regis wrote:
> > Why are you not calling the existing Bigint::NewCanonical as for Mint and
> Double
> > above?
> 
> Bigint::NewCanonical requires a String object which is scanned and converted
> into a bigint, it would be expensive to convert the bigint object back to a
> string and then call Bigint::NewCanonical.
> 
> I could have called Instance::CheckAndCanonicalize here but that is the code I
> am going to change next for instance/arrays to use a HashMap and hence that
code
> appears to be duplicated here.

I missed the fact that the argument is a String.

https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20539: for (intptr_t i = 0; i < len; i++) {
On 2016/04/06 22:26:36, siva wrote:
> On 2016/04/06 22:02:39, regis wrote:
> > Depending on len, this loop could be much more expensive than checking the
> type
> > arguments and could be run last (possibly only if len is above a threshold,
> but
> > that requires more code).
> > If the average array is small, checking the type arguments last is fine.
> 
> How many times do you think we will have a case where the length is the same
and
> all the objects are identical but the TypeArguments are different, I am
thinking
> that is a rarer case. Most of the times the length will not match or if the
> length matches then some objects in the array will not match and we will
bailout
> sooner.
> 
> Agreed there could be a degenerate case where a 1000 length array has 999
> elements that are identical and the last one is different.

True, checking the (identical) type first would not help in that case. What I
had in mind is more like 2 identical arrays of 1000 elements, but one is typed
whereas the other one is raw.
But I agree, it is all speculation at this time :-)
Please submit as is.

[siva, 2016-04-06 22:47:45]

Description was changed from

==========
- Restructure Canonicalization of instances in preparation for adding a
  HashMap to store the Canonical constants of Instances/Arrays. The goal is
  to continue to use an array for Numbers (Mint, Bigint, Double) and use a
  HashMap for other Instances/Arrays.

- Check first for the array length and elements being same before checking
  if the TypeArguments are equivalent

- Removed some unused code
==========

to

==========
- Restructure Canonicalization of instances in preparation for adding a
  HashMap to store the Canonical constants of Instances/Arrays. The goal is
  to continue to use an array for Numbers (Mint, Bigint, Double) and use a
  HashMap for other Instances/Arrays.

- Check first for the array length and elements being same before checking
  if the TypeArguments are equivalent

- Removed some unused code

R=regis@google.com

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

[siva, 2016-04-06 22:47:47]

Committed patchset #3 (id:40001) manually as
07d0c715ff516a29a43c54fb0987dbf92ab2a86b (presubmit successful).

[siva, 2016-04-06 23:22:34]

On 2016/04/06 22:42:44, regis wrote:
> https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
> runtime/vm/object.cc:17353: case kBigintCid: {
> On 2016/04/06 22:26:36, siva wrote:
> > On 2016/04/06 22:02:39, regis wrote:
> > > Why are you not calling the existing Bigint::NewCanonical as for Mint and
> > Double
> > > above?
> > 
> > Bigint::NewCanonical requires a String object which is scanned and converted
> > into a bigint, it would be expensive to convert the bigint object back to a
> > string and then call Bigint::NewCanonical.
> > 
> > I could have called Instance::CheckAndCanonicalize here but that is the code
I
> > am going to change next for instance/arrays to use a HashMap and hence that
> code
> > appears to be duplicated here.
> 
> I missed the fact that the argument is a String.
> 
>
https://codereview.chromium.org/1866713003/diff/20001/runtime/vm/object.cc#ne...
> runtime/vm/object.cc:20539: for (intptr_t i = 0; i < len; i++) {
> On 2016/04/06 22:26:36, siva wrote:
> > On 2016/04/06 22:02:39, regis wrote:
> > > Depending on len, this loop could be much more expensive than checking the
> > type
> > > arguments and could be run last (possibly only if len is above a
threshold,
> > but
> > > that requires more code).
> > > If the average array is small, checking the type arguments last is fine.
> > 
> > How many times do you think we will have a case where the length is the same
> and
> > all the objects are identical but the TypeArguments are different, I am
> thinking
> > that is a rarer case. Most of the times the length will not match or if the
> > length matches then some objects in the array will not match and we will
> bailout
> > sooner.
> > 
> > Agreed there could be a degenerate case where a 1000 length array has 999
> > elements that are identical and the last one is different.
> 
> True, checking the (identical) type first would not help in that case. What I
> had in mind is more like 2 identical arrays of 1000 elements, but one is typed
> whereas the other one is raw.
> But I agree, it is all speculation at this time :-)
> Please submit as is.

This switch seems to have improved the compile all times of green tea app
sources,
it used to take about 20 secs to do a compile all of the green tea app sources,
now it takes :
real 17.52
user 18.22
sys 1.12