- Use a hash table to canonicalize instances/arrays to avoid having to iterate over a linear list a…

- Use a hash table to canonicalize instances/arrays to avoid having to iterate over a linear list and search for canonical instances.
- Some minor cleanups
  - Return an empty array right away in the parser instead of creating an
    empty array and canonicalizing it
  - The canonical bit was not being set for some of the singleton objects.

R=regis@google.com

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

[siva, 2016-04-12 20:17:03]

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

[regis, 2016-04-12 21:12:32]

LGTM with comments/questions

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:4469: };
No blank line(s)?

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:4480: this->set_constants(constants.Release());
Since you are only doing a lookup in the constants array, it should not change.
So you should be able to replace the last line with an assert:
ASSERT(this->constants() == constants.Release().raw());

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:14774: hash = CombineHashes(hash, (value >> 3));
If the field is a Smi, you ignore 2 bits of its value. Is that OK?

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:20707: hash = CombineHashes(hash, (value >> 3));
Same comment about ignoring 2 bits if the array element is a Smi.

[siva, 2016-04-13 16:52:41]

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:4469: };
On 2016/04/12 21:12:32, regis wrote:
> No blank line(s)?

This seems to be the pattern that everybody has used in all the other places
that hash table typedefs have been defined.

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:4480: this->set_constants(constants.Release());
On 2016/04/12 21:12:32, regis wrote:
> Since you are only doing a lookup in the constants array, it should not
change.
> So you should be able to replace the last line with an assert:
> ASSERT(this->constants() == constants.Release().raw()); 

The problem is constants.Release() needs to be called to cleanup as the
destructor does nothing, we can't do the Release only in ASSERT.

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:14774: hash = CombineHashes(hash, (value >> 3));
On 2016/04/12 21:12:32, regis wrote:
> If the field is a Smi, you ignore 2 bits of its value. Is that OK?

Removed teh shift as discussed offline.

https://chromiumcodereview.appspot.com/1873143003/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:20707: hash = CombineHashes(hash, (value >> 3));
On 2016/04/12 21:12:32, regis wrote:
> Same comment about ignoring 2 bits if the array element is a Smi.

Done.

[siva, 2016-04-13 17:26:20]

Description was changed from

==========
- Use a hash table to canonicalize instances/arrays to avoid having to iterate
over a linear list and search for canonical instances.
- Some minor cleanups
  - Return an empty array right away in the parser instead of creating an
    empty array and canonicalizing it
  - The canonical bit was not being set for some of the singleton objects.
==========

to

==========
- Use a hash table to canonicalize instances/arrays to avoid having to iterate
over a linear list and search for canonical instances.
- Some minor cleanups
  - Return an empty array right away in the parser instead of creating an
    empty array and canonicalizing it
  - The canonical bit was not being set for some of the singleton objects.

R=regis@google.com

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

[siva, 2016-04-13 17:26:21]

Committed patchset #5 (id:80001) manually as
95a2b02a9e74714d7e9835e7af0fd85f694a4d2d (presubmit successful).

[rmacnak, 2016-04-15 18:19:17]

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

[rmacnak, 2016-04-15 18:19:18]

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

https://codereview.chromium.org/1873143003/diff/80001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:14840: uword value = reinterpret_cast<uword>(
This won't work for precompiled or app snapshots, as the address can be
different in the new process. Or a compacting old space.