dart2js: use Es6 maps when available.

dart2js: use Es6 maps when available.

R=herhut@google.com, sra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45105
Reverted: https://code.google.com/p/dart/source/detail?r=45107

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

[Vyacheslav Egorov (Google), 2015-03-25 08:43:52]

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

[Vyacheslav Egorov (Google), 2015-03-25 08:43:52]

dbc

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

https://codereview.chromium.org/1032783003/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:124: var bucket =
JS('var', '#[#]', rest, hash);
should use _getBucketHere

[floitsch, 2015-03-25 23:58:15]

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

https://codereview.chromium.org/1032783003/diff/20001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:124: var bucket =
JS('var', '#[#]', rest, hash);
On 2015/03/25 08:43:52, Vyacheslav Egorov (Google) wrote:
> should use _getBucketHere

applied your patch.

[floitsch, 2015-03-26 00:01:15]

floitsch@google.com changed reviewers:
+ het@google.com, sra@google.com
- vegorov@google.com

[floitsch, 2015-03-26 00:01:15]

Brings a nice speedup on v8.
However, on Slava's jsshell it degrades the performance of jsshell. My tests
with FF36.0.4 (released a few days ago) also shows an improvement. But that's
mainly because the baseline is way worse than the one Slava measured.

We could always make this a browser-specific flag (use Es6Maps in Chrome, but
not in other browsers).

[Vyacheslav Egorov (Google), 2015-03-26 00:03:57]

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

[Vyacheslav Egorov (Google), 2015-03-26 00:03:58]

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

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:530: Map map =
JsLinkedHashMap.supportsEs6Maps
Can this just call some force-inline static method? cause seems like a bit of
duplication going on (there are three places with the same condition.

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart (left):

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:256: return key is String
&& key != '__proto__';
We need to check that all browsers switched to ES6 handling of __proto__ (made
it an accessor on the Object.prototype) before making this change.

[Harry Terkelsen, 2015-03-26 00:40:07]

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

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:530: Map map =
JsLinkedHashMap.supportsEs6Maps
On 2015/03/26 00:03:57, Vyacheslav Egorov (Google) wrote:
> Can this just call some force-inline static method? cause seems like a bit of
> duplication going on (there are three places with the same condition.

Won't that require generic methods?

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:547: 
unnecessary blank line

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:576: 
unnecessary blank line

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart (right):

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:21: var _strings;
shouldn't the ES6 map obviate the need for these 3 parts? maybe also override
all methods that switch on key type

[floitsch, 2015-03-26 00:53:24]

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

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:530: Map map =
JsLinkedHashMap.supportsEs6Maps
On 2015/03/26 00:03:57, Vyacheslav Egorov (Google) wrote:
> Can this just call some force-inline static method? cause seems like a bit of
> duplication going on (there are three places with the same condition.

:)
I wondered the same, and had already started. The problem is, that functions
can't have generic types. So it would need to be a factory constructor.
That was my initial implementation (dispatching inside the JsLinkedHashMap
factory constructor). But that requires other changes all over the place,
because JsLinkedHashMap is subclassed.

One good alternative would be to create a named factory constructor in
JsLinkedHasMap. I just couldn't come up with a good name yet.

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:547: 
On 2015/03/26 00:40:07, Harry Terkelsen wrote:
> unnecessary blank line

Done.

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/collection_patch.dart:576: 
On 2015/03/26 00:40:06, Harry Terkelsen wrote:
> unnecessary blank line

Done.

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart (left):

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:256: return key is String
&& key != '__proto__';
On 2015/03/26 00:03:58, Vyacheslav Egorov (Google) wrote:
> We need to check that all browsers switched to ES6 handling of __proto__ (made
> it an accessor on the Object.prototype) before making this change.

I just tried with IE10 and there it works. I would be surprised if there was any
major browser that did it wrong.

var o = Object.create(null);
o.__proto__ = { foo: 499 };
document.write(o.foo);
for (var x in o) document.write(x);

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart (right):

https://codereview.chromium.org/1032783003/diff/60001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:21: var _strings;
On 2015/03/26 00:40:07, Harry Terkelsen wrote:
> shouldn't the ES6 map obviate the need for these 3 parts? maybe also override
> all methods that switch on key type

yes, probably. But I want to keep the changes small.
If we removed the individual maps, then the polyfill would need to be more
involved.

[Vyacheslav Egorov (Google), 2015-04-07 20:34:05]

One thing that we seemed to miss in our benchmarking is that Map.prototype.get
with a *symbol* key can be slower than obj[key] because later succeeds in the
stub, and the former goes to Map implementation (which does not seem to be
optimal).

See https://code.google.com/p/v8/issues/detail?id=4014

So me might want to packpedal on this - because this is going to penalize use
case where symbol keys lookups are dominating.

[floitsch, 2015-04-09 11:06:55]

PTAL.
I made the ES6 map dependent on a compiler flag.
This allows us to test it in customer code without committing to them yet.

[floitsch, 2015-04-13 08:09:58]

ping.

[herhut, 2015-04-13 12:58:32]

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

[herhut, 2015-04-13 12:58:33]

https://chromiumcodereview.appspot.com/1032783003/diff/140001/sdk/lib/_intern...
File sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart (right):

https://chromiumcodereview.appspot.com/1032783003/diff/140001/sdk/lib/_intern...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:46: if (_USE_ES6_MAPS &&
JsLinkedHashMap._supportsEs6Maps) {
Type inference will not currently understand this pattern but hopefully type
propagation will get rid of the constructor call, so that the code in the end
does not contain the implementation for the ES6 map if usage is disabled.

https://chromiumcodereview.appspot.com/1032783003/diff/140001/sdk/lib/_intern...
sdk/lib/_internal/compiler/js_lib/linked_hash_map.dart:269: return key is
String;
Nice!

[herhut, 2015-04-13 14:20:00]

lgtm

[floitsch, 2015-04-13 14:27:33]

Committed patchset #8 (id:140001) manually as 45105 (presubmit successful).

[floitsch, 2015-04-14 17:55:02]

floitsch@google.com changed reviewers:
- het@google.com, vegorov@google.com

[floitsch, 2015-04-14 17:55:03]

Had to revert.

PTAL.
I'm now using the "NoSideEffects" annotation. (I ran through all tests, and some
manual tests, and it seems to have the required semantics).

Normally the type-inferrer should figure out by itself that the map-creation
doesn't have any side-effect, but that doesn't work, because the small
conditional increases the size of the function, and the type-inferrer now
schedules it's analyses after the analysis of LinkedHashMap._empty. This means
that it will assume that the function has side-effects, just because it hasn't
seen it yet. Except for fixing the simple-type-inferrer there is not much that
can be done (except for artificially increasing the size of _empty).

[sra1, 2015-04-14 20:32:07]

lgtm.

I see that we still have LinkedHashMapCell and separate maps for strings and
numbers.  Under what conditions can we use a single ES6 Map directly?

[floitsch, 2015-04-15 09:14:59]

On 2015/04/14 20:32:07, sra1 wrote:
> lgtm.
> 
> I see that we still have LinkedHashMapCell and separate maps for strings and
> numbers.  Under what conditions can we use a single ES6 Map directly?

Eventually we should have a JS polyfill for the ES6 Map, and just use it
unconditionally.
ES6 Maps use '===' for comparison, except for "NaN" (so that you can look up
NaNs in the map). This is different to Dart, where NaN != NaN and therefore it
isn't possible to find your NaN in the table again.
The moment something overrides hashCode or equality we have to use our own
implementation.
Note: if something only overrides equality we still have to use our own
implementation (but only because an == implementation could always return false
for it's identity, like the NaN does).

[floitsch, 2015-04-15 10:45:03]

Committed patchset #10 (id:180001) manually as 45158 (presubmit successful).