Replace arrays and `HashMap` with native `Map` (with a fallback shim implementation) #15114

gkalpak · 2016-09-09T18:00:51Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Perf improvement (from incorporating #13209) and several refactorings.

What is the current behavior? (You can also link to an open issue here)
Same old, same old.

What is the new behavior (if this is a feature change)?
See #13209. On top of that, the internal HashMap/$$HashMap is replaced with native Map/NgMapShim, which should improve performance when native implementations are available (i.e. in most cases) and probably slightly decrease speed when falling back to the shim (tbd).

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

Other information:
This incorporates and builds on top of #13209.

Also rename: - `ES6Map[Shim]` --> `NgMap[Shim]` - `$$HashMap` --> `$$Map`

googlebot · 2016-09-09T18:00:55Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-09-09T18:00:55Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

gkalpak · 2016-09-09T18:08:40Z

I haven't checked how these changes affect performance for various cases.

Things that are affected (and the effects are likely to be noticeable):

copy() (and anything using copy() internally).
ngAnimate

Things that are affected (and the effects are unlikely to be noticeable):

$injector
select
ngMocks

gkalpak · 2016-09-09T18:10:09Z

Safari 8 is failing. Buggy implementation of window.Map?

jbedard · 2016-09-09T18:14:20Z

src/Angular.js

    var idx = this._idx(key);
    if (idx === -1) {
      idx = this._lastIndex = this._keys.length;
-      this._lastKey = key;


Won't deleting this line break something? Every assignment of lastIndex must be beside an assignment to lastKey (and the reverse)...

this._lastKey has been set to key in the previous call to _idx().
We need to re-assign this._lastIndex here, because the currently set -1 won't be true any more (since we are adding the item), but this._lastKey stays the same.

Yeah, looks like you're right. That isn't really obvious just looking at this though. Worth putting it back? Or more likely just a comment?

It might not be obvious if you are trying to figure out why it is being deleted, but if you look at the code (without knowing it was ever there), I think it is clearer without the comment.
I am fine adding if you still think it helps, though.

jbedard · 2016-09-09T18:23:07Z

Only thing I can see is safari not supporting constructor arguments: https://kangax.github.io/compat-table/es6/#test-Map

gkalpak · 2016-09-09T18:43:49Z

Actually, it could also be a buggy implementation of something unrelated to Map, e.g. splice.
I'll take a look on SauceLabs (since I don't have local access to Safari 8 😞).

jbedard · 2016-09-09T18:51:08Z

src/AngularPublic.js

@@ -262,7 +262,7 @@ function publishExternalAPI(angular) {
        $window: $WindowProvider,
        $$rAF: $$RAFProvider,
        $$jqLite: $$jqLiteProvider,
-        $$HashMap: $$HashMapProvider,
+        $$Map: $$MapProvider,


Do we even need this? If we're going to rename it (breaking change), can we just delete it?

This is a private service, so no breaking change (theoretically). It is used by ngAnimate (which doesn't have access to the closure).

gkalpak · 2016-09-09T19:13:50Z

I couldn't find anything suspicious with Safari 8 + Map. I restarted the job (just in case).

UPDATE: Still failing 😞

thorn0 · 2016-09-10T12:04:08Z

It's Mobile Safari, not the desktop 8.0 one. It says Mobile Safari 8.0.0 (iOS 10.10.5). The tests for #15119 (a docs-only PR!) are also failing in this browser. It might be something like this jquery/jquery#2145

mgol · 2016-09-10T12:39:55Z

It says Mobile Safari 8.0.0 (iOS 10.10.5)

I assume they mean OS X 10.10.5, not iOS 10.10.5... Sauce Labs should make it less confusing.

mgol · 2016-09-10T12:40:55Z

...but that would mean they're running it on an iOS simulator on OS X and that, in turn, eliminates the bug you linked to as it only manifested itself on real devices.

thorn0 · 2016-09-10T12:51:08Z

I'm just saying it may be a similar randomly appearing bug, not this specific one. Yes, it's a simulator, and it's even not totally clear which version of iOS it simulates.

gkalpak · 2016-09-10T21:01:33Z

The Map implementation in Mobile Safari 8 is really bad (see also google/traceur-compiler#1810 (comment)). Although we might be able to work around this specific failure, I am afraid other incomplete or buggy implementations will cause Angular to fail unexpectedly (and randomly).

Maybe we need stricter checks for Map (e.g. es6-shim has many more).

mgol · 2016-09-10T21:13:38Z

@gkalpak The comment only mentions that the Map constructor doesn't accept parameters (which we already know) and that its iterator is non-compliant (but we don't use it anyway, do we?). Any other problems?

gkalpak · 2016-09-10T21:42:05Z

Our specific problem (which I am not exaclty sure what it is) is indeed not mentioned in the comment, but it is a discussion about the incompleteness of the implementation of Map in that version of Safari.

So, although we don't need any of the known missing or buggy features, we are affected by some other bug, which results in not being able to retrieve values associated with jqLite objects as keys. Unfortunately, since it's iterators do not have a .next() method, nor does it support ~~for...of or~~_[update: I just realized I didn't try for...of]_ Array.from() and its forEach() method iterates over the values, I couldn't find a way to inspect the contents of the Map closer.

If anyone is interested, in order to test this, I changed the code to:

Always use NgMapShim (even if window.Map was available).
Instantiate a window.Map within every NgMapShim.
For every operation (get/set/delete) that was performed with the NgMapShim instance, perform the same operation on the native Map, compare the results and report any differences.

There were a bunch of differences (in GET operations only), where the native Map returned undefined, while the shim returned something - in all cases the sizes of the two maps where the same (and although I haven't kept proof, the values were there as well - can't tell anything about the keys).

Here is a Travis log showing (some of) the above (not sure how long Travis keeps old logs around).

gkalpak · 2016-09-10T21:49:44Z

BTW, I updated the commit to use the shim if (new Map()).keys().next is not a function and the tests are green. But I am still not sure how safe it is to rely on native implementations 😞

jbedard · 2016-09-10T21:50:28Z

Originally I had a test method before switching to essentially isNative(window.Map). Maybe we do need that to detect these these buggy implementations? The tests will have to catch errors like this one, whatever this one is...

jbedard · 2016-09-10T21:51:38Z

Why specifically (new Map()).keys().next? I assume that makes IE11 not pass either? EDIT: I see you did this check only if keys exists, so IE11 will probably still pass...

gkalpak · 2016-09-10T22:01:43Z

Why specifically (new Map()).keys().next? I assume that makes IE11 not pass either?

True 😞 There is nothing specific about it, except that it was a way to detect Safari 8 (and probably other browsers with incomplete implementations - but definitely not all).

I am fine doing the detection in another, more targeted way, but my main concern remains:
Are we confident enough that our tests would have caught any issues with incomplete/buggy Map implementations?
(I don't think I am. For example, note there are dozens of inconsistencies between our shim and the native Map on Mobile Safari 8 (see that log if still available), but only one test fails when using the native one. Who says there aren't more failures, which our tests didn't happen to catch?)

jbedard · 2016-09-10T22:07:02Z

I'd prefer finding ways of detecting specific bugs, instead of just detecting Safar8-like implementations. But then it might get too ugly, having a test that creates jqLite("<input class='a b c'>") etc...

That said, if this allows us to replace HashMap it might be worth it. Unlike originally where this was only for copy. I think it might be worth it...

jbedard · 2016-09-10T22:18:03Z

Looking through the es6-shim checks I don't see anything looking for a bug like this :|

thorn0 · 2016-09-10T22:19:27Z

It'd be interesting to run test262 in this simulator, but what about real iphone and ipads, not simulated? Is Angular actually tested on them?

gkalpak · 2016-09-12T12:56:15Z

I ran the ngBind version of largetable benchmark (on Chrome) with $$HashMap, window.Map and NgMapShim. This benchmark has a lot of ngRepeated nodes, which use $animate.enter(), which adds/removes the ng-enter/leave[-active] classes, which in turn uses the Map-based queue under the hood. This is probably the most common way (along with copy()ing deeply watched objects) that Angular will be exercising the Map.

I didn't see any consclusive difference between the three (which probably means that the time spent interacting with Map is not significant in the big picture). For reference, here are the values:

	`$$HashMap`	`window.Map`	`NgMapShim`
destroy	178 ms	173 ms	188 ms
create	949 ms	954 ms	924 ms
$apply	11 ms	10 ms	12 ms

@jbedard, how did you meassure the performance difference for copy()?

jbedard · 2016-09-12T15:45:00Z

f7bf118 is the copy benchmark I normally use

gkalpak · 2016-09-30T17:55:07Z

@jbedard, was that benchmark included into angular/angular.js at some point or are you using it locally?

jbedard · 2016-10-04T03:41:26Z

That benchmark is not checked in and I just use it locally. At some point I committed it to a local branch to show someone the benchmark I was using, so github does know about the commit but it was never merged.

googlebot · 2016-12-09T10:56:23Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-12-09T10:56:23Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

gkalpak · 2016-12-09T11:05:23Z

We have decided to:

Replace the current HashMap implementation with the NgMapShim implementation that is closer to the native Map API.
Look again in the future at whether to make use of native Map, once it has stabilised in more browsers.

gkalpak · 2016-12-09T13:30:26Z

Closing in favor of #15483.

jbedard and others added 7 commits September 9, 2016 16:34

perf(copy): use ES6 Map to track source/destination objects

1c187d7

fixup! perf(copy): Saving the last key/index lookup in ESMapShim

8f4eab4

fixup! perf(copy): replace Map feature-test with native-test

95f9179

refactor(copy): avoid creating source/dest Map for simple copies

4db3ac1

refactor(HashMap): use ES6Map under the hood

fdba297

refactor(*): replace HashMap with NgMap (former ES6Map)

a83f60e

Also rename: - `ES6Map[Shim]` --> `NgMap[Shim]` - `$$HashMap` --> `$$Map`

test(hashKey): add tests for hashKey()

dcbb707

gkalpak added frequency: high needs: review type: refactor component: misc core type: perf type: test labels Sep 9, 2016

gkalpak added this to the Backlog milestone Sep 9, 2016

googlebot added the cla: no label Sep 9, 2016

gkalpak mentioned this pull request Sep 9, 2016

perf(copy): use ES6 Map to track source/destination objects #13209

Closed

jbedard reviewed Sep 9, 2016
View reviewed changes

gkalpak force-pushed the refactor-HashMap-use-ES6Map branch 3 times, most recently from f4ba0e3 to 999138a Compare September 10, 2016 20:55

fixup! refactor(*): replace HashMap with NgMap (former ES6Map)

604d876

gkalpak force-pushed the refactor-HashMap-use-ES6Map branch from 999138a to 604d876 Compare September 10, 2016 21:23

gkalpak changed the title ~~WIP - Replace arrays and HashMap with native Map (with a fallback shim implementation)~~ Replace arrays and HashMap with native Map (with a fallback shim implementation) Sep 12, 2016

gkalpak modified the milestones: 1.6.x, Backlog Sep 12, 2016

fixup! refactor(*): replace HashMap with NgMap (former ES6Map)

a7f8d63

gkalpak mentioned this pull request Dec 9, 2016

refactor(*): replace HashMap with NgMap #15483

Closed

3 tasks

gkalpak closed this Dec 9, 2016

gkalpak deleted the refactor-HashMap-use-ES6Map branch December 9, 2016 13:36

Replace arrays and HashMap with native Map (with a fallback shim implementation) #15114

Replace arrays and HashMap with native Map (with a fallback shim implementation) #15114

Conversation

gkalpak commented Sep 9, 2016

googlebot commented Sep 9, 2016

googlebot commented Sep 9, 2016

gkalpak commented Sep 9, 2016

gkalpak commented Sep 9, 2016

jbedard Sep 9, 2016

Choose a reason for hiding this comment

gkalpak Sep 9, 2016

Choose a reason for hiding this comment

jbedard Sep 9, 2016

Choose a reason for hiding this comment

gkalpak Sep 9, 2016 • edited Loading

Choose a reason for hiding this comment

jbedard commented Sep 9, 2016 • edited Loading

gkalpak commented Sep 9, 2016

jbedard Sep 9, 2016

Choose a reason for hiding this comment

gkalpak Sep 9, 2016

Choose a reason for hiding this comment

gkalpak commented Sep 9, 2016 • edited Loading

thorn0 commented Sep 10, 2016

mgol commented Sep 10, 2016

mgol commented Sep 10, 2016

thorn0 commented Sep 10, 2016

gkalpak commented Sep 10, 2016

mgol commented Sep 10, 2016

gkalpak commented Sep 10, 2016 • edited Loading

gkalpak commented Sep 10, 2016

jbedard commented Sep 10, 2016

jbedard commented Sep 10, 2016 • edited Loading

gkalpak commented Sep 10, 2016 • edited Loading

jbedard commented Sep 10, 2016

jbedard commented Sep 10, 2016

thorn0 commented Sep 10, 2016

gkalpak commented Sep 12, 2016 • edited Loading

jbedard commented Sep 12, 2016

gkalpak commented Sep 30, 2016

jbedard commented Oct 4, 2016

googlebot commented Dec 9, 2016

googlebot commented Dec 9, 2016

gkalpak commented Dec 9, 2016

gkalpak commented Dec 9, 2016

Replace arrays and `HashMap` with native `Map` (with a fallback shim implementation) #15114

Replace arrays and `HashMap` with native `Map` (with a fallback shim implementation) #15114

gkalpak Sep 9, 2016 •

edited

Loading

jbedard commented Sep 9, 2016 •

edited

Loading

gkalpak commented Sep 9, 2016 •

edited

Loading

gkalpak commented Sep 10, 2016 •

edited

Loading

jbedard commented Sep 10, 2016 •

edited

Loading

gkalpak commented Sep 10, 2016 •

edited

Loading

gkalpak commented Sep 12, 2016 •

edited

Loading