VM-internalize the default Map implementation.

VM-internalize the default Map implementation.

Make the compact linked hash map, which is the default implementation for Map
(including map literals), a VM-internal class.

This makes it easy to have more efficient serialization of maps (although for
now, the implementation is straight-forward).

Refactor the compact hash class hierarchy to enable the VM-internal class and
the rest (also the Set classes) to share a maximal amount of code, by using
two different bases for implicit/explicit fields.

Remove existing proof-of-concept, C++-based internal VM-class.

BUG=http://dartbug.com/22982
R=asiva@google.com

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

[koda, 2015-05-20 13:28:00]

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

[siva, 2015-05-22 16:27:11]

https://codereview.chromium.org/1151523002/diff/40001/runtime/lib/compact_has...
File runtime/lib/compact_hash.dart (right):

https://codereview.chromium.org/1151523002/diff/40001/runtime/lib/compact_has...
runtime/lib/compact_hash.dart:74: }
I think leaving the fields in here commented out is confusing, even if the diffs
are going to be larger I think it would be better to move the comments below to
_HashFieldBase and _HashVMBase and remove the fields from this class.

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

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19530: // Keep this in sync with Dart implementation
(lib/compact_hash.dart).
I think you need to add a verification step for this see 
ClassFinalizer::VerifyImplicitFieldOffsets

I think we should also add this class into
ClassFinalizer::VerifyBootstrapClasses() to ensure InstanceSize matches the size
reported by the class.

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1700: RawSmi* deleted_keys_;
We have always put the RawSmi* fields also inside the from/to block and treated
them like regular heap objects in this file. This seems inconsistent with that.

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2242: */
Why is this done this way and not using the from/to loop like other types?

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2273:
writer->Write<RawObject*>(ptr()->deleted_keys_);
Ditto question.

[koda, 2015-05-26 12:22:05]

PTAL

https://codereview.chromium.org/1151523002/diff/40001/runtime/lib/compact_has...
File runtime/lib/compact_hash.dart (right):

https://codereview.chromium.org/1151523002/diff/40001/runtime/lib/compact_has...
runtime/lib/compact_hash.dart:74: }
On 2015/05/22 16:27:11, siva wrote:
> I think leaving the fields in here commented out is confusing, even if the
diffs
> are going to be larger I think it would be better to move the comments below
to
> _HashFieldBase and _HashVMBase and remove the fields from this class.

Done.

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

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19530: // Keep this in sync with Dart implementation
(lib/compact_hash.dart).
On 2015/05/22 16:27:11, siva wrote:
> I think you need to add a verification step for this see 
> ClassFinalizer::VerifyImplicitFieldOffsets

Done, as a unit test (which can be extended as more methods get C++
counterparts)

> 
> I think we should also add this class into
> ClassFinalizer::VerifyBootstrapClasses() to ensure InstanceSize matches the
size
> reported by the class.

Done.

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1700: RawSmi* deleted_keys_;
On 2015/05/22 16:27:11, siva wrote:
> We have always put the RawSmi* fields also inside the from/to block and
treated
> them like regular heap objects in this file. This seems inconsistent with
that.

Done.

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2242: */
On 2015/05/22 16:27:11, siva wrote:
> Why is this done this way and not using the from/to loop like other types?

Done.

https://codereview.chromium.org/1151523002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2273:
writer->Write<RawObject*>(ptr()->deleted_keys_);
On 2015/05/22 16:27:11, siva wrote:
> Ditto question.

Done.

[koda, 2015-05-26 13:25:09]

Also added a map serialization benchmark to guide future improvements to the
format.

Even with the current, straight-forward serialization, the compact maps already
score ~5x higher than the old non-compact ones.

[koda, 2015-05-27 21:15:55]

On 2015/05/26 13:25:09, koda wrote:
> Also added a map serialization benchmark to guide future improvements to the
> format.
> 
> Even with the current, straight-forward serialization, the compact maps
already
> score ~5x higher than the old non-compact ones.

Ping. Would like to improve the serialization further before 1.11.

[siva, 2015-05-27 22:06:02]

lgtm

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

https://codereview.chromium.org/1151523002/diff/80001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19541: #endif
Why not move this to platform/globals.h

[koda, 2015-05-28 16:33:03]

Committed patchset #6 (id:100001) manually as
ea46192ca34bdef17fc0e9f8316cfd83899443be (presubmit successful).

[koda, 2015-05-28 16:34:38]

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

https://codereview.chromium.org/1151523002/diff/80001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19541: #endif
On 2015/05/27 22:06:02, siva wrote:
> Why not move this to platform/globals.h
> 

It seems unlikely it will be used anywhere but here and the corresponding Dart
code. My unit test ensures they agree.

[siva, 2015-05-28 17:55:13]

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

https://codereview.chromium.org/1151523002/diff/80001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:19541: #endif
On 2015/05/28 16:34:38, koda wrote:
> On 2015/05/27 22:06:02, siva wrote:
> > Why not move this to platform/globals.h
> > 
> 
> It seems unlikely it will be used anywhere but here and the corresponding Dart
> code. My unit test ensures they agree.

We would like to keep the code in these files clean without cluttering it with
these #if defined() stuff and hence the suggestion.