perf(ngForm,ngModel): move initial addClass to the compile phase

[jbedard]

Other then moving from link to compile time the only difference (which unit tests didn't seem to care about) is that $animate.addClass(VALID_CLASS) and $animate.removeClass(INVALID_CLASS) are no longer called at link time since VALID_CLASS is added at compile and INVALID_CLASS shouldn't exist. If anyone depends on the initial animation calls or on the .removeClass(INVALID_CLASS) then this change won't work, at least in its current form...

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8268)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

what exactly is the performance benefit of this? this is unclear to me

[jbedard]

If a form or model is linked more then once it only adds the classes once at compile time (before cloning the element). Or am I missing something that makes this case different then other directives?

My use case is ng-model within ng-repeat, such as a list of items with a checkbox each...

[caitp]

right but, is something happening that you're not expecting to happen? adding/removing classes isn't a particularly expensive operation usually, so I'm wondering if you're really noticing this

[caitp]

oops, didn't mean to close that

[jbedard]

I think adding/removing classes is particularly expensive since it touches the DOM, and especially when it modifies the DOM. If it is done at compile time then it is done once and cloned where at link time it is done N times. This is basically what @IgorMinar has been doing to many directives, however this removes 3 calls to add/removeClass per link instead of just 1, but with the potential side effect of not going through $animate at link time (for 2/3 of the calls).

In the plunker below the NgModelController is about 25% of the bootstrapping, and half that is calling add/remove class 3 times...

http://plnkr.co/edit/kJWdsdy1NXjifSJOh21N?p=preview

[caitp]

I think adding/removing classes is particularly expensive since it touches the DOM

Classes aren't being added/removed all that frequently, so this is much less of a hit than it would be otherwise. Even in a list of 2000 form controls, it's pretty negligible.

Even when it comes to bootstrap time, I'm not sure that this is a real bottleneck in any real application --- I may be wrong, but I'd need to see proof to come to that conclusion. I think if it is a significant bottleneck, the application design probably needs to be re-worked, because it really shouldn't be.

Anyways, I'm pretty sure this is not right, but see what matias or igor think about it, really.

[jbedard]

Does the plunker not show add/removeClass being 10%+ of the link time? I thought that was pretty significant for something that can easily be avoided...

Do you have an example of how an app such as the plunker example can be re-worked to avoid this? Or do you mean 2000 checkboxes is just bad UI? (Which I'd agree with, but that much data is rare enough in my case so it wouldn't be worth changing the UI.)

[jbedard]

Note sure where your last comment went with the chrome screenshot, but I keep getting 10%+ of time being spent in add/removeClass and it seems fairly consistent across browsers (chrome/ff/ie) when profiling...

(Note that the 15% in NgModelController is 15/57% spent within angular)

[IgorMinar]

name anonymous fns

[IgorMinar]

@caitp I think that for ngModel used in a repeater this change could be significant.

I'll review this and get it in as part of rc2

[jbedard]

I've added the compile/link function names.

[IgorMinar]

do addClass(PRISTINE_CLASS).addClass(VALID_CLASS) in order to avoid extra string concat and splitting

[jbedard]

Performance wise I assume the extra string concat here is actually better because each addClass will do split+forEach+regex+get/setAttribute. But this is compile time so I don't think it matters much, and calling addClass twice looks more readable so I'll go ahead with that for now...

[jbedard]

@IgorMinar I've made the suggested changes to the addClass lines.

[IgorMinar]

can you rebase this on top of master please? it looks good otherwise.

[jbedard]

Rebased.

Note that in the rebase I added a .hasClass in the new addSetValidityMethod to ensure the classCache is correctly initialized. This could probably be changed to just true but I thought I'd do .hasClass for now just in case something else between compile and addSetValidityMethod changes the state. I found this .hasClass was about ~1% of the link time, where the replaced .addClasss were ~5-6%.

[IgorMinar]

merged. thanks