perf(ngAnimate): avoid repeated calls to addClass/removeClass when an… by Narretz · Pull Request #16613 · angular/angular.js

Narretz · 2018-06-25T16:15:07Z

…imation has no duration

Background:
ngAnimate writes helper classes to DOM elements to see if animations are defined on them. If many
elements have the same definition, we can cache the definition and skip the application of the
helper classes altogether. This helps particularly with large ngRepeat collections.

Closes #14165
Closes #14166

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

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

googlebot · 2018-06-25T16:15:09Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-06-25T16:15:10Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

gkalpak

I won't pretend to understand everything that's going on, but looks reasonable (and there are tests that test stuff 😁)

gkalpak · 2018-06-26T08:01:39Z

src/ngAnimate/animateCache.js

+  this.$get = [function() {
+    return {
+      cacheKey: function(node, method, addClass, removeClass) {
+        var parentNode = node.parentNode;


Isn't it possible for node.parentNode to be undefined?

Good question! You cannot animate elements that are not attached to the document / rootElement, and even an html element has a parentNode, so with this in mind, it's fine.

gkalpak · 2018-06-26T08:07:57Z

src/ngAnimate/shared.js

  if (classes.length) {
    options.preparationClasses = classes;
-    element.addClass(classes);
+    $$jqLite.addClass(element, classes);


Why this change?

gkalpak · 2018-06-26T08:08:57Z

src/ngAnimate/animateCache.js

+        return entry && entry.value;
+      },
+
+      put: function(key, value, isValid) {


What happens when you can put multiple times for the same key with different isValid values?
Why isn't isValid updated?

I don't think a different isValid value is possible for the same cacheKey in one and the same animation run. The cacheKey is based on the classes on the element, and the classes define if there's an animation with duration on the element. Since a cacheKey is unique per class combination, it should not be possible to have different validity per cacheKey.

gkalpak · 2018-06-26T08:12:46Z

src/ngAnimate/animateCss.js


+      // if a css animation has no duration we
+      // should mark that so that repeated addClass/removeClass calls are skipped
+      var hasNoDuration = allowNoDuration || (timings.transitionDuration > 0 || timings.animationDuration > 0);


Shouldn't this be named hasDuration?

gkalpak · 2018-06-26T08:17:48Z

src/ngAnimate/animateCss.js

          if (flags.recalculateTimingStyles) {
-            fullClassName = node.getAttribute('class') + ' ' + preparationClasses;
-            cacheKey = gcsHashFn(node, fullClassName);
+            fullClassName = node.className + ' ' + preparationClasses;


According to MDN:

className can also be an instance of SVGElement if the element is an SVGAnimatedString. It is better to get/set the className of an element using Element.getAttribute and Element.setAttribute if you are dealing with SVG elements.

gkalpak · 2018-06-26T08:23:27Z

src/ngAnimate/animation.js

+            finalAnimations[i][j] = entry.fn;
+
+            // the first row of elements shouldn't have a prepare-class added to them
+            // since the elements are at the top of the animation hierarcy and they


hierarcy --> hierarchy_

gkalpak · 2018-06-26T08:26:50Z

src/ngAnimate/animateCache.js

+        if (removeClass) {
+          parts.push(removeClass);
+        }
+        return parts.join(' ');


Is it a concerns that adding/removing the same class can result in the same key?
E.g. cacheKey(node, merhod, 'foo') === cacheKey(node, method, null, 'foo').

If you try to remove a class, the cacheKey will contain the class to remove twice: once because the class is on the element, once for removeClass.
Since the animate functions bail out early when you try to remove a class that doesn't exist on the element, it's not possible that the key for addClass is the same as for removeClass.

Narretz

Thanks for the thorough review, @gkalpak I don't think the points are actionable, but it's possible that I missed something.

Narretz · 2018-06-27T17:20:03Z

src/ngAnimate/animateCache.js

+  this.$get = [function() {
+    return {
+      cacheKey: function(node, method, addClass, removeClass) {
+        var parentNode = node.parentNode;


Good question! You cannot animate elements that are not attached to the document / rootElement, and even an html element has a parentNode, so with this in mind, it's fine.

Narretz · 2018-06-27T19:54:56Z

src/ngAnimate/animateCache.js

+        if (removeClass) {
+          parts.push(removeClass);
+        }
+        return parts.join(' ');


If you try to remove a class, the cacheKey will contain the class to remove twice: once because the class is on the element, once for removeClass.
Since the animate functions bail out early when you try to remove a class that doesn't exist on the element, it's not possible that the key for addClass is the same as for removeClass.

Narretz · 2018-06-27T20:17:19Z

src/ngAnimate/animateCache.js

+        return entry && entry.value;
+      },
+
+      put: function(key, value, isValid) {


I don't think a different isValid value is possible for the same cacheKey in one and the same animation run. The cacheKey is based on the classes on the element, and the classes define if there's an animation with duration on the element. Since a cacheKey is unique per class combination, it should not be possible to have different validity per cacheKey.

gkalpak

Other than the typos, LGTM 👍

gkalpak · 2018-06-29T07:38:31Z

test/ngAnimate/animateCacheSpec.js

-    var validEntry = { someEssentialProperty: true };
-    var invalidEntry = { someEssentialProperty: false };
+  describe('containsCachedAnimationWithoutDuration', function() {
+    it('should return false if the validity of an key is false', inject(function($$animateCache) {


an key --> a key

…imation has no duration Background: ngAnimate writes helper classes to DOM elements to see if animations are defined on them. If many elements have the same definition, we can cache the definition and skip the application of the helper classes altogether. This helps particularly with large ngRepeat collections. Closes angular#14165 Closes angular#14166

…imation has no duration Background: ngAnimate writes helper classes to DOM elements to see if animations are defined on them. If many elements have the same definition, and the same parent, we can cache the definition and skip the application of the helper classes altogether. This helps particularly with large ngRepeat collections. Closes #14165 Closes #14166 Closes #16613

Narretz added frequency: high component: ngAnimate type: perf labels Jun 25, 2018

Narretz added this to the 1.7.x milestone Jun 25, 2018

Narretz requested a review from gkalpak June 25, 2018 16:15

googlebot added the cla: no label Jun 25, 2018

gkalpak reviewed Jun 26, 2018

View reviewed changes

Narretz commented Jun 28, 2018

View reviewed changes

gkalpak approved these changes Jun 29, 2018

View reviewed changes

Narretz force-pushed the perf-animate-cache branch 2 times, most recently from bc5c234 to b13a6fa Compare July 3, 2018 19:37

Narretz force-pushed the perf-animate-cache branch from b13a6fa to 9463045 Compare July 4, 2018 14:26

Narretz closed this in 0e26197 Jul 5, 2018

This was referenced Jul 6, 2018

Update upstream angular-indonesia/angular.js#159

Merged

Update upstream javascript-indonesias/angular.js#104

Merged

ghost mentioned this pull request Aug 9, 2018

Update 6 dependencies from npm JetBrains/ring-ui#349

Merged

Conversation

Narretz commented Jun 25, 2018

Uh oh!

googlebot commented Jun 25, 2018

Uh oh!

googlebot commented Jun 25, 2018

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Narretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants