refactor(*): replace `HashMap` with `NgMap` #15483

gkalpak · 2016-12-09T13:28:21Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactoring.

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)?
HashMap is replaced with NgMap, which for the time being points to NgMapShim, an API-compatible Map implementation (for the features Angular needs). This will allow us to easily switch to native implementations once they become more stable.

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:
See #13209 and #15114 for related discussions.

For the time being, we will be using `NgMap`, which is an API-compatible implementation of native `Map` (for the features required in Angular). This will make it easy to switch to using the native implementations, once they become more stable. Note: At the momntm some native implementations are still buggy (often in subtle ways) and can cause hard-to-debug failures.)

gkalpak · 2016-12-09T13:33:07Z

BTW, this is the same code as in #15114, minus #13209. The ES6Map part of #13209 is irrelevant now. The perf(copy) part of #13209 (which is still relevant) can be rebased and merged once this has been merged.

dcherman · 2016-12-09T15:33:54Z

src/apis.js

  this.$get = [function() {
-    return HashMap;
+    return NgMap;


Can we feature detect native Map implementation and just use that if present? It should be significantly faster than this implementation since lookups are not O(n), and all modern browsers support it at this point. This fallback is really just for like...IE9 and IE10.

So I just saw the comments above because reading fail. What subtle bugs in native Maps exist such that we shouldn't use them for perf, other than IE11 return values not conforming to the spec? Can we add references to filed issues such that when these bugs are fixed, we know that it's safe to migrate?

There is some discussion in #15114. It is not as much about specific bugs as it is about our lack of confidence that there aren't any (or that our tests would have caught them).

(For example, there was some Safari 8 bug (which I couldn't even figure out what it was) that caused our tests to fail, even if the required features were theoretically available.)

With that logic, the native Map will never be used as there is no concrete way to judge whether or not they're "ready" - that's why I was suggesting we put some kind of comment in here that "when these things are fixed, re-evaluate"

"when these things are fixed, re-evaluate"

Sounds great. If we only knew what these things were... 😞

dcherman · 2016-12-09T15:36:25Z

So I know we were debating this in that other issue, but I still think an implementation closer to $$HashMap is more ideal with property lookups than using dual arrays for perf reasons.

It would not be a breaking change since due to how it worked, the old HashMap implementation could never support frozen objects anyway since it did a property assignment on them. This mainly affects IE9/IE10 though since in newer browsers, we should just use the native implementation.

gkalpak · 2016-12-09T16:00:11Z

According to my understanding, the discussion on #13209 was not conclusive on whether our implementation of HashMap is faster than our implementation of NgMap. Anyone feels like benchmarking? 😃

Furthermore, this benchmarking would only make sense for copy(), because based on #15114 (comment) other uses of NgMap don't seem to have a noticeable impact on performance. (And this PR does not affect copy(), so performance-wise it should make no difference overall.)

The main benefit of using NgMap imo is that it is easy to swap it for window.Map when we feel we're ready. Of course, we could change the underlying implementation of NgMapShim to use a HashMap if we find out it offers better performance, but keeping a window.Map-compatible interface is a good thing imo.

I suggest we merge this (which is just a refactoring - not a perf change) and debate performance on #13209 (which is a perf change).

dcherman · 2016-12-09T16:10:51Z

JSPerf is down at the moment so I can't do any basic tests, but do you have any stats on what the average size of a Map is versus the max size? If N is low enough in your use cases, it's possible that it's fast enough, but I suspect that it'd breakdown with larger maps. Will try to test once I can make some benchmarks.

No matter what we do, I am 100% in agreement that we should refactor any APIs to use a Map compatible interface. As long as it's present in the injector as well rather than being a closure variable, applications could choose to override the private $$Map factory with the native one if they deem it important so long as they understand that they're tinkering with private details that are subject to change at any time.

gkalpak · 2016-12-09T16:46:26Z

As long as it's present in the injector as well rather than being a closure variable, applications could choose to override the private $$Map factory with the native one if they deem it important so long as they understand that they're tinkering with private details that are subject to change at any time.

I assume "people" is "you" 😛 The problem is that not all cases retrieve NgMap via DI. Off the top of my head, NgMap is used in the following places:

ngAnimate
ngOptions
createInjector()

(In the future, it may also be used by copy().)

ngAnimate already uses DI and ngOptions can be changed to use it too.
createInjector() obviously can't and I don't think copy() (which is probably the most critical performance-wise) can either.

dcherman · 2016-12-09T17:49:35Z

I assume "people" is "you"

Yep. I tinker with a lot of internals for fun/performance sometimes, but always with the understanding that nothing I do is supported. Almost have an entire implementation of Observable bindings working that only participate in the digest cycle when necessary - including interpolation in templates and a new $ binding in components 😈

Anyway, I will try to run some basic perf tests when JSPerf is back up.

jbedard · 2016-12-09T18:06:40Z

test/auto/injectorSpec.js

-//    //
-//   s6
-
+    //          s1


This looks unrelated?

Wrong indentation is always relevant! 😛

Narretz · 2017-01-21T18:12:24Z

test/helpers/testabilityPatch.js

-  }
-  window.document.$$hashKey = null;
+  // These Nodes are persisted across tests.
+  // They used to be assigned a `$$hashKey` when animated, but not any more.


I find this could be more explicit. If you read this in a year , would you know hat this means?

You wouldn't? 😱
Updated the comment just in case 😛

gkalpak · 2017-01-24T22:17:39Z

PTAL

Narretz · 2017-01-25T19:14:20Z

LGTM

For the time being, we will be using `NgMap`, which is an API-compatible implementation of native `Map` (for the features required in Angular). This will make it easy to switch to using the native implementations, once they become more stable. Note: At the moment some native implementations are still buggy (often in subtle ways) and can cause hard-to-debug failures.) Closes #15483

For the time being, we will be using `NgMap`, which is an API-compatible implementation of native `Map` (for the features required in Angular). This will make it easy to switch to using the native implementations, once they become more stable. Note: At the moment some native implementations are still buggy (often in subtle ways) and can cause hard-to-debug failures.) Closes angular#15483

gkalpak added 2 commits December 9, 2016 15:07

test(hashKey): add tests for hashKey()

a34944d

googlebot added the cla: yes label Dec 9, 2016

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

gkalpak added this to the Backlog milestone Dec 9, 2016

gkalpak mentioned this pull request Dec 9, 2016

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

Closed

3 tasks

dcherman reviewed Dec 9, 2016

View reviewed changes

jbedard reviewed Dec 9, 2016

View reviewed changes

Narretz reviewed Jan 21, 2017

View reviewed changes

fixup! refactor(*): replace HashMap with NgMap

bc26d06

gkalpak closed this in 641e13a Jan 25, 2017

gkalpak deleted the refactor-HashMap-use-NgMap branch April 27, 2017 11:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(*): replace `HashMap` with `NgMap` #15483

refactor(*): replace `HashMap` with `NgMap` #15483

gkalpak commented Dec 9, 2016

gkalpak commented Dec 9, 2016

dcherman Dec 9, 2016

dcherman Dec 9, 2016

gkalpak Dec 9, 2016

gkalpak Dec 9, 2016

dcherman Dec 9, 2016

gkalpak Dec 9, 2016

dcherman commented Dec 9, 2016

gkalpak commented Dec 9, 2016

dcherman commented Dec 9, 2016

gkalpak commented Dec 9, 2016

dcherman commented Dec 9, 2016

jbedard Dec 9, 2016

gkalpak Dec 10, 2016

Narretz Jan 21, 2017

gkalpak Jan 24, 2017

gkalpak commented Jan 24, 2017

Narretz commented Jan 25, 2017

-              //    //
-              //   s6
+                  //          s1

refactor(*): replace HashMap with NgMap #15483

refactor(*): replace HashMap with NgMap #15483

Conversation

gkalpak commented Dec 9, 2016

gkalpak commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcherman commented Dec 9, 2016

gkalpak commented Dec 9, 2016

dcherman commented Dec 9, 2016

gkalpak commented Dec 9, 2016

dcherman commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Jan 24, 2017

Narretz commented Jan 25, 2017

refactor(*): replace `HashMap` with `NgMap` #15483

refactor(*): replace `HashMap` with `NgMap` #15483