Add custom equals and hashCode for HashMap implementation.

Add custom equals and hashCode for HashMap implementation.

R=floitsch@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27106

[Lasse Reichstein Nielsen, 2013-09-03 13:02:50]

Preliminary review, I think there's a bug in the dart2js code.

[floitsch, 2013-09-03 13:59:08]

fyi.

https://codereview.chromium.org/23893002/diff/4001/runtime/lib/collection_pat...
File runtime/lib/collection_patch.dart (right):

https://codereview.chromium.org/23893002/diff/4001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:202: class _CustomHashMap<K, V> extends
_HashMapImpl<K, V> {
File a bug against the VM asking for customization and add it here.
We shouldn't need to customize the classes ourselves.

Phrase the bug in a way that explains the problem (code duplication) but doesn't
fixate on one solution (customization). Maybe they have other ideas/approaches
to reduce code duplication without sacrificing speed.

https://codereview.chromium.org/23893002/diff/4001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:297: class _IdentityHashMap<K, V> extends
_HashMapImpl<K, V> {
I'm not sure we need to have Identity maps to be customized yet.
I would extend a base class, overwriting the _equals and _hashCode methods, and
wait for the VM to customize.

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/_internal/lib/coll...
File sdk/lib/_internal/lib/collection_patch.dart (right):

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/_internal/lib/coll...
sdk/lib/_internal/lib/collection_patch.dart:714: static List _getBucket(var
table, var key) {
Where is this called?

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/_internal/lib/coll...
sdk/lib/_internal/lib/collection_patch.dart:719: static int _findBucketIndex(var
bucket, var key) {
ditto.

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/collection/hash_ma...
File sdk/lib/collection/hash_map.dart (right):

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/collection/hash_ma...
sdk/lib/collection/hash_map.dart:48: external factory HashMap({bool equals(K
key1, K key2), int hashCode(K key)});
I would be ok to have a HashMap.identity (and Map.identity) but we can add those
later.

[Lasse Reichstein Nielsen, 2013-09-04 05:59:21]

https://codereview.chromium.org/23893002/diff/4001/runtime/lib/collection_pat...
File runtime/lib/collection_patch.dart (right):

https://codereview.chromium.org/23893002/diff/4001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:202: class _CustomHashMap<K, V> extends
_HashMapImpl<K, V> {
On 2013/09/03 13:59:08, floitsch wrote:
> File a bug against the VM asking for customization and add it here.
> We shouldn't need to customize the classes ourselves.

> Phrase the bug in a way that explains the problem (code duplication) but
doesn't
> fixate on one solution (customization). Maybe they have other ideas/approaches
> to reduce code duplication without sacrificing speed.

I can avoid the code duplication myself by abstracting the equals and hashcode
functionality into overridable methods. I just decided not to, because the
functions are so small and simple (well, except for putIfAbsent maybe) that
duplication is probably better. Performance-wise, it's the *safe* bet (it
doesn't change what we have now for HashMap), and we can always try to avoid the
code duplication later.

Whether the VM customizes functions or not was not part of the decision (since I
honestly don't know if it does).

https://codereview.chromium.org/23893002/diff/4001/runtime/lib/collection_pat...
runtime/lib/collection_patch.dart:297: class _IdentityHashMap<K, V> extends
_HashMapImpl<K, V> {
On 2013/09/03 13:59:08, floitsch wrote:
> I'm not sure we need to have Identity maps to be customized yet.
> I would extend a base class, overwriting the _equals and _hashCode methods,
and
> wait for the VM to customize.

I expect IdentityMap to be the main usage of customizable HashMap, so I don't
want to wait for an optimization to occur. I'd rather have it as efficient as
possible right now.

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/_internal/lib/coll...
File sdk/lib/_internal/lib/collection_patch.dart (right):

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/_internal/lib/coll...
sdk/lib/_internal/lib/collection_patch.dart:714: static List _getBucket(var
table, var key) {
Ick, bad paste, I think.
Removed.

https://codereview.chromium.org/23893002/diff/4001/sdk/lib/_internal/lib/coll...
sdk/lib/_internal/lib/collection_patch.dart:719: static int _findBucketIndex(var
bucket, var key) {
do.

[floitsch, 2013-09-04 09:04:49]

LGTM.

[Lasse Reichstein Nielsen, 2013-09-04 09:42:34]

Committed patchset #3 manually as r27106 (presubmit successful).