perf(ngClass): optimize the case of static map of classes of large objects

[drpicox]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...): performance optimization of the ngClass

What is the current behavior? (You can also link to an open issue here): when there is an ngClass with an object, it makes a deep copy of all its values, although it only needs to check for truly or falsy. It is because it uses $watch(,, true).

What is the new behavior (if this is a feature change)?: it tries to statically detect the case of ng-class="{foo: bar, ...}" case and then do a $watchCollection(,) instead of a $watch(,, true). Its a mere and simple test against initial attribute value.

Does this PR introduce a breaking change?: nop

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)
• [N/A] Docs have been added / updated (for bug fixes / features)

Other information:

Time to time, working with my customers, they ask to me to make performance tasks: they give me access to angular projects and they ask me to optimize them. One of the mistakes that I found many times is to do the following:

<div ng-class="{friendly: user.friends}">
  ...
</div>

because user.friends is truly (it has a value) or falsy (it is undefined or null) it works, but some times with truly values, associated data can be big taking up to seconds perform some copy (imagine that friends are not ids but objects with more relationships, ...), in such cases, always give white papers asking for things like:

<div ng-class="{friendly: !!user.friends}">
  ...
</div>

which solves all performance problems, or if it is only one class do something like:

<div ng-class="user.friends && 'friendly'">
  ...
</div>

which gives a string easy to copy and compare (but not so recommendable because not all designers or programmers understand it easily and difficult to express more than one class)

The current solution that I give is simple, fast to compute, and it works in 99% of the cases. I can try to do a custom changeDetector (like the one in $watchCollection) but I believe that it is not worth (I didn't know that arrays could be used to add classes). So it will work well in the initial example:

<div ng-class="{friendly: user.friends}">
  <!-- new: starts with { so it becomes optimized through $watchCollection -->
  ...
</div>

but:

<div ng-init="classes = {friendly: user.friends}"></div>
<div ng-class="classes">
  <!-- new: does not starts with { so it is not optimized, not very common -->
  ...
</div>

[gkalpak]

Why aren't we always using $watchCollection anyway ? There doesn't seem to be any need for $watch(...true).

This was probably implemented before $watchCollection was available and nobody thought to change it. (Can't be sure though.)

[drpicox]

Is the first thing that I tried, just in case that it worked well with strings too, but then is when I discovered (some test failed) that the follow array form is now valid:

<div ng-class="['some', 'thing', {else: else}]">...</div>

Chrome 49.0.2623 (Mac OS X 10.11.4) ngClass should support adding multiple classes via an array mixed with conditionally via a map FAILED
    Error: [$rootScope:infdig] 10 $digest() iterations reached. Aborting!
    Watchers fired in the last 5 iterations: [[{"msg":"fn: regularInterceptedExpression","newVal":10,"oldVal":9}],[{"msg":"fn: regularInterceptedExpression","newVal":11,"oldVal":10}],[{"msg":"fn: regularInterceptedExpression","newVal":12,"oldVal":11}],[{"msg":"fn: regularInterceptedExpression","newVal":13,"oldVal":12}],[{"msg":"fn: regularInterceptedExpression","newVal":14,"oldVal":13}]]

And in this case $watchCollection is not enough. That is the reason why I thought it was not enough.

There is another option: make the watching function to convert always the input to a string, it should be simple, fast (at least as fast as now), it might simplify some cases, but it might require a quite refactor.

I'm not sure if it is necessary, because all cases that I have found of large copy structures were always written with ng-class="{...}" form. But, if you consider so, I can try the rewrite.

[drpicox]

Uhmmm may be is not so complicated, I think that there is a short cut, I'll try.

[gkalpak]

I forgot we support the object-inside-array syntax. Maybe we should special-case the [...] syntax then.

[drpicox]

I'm afraid that's not possible, because you can do:

<div ng-init="foo = ['some', 'thing', {else: else}]"></div>
<div ng-class="foo">
  ..
</div>

And in such case it is not compliant the use of $watchCollection.

Anyway, I have an alternative solution, I'm just cleaning up it a little bit, and I'll push in a separated commit, so you can choose which one you prefer. (I think that the second is better, the first is the one that I had in my head for a long time).

[drpicox]

Done.

Now it compares always strings and the watch computes classes.
Because to build the string an object it must do the same checkings and traversing than $watch (equals+copy) , or less in the case of nested objects, the overall performance should be always better.
The code also seems to be slightly simplier, but it changes many things.

I think that the second one is better, If you want I can:

• rebase into one single commit and remove the previous solution, or
• drop the second solution commit and keep the first
• make two PR, one with each proposal :-)

[gkalpak]

In #14394 (comment) I meant use $watchCollection everywhere except for arrays. But I don't know if it would be faster than your 2nd commit anyway.

Could you put your second commit on a separate PR, remove the unused staticMapClassRegEx (probably a left-over from the first commit) and make sure that your approach works with one-time bindings ?
(I'm not sure if there is a test for that already. If there isn't please add a test that uses e.g. ng-class="::{foo: bar, baz: qux}".)

[drpicox]

About #14394 (comment) the problem is that the detection of which kind of watch is applied is being doing at compile time, but knowing that there is an array, a string, or an object is something that has to be resolved un runtime. The binded value can be a variable containing either an array or an object.
I did a plnkr here http://plnkr.co/edit/f0V9RVDGREetSW00TTRZ?p=preview to illustrate what I mean. Anyway I think that there is a bug.

About second PR, great! I had this in mind, better two branches so it is easy to compare performances. And thanks for noticing that I left an unused constant.
(but I have to dig out in the may be bug that I've seen in the previous plnkr).

About "::{" expressions, I did the check the first time and the RegEx, but it turns out that a the value never gets copied because as soon as it has a value the watcher is removed. Anyway I add the test case.

[drpicox]

Uhmmm I have put a console.log inside copy and it copies verylargeobject just once :/ but it is not stored into last. I'm not sure how to test that it does not copy verylargeobject, ¿any idea?

[gkalpak]

Off the top of my head, I would try putting a get accessor property on verylargeobject and verify how many times it's been accessed. Something like:

var verylargeobject = {};
Object.defineProperty(verylargeobject, 'prop', {
  get: jasmine.createSpy('getProp')
});

[drpicox]

yep! thanks! I'll try it.

[drpicox]

Part 1 done.

[drpicox]

Part 2 done: the alternative is here: #14404

[drpicox]

Part 3 done: the bug that I spotted in the plnkr http://plnkr.co/edit/f0V9RVDGREetSW00TTRZ?p=preview is tested and solved here: #14405 (it is applied here, but #14404 also solves it in the refactor)

One question about this undocumented behaviour:

  it('should support mixed array/object variable with a mutating object', inject(function($rootScope, $compile) {
    $rootScope.classVar = ['red', ['green', {blue: true}]];
    element = $compile('<div class="existing" ng-class="classVar"></div>')($rootScope);
    $rootScope.$digest();

    expect(element.hasClass('red')).toBeTruthy();
    expect(element.hasClass('green')).toBeTruthy();
    expect(element.hasClass('blue')).toBeTruthy();
  }));

because it it works in 1.5.3 buggy (even with the fix) the following test fails:

  it('should support mixed nested array and object leafs variable with mutating object', inject(function($rootScope, $compile) {
    $rootScope.classVar = ['red', ['green', {blue: true}]];
    element = $compile('<div class="existing" ng-class="classVar"></div>')($rootScope);
    $rootScope.$digest();

    $rootScope.classVar[1][1].blue = false;

    expect(element.hasClass('red')).toBeTruthy();
    expect(element.hasClass('green')).toBeTruthy();
    expect(element.hasClass('blue')).toBeFalsy();
  }));

So, ¿Is it a bug to

• accept multiple nested arrays (in opposite to what Angular2 with injections/directives do)? or,
• do not track changes of mutating objects inside nested arrays?
  (I can fix either)

In #14404 works 100% well with multiple nested arrays.
Note: avoid nesting arrays might break some existing code, but it is unlikely because it is buggy (1.5.3 does not track changes of nested arrays neither objects).

[gkalpak]

Closing this in favor of #14404.