ngAnimate 1.2.0-rc2 has performance impact

[damrbaby]

Just by including the ngAnimate module in my project (using latest code on master), I am seeing noticeable performance degradation that is especially visible on devices with slower processors like the iPod Touch 4g.

In my case an ngRepeat with around 50-100 items and no CSS transitions / animations takes what could be 2-3x longer to render on a mobile device with ngAnimate. I noticed that the performAnimation function is being called 50-100 times per digest (the number goes up or down depending on the number of items in the ngRepeat).

The animation code is being executed on all elements of directives that support animation whether or not there are actually CSS transitions / animations that apply to those elements, and it has a performance impact. For most animation-enabled directives it might not be a big deal but it is especially noticeable during the rendering and re-rending of items in an ngRepeat.

[mgcrea]

That's scary, we've also witnessed a few strange css regressions on some of our mobile applications, css transitions (fadeIn/fadeOut) with big pictures (retina) often get glitches on the latest rc2 but were working fine with v1.1.5, bug only happen on device (iPad), not as critical as this ngRepeat issue.

I quite liked the clean separation of animations and classes used in v1.1.5, it wasn't perfect, but imo it was less confusing & error/bug-prone that the way chosen for the v1.2.0. Moreover, I'm still wondering why you chose to release a release-candidate with such major breaking changes regarding animations, why not drop another 1.1.x release before the RC? I don't get the point of having an unstable branch to drop an such a clearly different (and somewhat broken) RC on the stable branch, it mislead people to think that animations were production-ready.

Anyway, if we have to add performance regression to the list, it's a little bit hard to digest. I think that mobile apps are the last territory to conquer for AngularJS, and performance will be a crucial stake. I'm having big hopes for AngularJS on mobile (and I pretty sure many developers feel the same), but theses animation issues are holding us back, it should really be the top 1 priority now.

[cburgdorf]

I'm also worried about performance of the new ngAnimate rewrite but I don't expect them to let this performance regressions slip into the final 1.2 release. I'm sure @matsko is on it and it will be resolved soon.

[JasonGoemaat]

It does seem rather crazy to do an animation cycle on elements when no animation is desired... ng-animate was a great way to control it..

[olostan]

Animations are one of the main features in upcoming 1.2 release. But as it used a lot on mobile devices, such performance drawback is critical...

Although I really liked the new style of doing animations via only css without modification of html/js...

[ajoslin]

One idea would be to have a 'strict' mode, where it only animates an element if ng-animate is present on the element.

myApp.config(function($animateProvider) {
  $animateProvider.onlyAnimateWithAttribute(true);
});

<div class="fade" ng-show="isShown" ng-animate>I will animate</div>
<div class="fade" ng-show="isShown">I won't animate</div>

[damrbaby]

@ajoslin I like your idea, and almost think that should be the default.

[ajoslin]

Yeah, that does seem to make more sense.

[mgcrea]

I'd second @ajoslin idea of a strict mode, would also default to it.

But if we have to add the attribute back, it may be worth considering reverting to use ngAnimate as an ngClass-like directive since it looks like the ngClass animation support has added quite some complexity & overhead to the directive compilation cycle (ie. 36ad40b).

I'd also love to have some benchmarking tests (jsperf like) giving ranks to some basic use-cases, like render an ngRepeat with 1K elements, not sure if this could be easily achievable with jasmine/karma, but it would be great to have this kind of performance testing in the core.

[ajoslin]

I think it's being discussed; a benchmark could be done using e2e tests.

On Thursday, September 19, 2013, Olivier Louvignes wrote:

I'd second @ajoslin https://github.com/ajoslin idea of a strict mode.

I'd also love to have some benchmarking tests (jsperf like) giving ranks
to some basic use-cases, like render an ngRepeat with 1K elements, not
sure if this could be easily achievable with jasmine/karma, but it would be
great to have this kind of performance testing in the core.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4011#issuecomment-24739551
.

...sent from my iPhone

[runxc1]

I have been developing a fairly heavy angularjs app with multiple instances of ngRepeat and a few different animations and I didn't notice any degradation of performance when upgrading from 1.1.5 to 1.2.0-rc1 but after updating to 1.2.0-rc2 I have noticed a significant performance hit and am probably going to need to roll back to 1.2.0-rc1. I have been able to get the app "performant enough" on desktop but it really starts to break down on an iPhone 4S where as it was working well with rc1.

[bfricka]

I really like this @ajoslin. ngAnimate on rc2 is no longer usable on mobile in the case of repeaters and all but the simplest cases. Additionally, there is still significant overhead generated just by including the module, even when setting $animate.enabled(false); due to all the timers still being registered and firing.

[joegaudet]

We also experience this regression, on tables of any considerable size. Seems like making animation inclusion declarative (as suggested above) is consistent with the angular ideals.

.joe

[matsko]

Hey guys. The solution isn't to tweak the template or to temporarily disable animations surrounding repeated lists, but to just improve the efficiency of $animate. The goal is that ngAnimate will be internally optimized to avoid calling performAnimation multiple times per digest and, as long as the className value is the same for the element as it was before and for each item in a list when compared to other items, then caching can boost the performance. This is the plan. Does anyone have a good benchmark I can look at to compare the caching code to?

[joegaudet]

The issue arrises for me when a repeater exists with a large number of rows, and then inside of the repeater there are visibility (hide / show) bits which all animate by default now. This all stacks up to make things really slow.

[olostan]

Did anybody achieved any results? Would appreciate any even temporary workaround (except downgrading version)

[joegaudet]

I just put a guard clause on perform animation for now - on my own fork. Works reasonably well.

[Narretz]

Any eta on this? Prevents me currently from testing the (bitter-)sweet new animations of 1.2, and all the other improvements

[matsko]

I've got a work in progress. Can anyone here use the JS files found here and test against their own code to see if things run any faster.

https://s3.amazonaws.com/angularjs-dev/faster-ng-repeat/angular.js
https://s3.amazonaws.com/angularjs-dev/faster-ng-repeat/angular-animate.js
https://s3.amazonaws.com/angularjs-dev/faster-ng-repeat/angular-route.js

[bfricka]

I'd be happy to do some testing with this. I likely won't be able to make
time until this evening, but I will report back.
On Oct 1, 2013 7:32 AM, "Matias Niemelä" notifications@github.com wrote:

I've got a work in progress. Can anyone here use the JS files found here:

https://s3.amazonaws.com/angularjs-dev/faster-ng-repeat/angular.js
https://s3.amazonaws.com/angularjs-dev/faster-ng-repeat/angular-animate.js
https://s3.amazonaws.com/angularjs-dev/faster-ng-repeat/angular-route.js

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4011#issuecomment-25454309
.

[damrbaby]

@matsko It does not appear to be running any faster for me when rendering a new ng-repeat with 50+ items on a 4th generation iPod Touch (still takes what could be 4x longer than without ngAnimate). It doesn't look like the cache will help for a new ng-repeat, and brief debugging makes me believe the bulk of extra time is coming from the code in this forEach loop, which gets executed for each item in the ngRepeat list whether or not animations are present: https://github.com/matsko/angular.js/blob/1022d56669a8d52bce8e4dd66a9389e7b93a176b/src/ngAnimate/animate.js#L471-L475

[matsko]

@damrbaby that code is actually cached for the repeater, but I don't think that it causes any slowdown since the injection fetch is constant. Unless the try/catch brace around it slows things down. We'll find out after testing these files.

So I believe the bottleneck is coming mostly from the forced repaints that occur per each animation. A repaint (or reflow) occurs when there is a delay (timeout) during the JavaScript execution. Basically when the JS code gets a break from crunching synchronous code. You can also trigger a repaint using other tricks ... What ngAnimate does is it calls element.prop('clientWidth') each time enter/leave/move/addClass/removeClass is called to trigger the active animation and the reflow. This I believe really clogs the animation execution.

Here are the JS files which queue all the reflows into one operation, please let me know if the animation is any more performant. Instead of having N repaints you only have 1 repaint after N items have been processed.

https://s3.amazonaws.com/angularjs-dev/ng-animate-queue/angular.js
https://s3.amazonaws.com/angularjs-dev/ng-animate-queue/angular-animate.js
https://s3.amazonaws.com/angularjs-dev/ng-animate-queue/angular-route.js

[matsko]

@mzgol here's a commit for you:
matsko@5e3fa50

[damrbaby]

@matsko same issue, nothing has changed

[bfricka]

Really? No change? What metrics are you using @damrbaby ?

@matsko I have been getting my ass kicked at work but tomorrow I WILL test these changes against our main app and report back w/ some real numbers. Thanks for your time.

[mgol]

@matsko still lots of style recalculation.

[the-machinist]

OMG, I'm so sorry. Thanks @matsko - It works after a style is added to .ng-leave-active! :D

[matsko]

No worries. I'll remember to document this in a noticeable way.

[mgol]

@matsko No excessive reflows after applying your patch. :) (I mean: no reflows caused by ngAnimate, it seems angular-sanitize now causes some).

[matsko]

I thought this day would never come :) Alright it's time for a PR and some merging.

[mgcrea]

Congrats @matsko for the amazing job fixing this, I'm looking forward to dig down into animations again!

[ajoslin]

Thanks matsko! This is great :-)

Sent from my iPhone

On Oct 10, 2013, at 8:17 AM, Olivier Louvignes notifications@github.com wrote:

Congrats @matsko for the amazing job fixing this, I'm looking forward to dig down into animations again!

—
Reply to this email directly or view it on GitHub.

[mgol]

Yup, great job, @matsko! \o/

[deakster]

I don't want to ruin the celebrations, but after swapping the latest files over from @matsko , it seems about half of the animations in my project no longer work (compared to RC2). I have been using ngAnimate pretty heavily throughout this project.

I will try to dig into this a bit more and recreate on a plunkr, but upon first inspection, it seems the animations that are not working are ones that are on elements that are within an ngView or ngInclude. In many cases, I have an ngRepeat that is inside an ngInclude which is inside of an ngView, and these certainly don't seem to be firing.

[matsko]

@deakster are these animations triggered during the change of the ngInclude / ngView? Because that's expected. Please provide a plunkr as this should be closed by tomorrow.

[deakster]

It is not just during change of ngInclude/ngView. The exact setup is ngView->ngInclude->ngRepeat, and the ngRepeat animations aren't firing even once things are added/remove after view/include page change. As a quick test, I copy and pasted the ngInclude contents into the view, so it is now just ngView->ngRepeat, and it still doesn't work.

Both the ngView and ngInclude are loading pages that have been preprocessed into $templateCache could it be anything to do with that? As I mentioned, all of this works in RC2.

In any case, I will try to extract this out into a plunkr ASAP.

[deakster]

Ok, I figured out the cause. The animations only seem to work now if you have a CSS transition property specifically targetting the ng-enter and ng-leave classes.

So you basically need a .item.ng-enter,.item.ng-leave { transition... }

Whereas I had the transition defined at the item's base selector as .item { transition... }, since I am re-using those transition parameters outside of ng-enter and ng-leave.

It's not a problem for me though, I can easily add in more specific transition parameters into each of the sub-classes.

Great job on the performance improvements!

[matsko]

Yes this is the expected behaviour and it is strictly enforced with this patch. Great job finding out what was going on.

And... back to rebasing for me :)

[deakster]

Yes, I think it is fair enough to enforce this, but it is a change from how RC2 worked, so just noting it here in case others come across the same issue.

[joegaudet]

How soon till this is available through normal channels ?

Our build all relies on bower.

.jo

On Thu, Oct 10, 2013 at 8:15 AM, deakster notifications@github.com wrote:

Yes, I think it is fair enough to enforce this, but it is a change from
how RC2 worked, so just noting it here in case others come across the same
issue.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4011#issuecomment-26062276
.

.joe out.

+1.778.994.4846

[joegaudet]

Oh yes and a big thanks !

On Thu, Oct 10, 2013 at 9:04 AM, Joe Gaudet joe@joegaudet.com wrote:

How soon till this is available through normal channels ?

Our build all relies on bower.

.jo

On Thu, Oct 10, 2013 at 8:15 AM, deakster notifications@github.comwrote:

Yes, I think it is fair enough to enforce this, but it is a change from
how RC2 worked, so just noting it here in case others come across the same
issue.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/4011#issuecomment-26062276
.

.joe out.

+1.778.994.4846

.joe out.

+1.778.994.4846

[matsko]

Should be in master tomorrow.

[Narretz]

master is not in bower, though. This'll land in 1.2 RC3, I think.

[mbelshe]

@matsko - Just wanted to chime in and let you know that your test files definitely fixed my ngAnimate performance problem as well. Look forward to it landing! Thanks!

[matsko]

Happily landed as ddcbf76

[guillaume86]

@matsko could you specify what is exactly expected in css/dom to trigger animations with the last changes?

I have trouble restoring animations in this case (angular animate the entering/leaving, masonry the positionning):
1.1.5: http://jsfiddle.net/g/3SH7a/
1.2.0-rc3: http://jsfiddle.net/g/3SH7a/45/

[matsko]

RC3 introduces a restriction on placing the transition on the base class. However, with RC4 (which is 1.2.0) this restriction is gone. So wait a few days until that's out to test your code against that.