perf(*): don't trigger digests after enter/leave of structural directives #15345

Narretz · 2016-11-01T12:58:13Z

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

What is the current behavior? (You can also link to an open issue here)
Structural animations such as ngIf trigger an additional digest after $animate.leave completes

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)~~

dcherman · 2016-11-01T15:43:39Z

👏

Nitpicky, but unnessecary --> unnecessary in the commit message. ❤️

gkalpak

An alternative, more radical approach (involving a BC) would be using $$q instead of $q for the AnimateRunner and let people manually call $apply() if their callback needs it. Most of the time, the animation callbacks trigger DOM-related operations/cleanup (and not touch on business logic or data), so a digest should not be necessary.

Unfortunately, such a BC has to wait for 1.7 now 😞

gkalpak · 2016-11-01T15:46:10Z

src/ng/directive/ngIf.js

@@ -115,7 +115,7 @@ var ngIfDirective = ['$animate', '$compile', function($animate, $compile) {
            }
            if (block) {
              previousElements = getBlockNodes(block.clone);
-              $animate.leave(previousElements).then(function() {
+              $animate.leave(previousElements).done(function() {


This is not functionally the same as .then(). We need to check for success/error.
(Not sure if it makes a difference in practice.)

Doh ... you are right, I'll change that.

gkalpak · 2016-11-01T15:53:11Z

src/ng/directive/ngSwitch.js

-        for (i = 0, ii = previousLeaveAnimations.length; i < ii; ++i) {
+
+        // Start from the end, in case the array is modified during the loop
+        for (i = previousLeaveAnimations.length - 1; i >= 0; i--) {


This change should not be necessary if we call the .done() callback on success only.

Narretz · 2016-11-01T16:07:58Z

@gkalpak Using $$q and letting people do $apply manually was implemented before and was changed specifically to use digest-integrated promises. I don't know if this was actually an issue for people, but I assume that it at least makes the behavior consistent with $http, and $q issued promises.

Narretz · 2016-11-04T17:00:30Z

@gkalpak This is ready for another review :)

gkalpak

LGTM with the exception of one comment/question.

gkalpak · 2016-11-04T22:36:55Z

src/ng/animate.js

-        if (runner.end) {
-          runner.end();
+        if (runner.cancel) {
+          runner.cancel();


I think this is a good change (i.e. the right thing to do), but isn't it a breaking change.
I couldn't find the behavior documented anywhere btw.

If it's not documented that it's not a change, so also no Breaking Change :>
Seriously though, I thought this change would only align ng.$animate with ngAnimate.$animate, but it turns out cancelling an actual animation resolves the promise.
The reason why I change this code is because otherwise .done isn't called with false, so some switch tests fail. The problem is that these ngSwitch tests are "synthetic" in the sense that they never complete the "animations" they start because the rAFs aren't flushed. I think if we did that, then we didn't have to do this change. However, we would have to insert raF flushes in about 8 ngSwitch tests where they actually don't need to be there from considering the test's scope (these are not animation tests).

I'd be okay with introducing promise rejection on cancellation as a breaking change for 1.7 on both ng + ngAnimate, but that would mean this perf improvemtn has to wait ... at least for ngSwitch.

I thought this change would only align ng.$animate with ngAnimate.$animate

Isn't it the exact same $animate that is being used. Only underlying pieces (e.g. $$animateQueue) are different with ngAnimate. So, whether we change it or not, $animate will be consistent with and without ngAnimate.

The reason why I change this code is because otherwise .done isn't called with false, so some switch tests fail.

It sounds more like a test issue, not an implementation issue.

I'd be okay with introducing promise rejection on cancellation as a breaking change for 1.7 on both ng + ngAnimate, but that would mean this perf improvemtn has to wait

Sorry, I didn't get why. Apart from some (bad) tests failing, what is the problem with using .done() (but keeping the runner.end() call above)?

If $animate.cancel doesn't reject the promise, then .done callback handlers aren't called with false, which means in the ngSwitch done handler, I cannot distinguish between failed and successful animations, which leads to "cannot read function .end of undefined" error I had at the beginning. I then changed the code in ngSwitch to read the stored runners from the end to work around this problem, but then you said, I should check for successful animations inside done, which is how I found this problem with cancel in the first place

It's probably easier to understand if you run the tests with the various changes to see the different behavior.

I still don't understand why it worked before but not now. Previously, .then(cb) was used. This is implemented in $animate like this (code changed for brevity):

then: resolveHandler => { return $q((resolve, reject) => this.done(status => status === false ? reject() : resolve())). then(resolveHandler) }

So, afaict we should be able to safely replace .then(cb) with .done(status => status !== false && cb()) keeping the exact same behavior minus the extra digest (and asynchronicity) of $q.

Is the fact that the callback is now synchronously called causing the problem?

I don't see why my fix would fail. It doesn't rely on the index but on the item itself (looking it up via previousLeaveAnimations.indexOf(...)). So, it is not affected by structural changes to the array.

Removing in reverse order should also work. (Why did I say I didn't like that?)

Because the error isn't coming from the splicer but from the cancel loop: http://plnkr.co/edit/MPUiYm4mTr9nNwg8SRS3?p=preview (Apologies if I'm being dense, it's very late again :D)

Right. I think we have two problems (and my suggestion only takes care of one of them). So, in addition we should also change the cancellation code as follows (demo):

// Before: for (i = 0, ii = previousLeaveAnimations.length; i < ii; ++i) { $animate.cancel(previousLeaveAnimations[i]); } previousLeaveAnimations.length = 0; // After: while (previousLeaveAnimations.length) { $animate.cancel(previousLeaveAnimations.shift()); }

In any case, I think it is fine to traverse the array backwards as an alternative (if it doesn't create other problems).

But do we need the splice change? I don't see any errors without it.

I don't think we need the slice change if we traverse the array backwards. Either backwards-array-traversal or slice-change+while-loop. Up to you 😃

gkalpak

A couple of minor comments. LGTM otherwise.

gkalpak · 2016-11-14T09:15:31Z

test/ng/directive/ngSwitchSpec.js

@@ -333,6 +333,36 @@ describe('ngSwitch', function() {
  }));


+  it('should handle changes to the switch value in a digest loop with multiple value matches',
+    inject(function($$rAF, $compile, $rootScope, $timeout) {


Are $$rAF and $timeout needed?

gkalpak · 2016-11-14T09:18:17Z

test/ng/directive/ngSwitchSpec.js

    $rootScope.select = 2;
    $rootScope.$apply();
+    $$rAF.flush();


Now that 696b7a4 has been added, are these extra $$rAF calls necessary?

gkalpak · 2016-11-14T09:23:42Z

test/ng/directive/ngSwitchSpec.js

+
+
+  it('should handle changes to the switch value in a digest loop with multiple value matches',
+    inject(function($$rAF, $compile, $rootScope, $timeout) {


Are $$rAF and $timeout necessary?

Narretz · 2016-11-14T19:12:07Z

Good catch, no, these flushes are not necessary. The only one necessary is in the test that ensures that no digest is triggered after the leave animation

petebacondarwin · 2016-11-21T16:28:08Z

OK, are we happy to merge this?

gkalpak · 2016-11-21T17:50:35Z

I am 😃

petebacondarwin · 2016-11-21T18:34:58Z

Please do: 1.5.x and master?

…ives ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView). Previously, they were using promise callbacks (`then`), which caused an unnessecary digest. Background: In angular@bf0f550, animation promises where introduced instead of animation callbacks. These promises were however not tied to the digest cycle, so you had to manually call `$apply` inside them. This was changed in angular@c8700f0, so animation promise callbacks would always trigger a `$digest`. This meant that several built-in directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude, ngView). The `done` callback, which receives a single argument indicating success / failure was introduced to allow digest-less responses, but wasn't applied to these directives. Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and no actual animations are running, because the animation runner code is in the core ng module. Fixes angular#15322 Closes angular#15345

…ives ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView). Previously, they were using promise callbacks (`then`), which caused an unnessecary digest. Background: In bf0f550, animation promises where introduced instead of animation callbacks. These promises were however not tied to the digest cycle, so you had to manually call `$apply` inside them. This was changed in c8700f0, so animation promise callbacks would always trigger a `$digest`. This meant that several built-in directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude, ngView). The `done` callback, which receives a single argument indicating success / failure was introduced to allow digest-less responses, but wasn't applied to these directives. Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and no actual animations are running, because the animation runner code is in the core ng module. Fixes #15322 Closes #15345

…ives ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView). Previously, they were using promise callbacks (`then`), which caused an unnessecary digest. Background: In angular@bf0f550, animation promises where introduced instead of animation callbacks. These promises were however not tied to the digest cycle, so you had to manually call `$apply` inside them. This was changed in angular@c8700f0, so animation promise callbacks would always trigger a `$digest`. This meant that several built-in directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude, ngView). The `done` callback, which receives a single argument indicating success / failure was introduced to allow digest-less responses, but wasn't applied to these directives. Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and no actual animations are running, because the animation runner code is in the core ng module. Fixes angular#15322 Closes angular#15345

Narretz added frequency: high component: ngAnimate component: misc core type: perf labels Nov 1, 2016

Narretz added this to the 1.5.9 milestone Nov 1, 2016

googlebot added the cla: yes label Nov 1, 2016

Narretz assigned gkalpak Nov 1, 2016

gkalpak suggested changes Nov 1, 2016

View reviewed changes

Narretz changed the title ~~perf(*): don't trigger additional digests on enter/leave of structura… …l directives~~ perf(*): don't trigger digests after enter/leave of structural directives Nov 3, 2016

Narretz force-pushed the fix-animate-done branch from 220df23 to ad104a6 Compare November 3, 2016 10:07

gkalpak reviewed Nov 4, 2016

View reviewed changes

Narretz force-pushed the fix-animate-done branch from 2e29fc3 to 696b7a4 Compare November 11, 2016 13:30

gkalpak reviewed Nov 14, 2016

View reviewed changes

gkalpak approved these changes Nov 21, 2016

View reviewed changes

style(ngIfSpec.js, ngIncludeSpec.js): add top level describe

600b3c6

Narretz assigned Narretz and unassigned gkalpak Nov 22, 2016

Narretz force-pushed the fix-animate-done branch from dc57ef2 to 03ce2e7 Compare November 22, 2016 16:26

Narretz merged commit f4fb6e0 into angular:master Nov 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(*): don't trigger digests after enter/leave of structural directives #15345

perf(*): don't trigger digests after enter/leave of structural directives #15345

Narretz commented Nov 1, 2016

dcherman commented Nov 1, 2016

gkalpak left a comment

gkalpak Nov 1, 2016

Narretz Nov 2, 2016

gkalpak Nov 1, 2016

Narretz commented Nov 1, 2016

Narretz commented Nov 4, 2016

gkalpak left a comment •

edited

Loading

gkalpak Nov 4, 2016

Narretz Nov 5, 2016 •

edited

Loading

gkalpak Nov 5, 2016

Narretz Nov 5, 2016 •

edited

Loading

gkalpak Nov 5, 2016

gkalpak Nov 8, 2016

Narretz Nov 8, 2016

gkalpak Nov 9, 2016 •

edited

Loading

Narretz Nov 9, 2016 •

edited

Loading

gkalpak Nov 9, 2016

gkalpak left a comment

gkalpak Nov 14, 2016

gkalpak Nov 14, 2016

gkalpak Nov 14, 2016

Narretz commented Nov 14, 2016

petebacondarwin commented Nov 21, 2016

gkalpak commented Nov 21, 2016

petebacondarwin commented Nov 21, 2016



		it('should handle changes to the switch value in a digest loop with multiple value matches',
		inject(function($$rAF, $compile, $rootScope, $timeout) {

perf(*): don't trigger digests after enter/leave of structural directives #15345

perf(*): don't trigger digests after enter/leave of structural directives #15345

Conversation

Narretz commented Nov 1, 2016

dcherman commented Nov 1, 2016

gkalpak left a 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

Narretz commented Nov 1, 2016

Narretz commented Nov 4, 2016

gkalpak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz Nov 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz Nov 5, 2016 • edited Loading

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 Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Narretz Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a 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

Narretz commented Nov 14, 2016

petebacondarwin commented Nov 21, 2016

gkalpak commented Nov 21, 2016

petebacondarwin commented Nov 21, 2016

gkalpak left a comment •

edited

Loading

Narretz Nov 5, 2016 •

edited

Loading

Narretz Nov 5, 2016 •

edited

Loading

gkalpak Nov 9, 2016 •

edited

Loading

Narretz Nov 9, 2016 •

edited

Loading