Use an ES6 map for a linked identity hash map.

Use an ES6 map for a linked identity hash map.

BUG=
R=floitsch@google.com

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

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

[Harry Terkelsen, 2015-06-17 18:27:31]

het@google.com changed reviewers:
+ floitsch@google.com, sra@chromium.org

[Harry Terkelsen, 2015-06-17 18:27:32]

I am only overriding the LinkedIdentityHashMap to avoid having to rehash
everything if something overrides hashCode.

[Harry Terkelsen, 2015-06-17 18:27:50]

het@google.com changed reviewers:
+ sra@google.com
- sra@chromium.org

[floitsch, 2015-06-18 09:48:20]

LGTM.

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/collection_patch.dart (right):

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:552: 'typeof Map !=
"undefined"');
indent.

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:557: ? new
_LinkedIdentityHashMap<K, V>()
indent by 2 more.

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:615: return isUndefined
? null : value;
why not just return the value?
undefined and null behave the same way in dart2js.

[Harry Terkelsen, 2015-06-18 16:12:37]

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/collection_patch.dart (right):

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:552: 'typeof Map !=
"undefined"');
On 2015/06/18 09:48:19, floitsch wrote:
> indent.

Done.

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:557: ? new
_LinkedIdentityHashMap<K, V>()
On 2015/06/18 09:48:19, floitsch wrote:
> indent by 2 more.

Done.

https://codereview.chromium.org/1188713005/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:615: return isUndefined
? null : value;
On 2015/06/18 09:48:19, floitsch wrote:
> why not just return the value?
> undefined and null behave the same way in dart2js.

good to know

[Harry Terkelsen, 2015-06-18 16:13:58]

Committed patchset #3 (id:40001) manually as
008b41dc3367a5990a1c8dc8aaa7af3851068cf7 (presubmit successful).

[sra1, 2015-06-18 17:01:01]

I'm excited to see results!

https://codereview.chromium.org/1188713005/diff/40001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/collection_patch.dart (right):

https://codereview.chromium.org/1188713005/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:661: // detection, and
iteration can go sour. Don't do that.
I would not say "go sour".  It is ominous and nonspecific.  I would not say
"Don't do that" - it is not clear who "you" is addressing in an internal library
comment. I would say:

Value cycles after 2^30 modifications so that modification counts are always
unboxed (Smi) values. Modification detection will be missed if you make exactly
some multiple of 2^30 modifications between advances of an iterator.

https://codereview.chromium.org/1188713005/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:718: _next = JS('var',
'#.next()', _jsIterator);
Can you make it that you only call #.next() when the user calls moveNext()?

Imagine we had an Es6Iterator that simply wrapped any ES6 JavaScript iterator. 
We would not want to 'be ahead' of where we would be if we ported the code
naively to JavaScript.

If we had an Es6Iterator, how would you make this iterator different? I would
hope we could make the code identical except for the ConcurrentModificationError
check. I'm not suggesting using inheritance or composition, but I am suggesting
making all iterators that wrap JS iterators follow the same pattern.

I think you can get rid of _current and make get current => JS('var', '#.value',
_next);

If you push the #.keys() call to the constructor call site (get iterable) and
pass in _jsIterator instead of _isKeys you will see there is shared code between
_Es6MapIterable.get iterator and forEach.

[Harry Terkelsen, 2015-06-19 01:08:12]

Spent a long time fixing this test:
http://build.chromium.org/p/client.dart/builders/dart2js-safari-mac10.9-2-3-b...

The test checks that an exception is thrown when you try to send a mirror to an
isolate. The current identity map implementation passes the test because when
you try to add the mirror to a map during serialization an exception is thrown
when you try to call hashCode on it's field which is a js function. I had to
specifically check for the mirror case and disallow it with the new identity map
implementation.

https://codereview.chromium.org/1188713005/diff/40001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/collection_patch.dart (right):

https://codereview.chromium.org/1188713005/diff/40001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:661: // detection, and
iteration can go sour. Don't do that.
On 2015/06/18 17:01:01, sra1 wrote:
> I would not say "go sour".  It is ominous and nonspecific.  I would not say
> "Don't do that" - it is not clear who "you" is addressing in an internal
library
> comment. I would say:
> 
> Value cycles after 2^30 modifications so that modification counts are always
> unboxed (Smi) values. Modification detection will be missed if you make
exactly
> some multiple of 2^30 modifications between advances of an iterator.

Done. I also changed the comment that I copied this from.

[Harry Terkelsen, 2015-06-23 21:15:34]

PTAL, had to fix the serialization issue without using 'dart:mirrors'

[floitsch, 2015-06-24 12:46:01]

LGTM.
Why was the serialization a problem?
Does that mean that we currently can't serialize an es6 map?

[Harry Terkelsen, 2015-06-24 19:46:34]

On 2015/06/24 12:46:01, floitsch wrote:
> LGTM.
> Why was the serialization a problem?
> Does that mean that we currently can't serialize an es6 map?

We can serialize an es6 map because it doesn't use serializeDartObject, it uses
serializeMap.

The serialization was a problem because you aren't supposed to be able to
serialize Mirrors. The way that it was "disallowed" before was that a mirror
would serialize using serializeDartObject and then when it tried to serialize
the field that is a JS function it would just throw an error when it tried to
insert it into the identity hashmap (when it tried to get the hashCode). The new
map doesn't check the hashCode and simply adds whatever you give it to the map,
so we have to check for the case when we are trying to serialize a non-Dart
object.

[Harry Terkelsen, 2015-06-24 22:16:53]

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