Serialize maps without hashes.

Serialize maps without hashes.

Re-generate the indices after deserialization.

This is a more compact representation, and also fixes the issue with identity hash codes not being portable between isolates.

BUG=21675
R=asiva@google.com

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

[koda, 2015-06-01 17:01:41]

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

[koda, 2015-06-01 17:18:32]

I started with a eager finalize approach, but then chose lazy regeneration to
avoid changes to interface of SnapshotReader and keep the logic within map
class.

It should also piggy-back nicely with the null-check for _index that is already
happening, and having this case could use useful for other optimized
representations (e.g., to replace the Maplets/Setlets of d2js).

However, if we later decide to implement lrn's suggestion to allow a user-level
callback interface to do post-deserialization fixups, then we could also use
that for the maps.

[siva, 2015-06-01 22:19:37]

lgtm

https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
File runtime/lib/compact_hash.dart (right):

https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
runtime/lib/compact_hash.dart:178: return _index.length;
instead of checking every caller to this function why not just do
if (_index == null) {
  ......
  ......
}
return _index.length;

https://codereview.chromium.org/1151013005/diff/120001/runtime/vm/raw_object_...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1151013005/diff/120001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:2225: 
Maybe add comments here to match the comments on the write side:

e.g:

// Read the type arguments.
........


// Read the number of key/value pairs.
.....

// Allocate the Hashmap object.
.....

// Read the keys and values.
.....

https://codereview.chromium.org/1151013005/diff/120001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:2241: // in particular, if/when (const) maps
are needed in the VM isolate snapshot.
We need an assertion to make sure we don't write these into the VM isolate
snapshot currently.

[koda, 2015-06-01 23:01:01]

Let me know if you feel strongly about the null check, otherwise I'll leave the
"optimization" in.

https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
File runtime/lib/compact_hash.dart (right):

https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
runtime/lib/compact_hash.dart:178: return _index.length;
On 2015/06/01 22:19:36, siva wrote:
> instead of checking every caller to this function why not just do
> if (_index == null) {
>   ......
>   ......
> }
> return _index.length;

Since we don't do partial inlining (right?), that will either add a function
call to all those operations or blow up their code size.

Do you think this is a premature optimization?

https://codereview.chromium.org/1151013005/diff/120001/runtime/vm/raw_object_...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1151013005/diff/120001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:2225: 
On 2015/06/01 22:19:36, siva wrote:
> Maybe add comments here to match the comments on the write side:
> 
> e.g:
> 
> // Read the type arguments.
> ........
> 
> 
> // Read the number of key/value pairs.
> .....
> 
> // Allocate the Hashmap object.
> .....
> 
> // Read the keys and values.
> .....

Done.

https://codereview.chromium.org/1151013005/diff/120001/runtime/vm/raw_object_...
runtime/vm/raw_object_snapshot.cc:2241: // in particular, if/when (const) maps
are needed in the VM isolate snapshot.
On 2015/06/01 22:19:36, siva wrote:
> We need an assertion to make sure we don't write these into the VM isolate
> snapshot currently.

Added an explicit assertion here, for documentation purposes. (This case was
already protected against by the UNREACHABLE for full and script snapshots in
the WriteTo.)

[siva, 2015-06-02 00:13:35]

https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
File runtime/lib/compact_hash.dart (right):

https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
runtime/lib/compact_hash.dart:178: return _index.length;
On 2015/06/01 23:01:01, koda wrote:
> On 2015/06/01 22:19:36, siva wrote:
> > instead of checking every caller to this function why not just do
> > if (_index == null) {
> >   ......
> >   ......
> > }
> > return _index.length;
> 
> Since we don't do partial inlining (right?), that will either add a function
> call to all those operations or blow up their code size.
> 
> Do you think this is a premature optimization?

I don't feel strongly about moving the null check into the function but it does
make the code seem verbose at the call sites.

[koda, 2015-06-02 16:03:09]

On 2015/06/02 00:13:35, siva wrote:
>
https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
> File runtime/lib/compact_hash.dart (right):
> 
>
https://codereview.chromium.org/1151013005/diff/120001/runtime/lib/compact_ha...
> runtime/lib/compact_hash.dart:178: return _index.length;
> On 2015/06/01 23:01:01, koda wrote:
> > On 2015/06/01 22:19:36, siva wrote:
> > > instead of checking every caller to this function why not just do
> > > if (_index == null) {
> > >   ......
> > >   ......
> > > }
> > > return _index.length;
> > 
> > Since we don't do partial inlining (right?), that will either add a function
> > call to all those operations or blow up their code size.
> > 
> > Do you think this is a premature optimization?
> 
> I don't feel strongly about moving the null check into the function but it
does
> make the code seem verbose at the call sites.

I solved it by adding a small helper method to share the conditional expression.

Also fixed an issue with recursive reuse of a handle in the snapshot reader.

[koda, 2015-06-02 16:04:49]

Committed patchset #9 (id:160001) manually as
6090af8f0b70a5bb0d1b38346a65428556eb0ac3 (presubmit successful).