perf(ngClass): refactor to optimize the case of static map of classes with large objects #14404

drpicox · 2016-04-09T22:06:39Z

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

What is the current behavior? (You can also link to an open issue here): it makes copies of very large objects:

    it('should not copy verylargeobject', inject(function($rootScope, $compile) {
      var getProp = jasmine.createSpy('getProp');
      var verylargeobject = {};
      Object.defineProperty(verylargeobject, 'prop', {
        get: getProp,
        enumerable: true
      });
      element = $compile('<div ng-class="{foo: verylargeobject}"></div>')($rootScope);
      $rootScope.verylargeobject = verylargeobject;
      $rootScope.$digest();

      expect(getProp).not.toHaveBeenCalled();
    }));

What is the new behavior (if this is a feature change)?: the user perceives no change but that it is faster with very large objects

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)

Other information:
This PR responds to #14394 (comment)
It is alternative to #14394

gkalpak · 2016-04-10T12:41:38Z

test/ng/directive/ngClassSpec.js

@@ -409,6 +409,78 @@ describe('ngClass', function() {
    expect(e2.hasClass('even')).toBeTruthy();
    expect(e2.hasClass('odd')).toBeFalsy();
  }));
+
+  it('should do value stabilization as expected when one-time binding', inject(function($rootScope, $compile) {


Please, wrap lines at 100 chars.

drpicox · 2016-04-10T16:59:56Z

For one time binding is getting hard, I just cannot previously convert to a plain string because of:

  it('stabilizes when in a literal are not undefineds', inject(function($rootScope, $compile) {
    $rootScope.foo = true;
    element = $compile('<div ng-class="::{foo:foo,bar:bar}"></div>')($rootScope);
    $rootScope.$digest();
    expect(element).toHaveClass('foo');
    $rootScope.bar = true;
    $rootScope.$digest();
    expect(element).toHaveClass('bar');
    $rootScope.bar = false;
    $rootScope.$digest();
    expect(element).toHaveClass('bar');
  }));

In the current implementation the watcher watches a string which cannot contain undefineds. I will change the strategy. Fortunately it is only one level:

  // this test fails (nested undefineds)
  it('stabilises when nested?', inject(function($rootScope, $compile) {
    $rootScope.foo = true;
    element = $compile('<div ng-class="::[{foo:foo,bar:bar}]"></div>')($rootScope);
    $rootScope.$digest();
    expect(element).toHaveClass('foo');
    $rootScope.bar = true;
    $rootScope.$digest();
    expect(element).toHaveClass('bar');
    $rootScope.bar = false;
    $rootScope.$digest();
    expect(element).toHaveClass('bar');
  }));

from parse.js (see that isAllDefined is one level, not nested):

    function oneTimeLiteralWatchDelegate(scope, listener, objectEquality, parsedExpression) {
      var unwatch, lastValue;
      return unwatch = scope.$watch(function oneTimeWatch(scope) {
        return parsedExpression(scope);
      }, function oneTimeListener(value, old, scope) {
        lastValue = value;
        if (isFunction(listener)) {
          listener.call(this, value, old, scope);
        }
        if (isAllDefined(value)) {
          scope.$$postDigest(function() {
            if (isAllDefined(lastValue)) unwatch();
          });
        }
      }, objectEquality);

      function isAllDefined(value) {
        var allDefined = true;
        forEach(value, function(val) {
          if (!isDefined(val)) allDefined = false;
        });
        return allDefined;
      }
    }

drpicox · 2016-07-21T12:52:27Z

@gkalpak ¿can you take a look to:

https://travis-ci.org/angular/angular.js/pull_requests ?

it seems that the following test fails randomly in Firefox:

[firefox 28 #2] ..F...............................................................................................................................................................
[firefox 28 #2] 
[firefox 28 #2] Failures:
[firefox 28 #2] 1) $anchorScroll with `yOffset` should automatically scroll when navigating to a URL with a hash
[firefox 28 #2]   Message:
[firefox 28 #2]     Expected #anchor-5's top to be 50, but it was 208
[firefox 28 #2]   Stack:
[firefox 28 #2]     Error: Failed expectation
[firefox 28 #2]         at /home/travis/build/angular/angular.js/test/e2e/tests/anchor-scroll.spec.js:113:32
[firefox 28 #2]         at /home/travis/build/angular/angular.js/test/e2e/tests/anchor-scroll.spec.js:160:7

gkalpak · 2016-07-21T15:03:18Z

Yeah, I've seen it too. I thought these were flakes. I mean, they are flakes - but they seem to be more common than other flakes. These tests are "flake-prone".

Maybe #14912 will make them go away.

drpicox · 2016-07-21T15:36:29Z

Indeed. Firefox 28 is quite old :D

gkalpak · 2016-07-21T17:09:57Z

It's green now.

Narretz · 2016-09-08T19:33:31Z

Did this LGTY back in July @gkalpak ?

Narretz · 2016-09-08T19:34:17Z

@drpicox can you please rebase this?

gkalpak · 2016-09-08T20:19:30Z

Did this LGTY back in July @gkalpak ?

Not if I didn't say so 😃
(I got caught up in other stuff and I never gave this a final review. I'll take a look asap.)

drpicox · 2016-09-09T09:59:02Z

Ops!

I'll do it now.

drpicox · 2016-09-09T10:23:10Z

Rebased. Waiting for travis tests.

drpicox · 2016-09-09T10:56:37Z

Green.

gkalpak

There a couple of issues with this implementation.

TBH, I feel like there should be a more straight-forward way to achieve the same. I'll poke around.

gkalpak · 2016-10-06T07:12:58Z

src/ng/directive/ngClass.js

-        attr.$observe('class', function(value) {
-          ngClassWatchAction(scope.$eval(attr[name]));
+      compile: function(tElement, tAttr) {
+        var classFn = $parse(tAttr[name], function(value) {


Having this in compile is an (unnecessary) breaking chage: Previously other directives would be able to modify the value before ngClass' linking. (Unlikely that anyone is actually relying on it, but we don't gain anything by breaking it either.)

I did it because of performance reasons: ng-class are often used inside ngRepeat in which compile is executed once but link many. It can be undone.

The overhead should be negligible (and not enough to justify the breaking change).

_{(Note that $parse has a cache, so it doesn't have to actually lex/parse the same expressiom twice. It only has to retrieve the parsed expression and apply the interceptor.)}

I'll put it again inside a link function.
I suggest that it can be considered a "major upgrade" to do such refactor of all parse expression code from link functions to compile functions, in order to improve performance when ng-repeat & others are involved.

gkalpak · 2016-10-06T07:14:09Z

src/ng/directive/ngClass.js

-            /* eslint-enable */
-          });
-        }
+          scope.$watch(classFn, ngClassWatchAction, classArrayEquals);


The 3rd argument is expected to be boolean. Passing a function is interpreted as true.

sure? It dit it some time ago, so I'm not sure, but I remember look for this code inside angular and the third argument become the comparator if was not true (some kind of undocumented function). I'll check.

I think there is a PR for adding that feature, but right now it is treated a boolean.

I'll check, I was wrong. It takes classArrayEquals, using true, all tests passes as expected, no extra copies performed (I have also check the $watch code).
So it is updated, Thanks.

gkalpak · 2016-10-06T07:45:52Z

src/ng/directive/ngClass.js

+              } else if (!classArrayEquals(oldClasses, newClasses)) {
+                updateClasses(oldClasses, newClasses);
+              }
+              oldClasses = shallowCopy(newClasses);


By not storing the oldClasses when (scope.$index & 1) !== selector, you could end up with an outdated oldClasses value when the $index changes. There is a similar bug currently in the scope.$index watch above.

I'll try to write a test to make it fail and fix it.

Trying to find a test to make it break, I found a case that breaks angular in older versions (at least since 1.3.8 -I didn't tried more-). Is the case of the first scope.$watch that you comment.
I have the plnkr here: https://plnkr.co/edit/W1ck8dS08TCJpirWiGVA?p=preview
I solve it here, but I do not know if I should create another PR for solving it outside this PR, what did you suggest?

I tried to find a failing test for the shallow copy inside the condition so I should move it. But I have found that moving the shallow copy outside the condition interferes with the $index $watch and make the new test fails.

gkalpak · 2016-10-06T07:52:42Z

test/ng/directive/ngClassSpec.js

+    })
+  );
+
+  it('should do value remove the watcher when static array one-time binding',


should do value remove --> should remove

gkalpak · 2016-10-06T07:53:33Z

test/ng/directive/ngClassSpec.js

+    })
+  );
+
+  it('should do value remove the watcher when static map one-time binding',


should do value remove --> should remove

gkalpak · 2016-10-06T07:54:32Z

test/ng/directive/ngClassSpec.js

+      $rootScope.classVar[0].orange = false;
+      $rootScope.$digest();
+
+      expect(element).not.toHaveClass('orange');


Check that the class was present before changing orange to false.

…ide arrays Mostly taken from angular#14404.

@drpicox

The cases that should benefit most are: 1. When using large objects as keys (e.g.: `{loaded: $ctrl.data}`). 2. When using objects/arrays and there are frequent changes. 3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s). The differences in operations per digest include: 1. `Regular expression (when not changed)` **Before:** `equals()` **After:** `toClassString()` 2. `Regular expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** 2 x `split()` 3. `One-time expression (when not changed)` **Before:** `equals()` **After:** `toFlatValue()` + `equals()`* 4. `One-time expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** `copy()`* + `toClassString()`* + 2 x `split()` 5. `$index modulo changed` **Before:** `arrayClasses()` **After:** - (*): on flatter structure In large based on angular#14404. Kudos to @drpicox for the initial idea and a big part of the implementation. Closes angular#14404

gkalpak · 2016-10-08T07:47:35Z

I have an alternative PR (#15228) which includes several refactorings, the fix mentioned above and some perf improvements (that should cover the ones in this PR and more).

I have "borrowed" the tests and part of the implementation 😁

WDYT?

@drpicox

The cases that should benefit most are: 1. When using large objects as values (e.g.: `{loaded: $ctrl.data}`). 2. When using objects/arrays and there are frequent changes. 3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s). The differences in operations per digest include: 1. `Regular expression (when not changed)` **Before:** `equals()` **After:** `toClassString()` 2. `Regular expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** 2 x `split()` 3. `One-time expression (when not changed)` **Before:** `equals()` **After:** `toFlatValue()` + `equals()`* 4. `One-time expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** `copy()`* + `toClassString()`* + 2 x `split()` 5. `$index modulo changed` **Before:** `arrayClasses()` **After:** - (*): on flatter structure In large based on angular#14404. Kudos to @drpicox for the initial idea and a big part of the implementation. Closes angular#14404

drpicox · 2016-10-08T15:29:36Z

You have done a really good job 👍

…objects by refactor

… bug

@drpicox

The cases that should benefit most are: 1. When using large objects as values (e.g.: `{loaded: $ctrl.data}`). 2. When using objects/arrays and there are frequent changes. 3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s). The differences in operations per digest include: 1. `Regular expression (when not changed)` **Before:** `equals()` **After:** `toClassString()` 2. `Regular expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** 2 x `split()` 3. `One-time expression (when not changed)` **Before:** `equals()` **After:** `toFlatValue()` + `equals()`* 4. `One-time expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** `copy()`* + `toClassString()`* + 2 x `split()` 5. `$index modulo changed` **Before:** `arrayClasses()` **After:** - (*): on flatter structure In large based on angular#14404. Kudos to @drpicox for the initial idea and a big part of the implementation. Closes angular#14404

@drpicox

…d copies Includes the following commits (see #15246 for details): - **perf(ngClass): only access the element's `data` once** - **refactor(ngClass): simplify conditions** - **refactor(ngClass): move helper functions outside the closure** - **refactor(ngClass): exit `arrayDifference()` early if an input is empty** - **perf(ngClass): avoid deep-watching (if possible) and unnecessary copies** The cases that should benefit most are: 1. When using large objects as values (e.g.: `{loaded: $ctrl.data}`). 2. When using objects/arrays and there are frequent changes. 3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s). The differences in operations per digest include: 1. `Regular expression (when not changed)` **Before:** `equals()` **After:** `toClassString()` 2. `Regular expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** 2 x `split()` 3. `One-time expression (when not changed)` **Before:** `equals()` **After:** `toFlatValue()` + `equals()`* 4. `One-time expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** `copy()`* + `toClassString()`* + 2 x `split()` 5. `$index modulo changed` **Before:** `arrayClasses()` **After:** - (*): on flatter structure In large based on #14404. Kudos to @drpicox for the initial idea and a big part of the implementation. Closes #14404 Closes #15246

@drpicox

…d copies Includes the following commits (see angular#15246 for details): - **perf(ngClass): only access the element's `data` once** - **refactor(ngClass): simplify conditions** - **refactor(ngClass): move helper functions outside the closure** - **refactor(ngClass): exit `arrayDifference()` early if an input is empty** - **perf(ngClass): avoid deep-watching (if possible) and unnecessary copies** The cases that should benefit most are: 1. When using large objects as values (e.g.: `{loaded: $ctrl.data}`). 2. When using objects/arrays and there are frequent changes. 3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s). The differences in operations per digest include: 1. `Regular expression (when not changed)` **Before:** `equals()` **After:** `toClassString()` 2. `Regular expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** 2 x `split()` 3. `One-time expression (when not changed)` **Before:** `equals()` **After:** `toFlatValue()` + `equals()`* 4. `One-time expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** `copy()`* + `toClassString()`* + 2 x `split()` 5. `$index modulo changed` **Before:** `arrayClasses()` **After:** - (*): on flatter structure In large based on angular#14404. Kudos to @drpicox for the initial idea and a big part of the implementation. Closes angular#14404 Closes angular#15246

googlebot added the cla: yes label Apr 9, 2016

This was referenced Apr 9, 2016

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

Closed

Fix ngClass mixed array/object when object instance does not changes #14405

Closed

gkalpak added frequency: moderate needs: review component: misc core type: perf severity: inconvenient labels Apr 10, 2016

gkalpak added this to the Backlog milestone Apr 10, 2016

gkalpak reviewed Apr 10, 2016
View reviewed changes

drpicox force-pushed the perf-ngClass-watchExpr branch 2 times, most recently from d01e0c9 to 6b4ba2b Compare July 21, 2016 12:50

drpicox force-pushed the perf-ngClass-watchExpr branch 4 times, most recently from 64bae8d to 0a5b49f Compare July 21, 2016 14:52

drpicox force-pushed the perf-ngClass-watchExpr branch from 0a5b49f to 4d4c88c Compare July 21, 2016 15:35

drpicox force-pushed the perf-ngClass-watchExpr branch from 4d4c88c to 5fe61c2 Compare September 9, 2016 10:22

gkalpak self-assigned this Oct 6, 2016

gkalpak suggested changes Oct 6, 2016

View reviewed changes

drpicox force-pushed the perf-ngClass-watchExpr branch 2 times, most recently from e0d7ea8 to 510021d Compare October 7, 2016 22:14

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Oct 8, 2016

test(ngClass): add some tests about one-time bindings and objects ins…

2705085

…ide arrays Mostly taken from angular#14404.

gkalpak mentioned this pull request Oct 8, 2016

ngClass[Odd/Even] overhaul #15228

Closed

3 tasks

drpicox added 2 commits October 10, 2016 20:57

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

aafb9d9

…objects by refactor

test(ngClass): test large objects, ::one time expressions, and $index…

01e395a

… bug

drpicox force-pushed the perf-ngClass-watchExpr branch from 510021d to 01e395a Compare October 10, 2016 18:58

gkalpak closed this in b820970 Dec 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ngClass): refactor to optimize the case of static map of classes with large objects #14404

perf(ngClass): refactor to optimize the case of static map of classes with large objects #14404

drpicox commented Apr 9, 2016 •

edited

Loading

gkalpak Apr 10, 2016

drpicox commented Apr 10, 2016

drpicox commented Jul 21, 2016 •

edited

Loading

gkalpak commented Jul 21, 2016 •

edited

Loading

drpicox commented Jul 21, 2016

gkalpak commented Jul 21, 2016

Narretz commented Sep 8, 2016

Narretz commented Sep 8, 2016

gkalpak commented Sep 8, 2016

drpicox commented Sep 9, 2016 •

edited

Loading

drpicox commented Sep 9, 2016

drpicox commented Sep 9, 2016

gkalpak left a comment

gkalpak Oct 6, 2016

drpicox Oct 7, 2016

gkalpak Oct 7, 2016

drpicox Oct 7, 2016

gkalpak Oct 6, 2016

drpicox Oct 7, 2016

gkalpak Oct 7, 2016

drpicox Oct 7, 2016

gkalpak Oct 6, 2016

drpicox Oct 7, 2016

drpicox Oct 7, 2016 •

edited

Loading

gkalpak Oct 6, 2016

drpicox Oct 7, 2016

gkalpak Oct 6, 2016

drpicox Oct 7, 2016

gkalpak Oct 6, 2016

drpicox Oct 7, 2016

gkalpak commented Oct 8, 2016

drpicox commented Oct 8, 2016

perf(ngClass): refactor to optimize the case of static map of classes with large objects #14404

perf(ngClass): refactor to optimize the case of static map of classes with large objects #14404

Conversation

drpicox commented Apr 9, 2016 • edited Loading

Choose a reason for hiding this comment

drpicox commented Apr 10, 2016

drpicox commented Jul 21, 2016 • edited Loading

gkalpak commented Jul 21, 2016 • edited Loading

drpicox commented Jul 21, 2016

gkalpak commented Jul 21, 2016

Narretz commented Sep 8, 2016

Narretz commented Sep 8, 2016

gkalpak commented Sep 8, 2016

drpicox commented Sep 9, 2016 • edited Loading

drpicox commented Sep 9, 2016

drpicox commented Sep 9, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drpicox Oct 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 8, 2016

drpicox commented Oct 8, 2016

drpicox commented Apr 9, 2016 •

edited

Loading

drpicox commented Jul 21, 2016 •

edited

Loading

gkalpak commented Jul 21, 2016 •

edited

Loading

drpicox commented Sep 9, 2016 •

edited

Loading

drpicox Oct 7, 2016 •

edited

Loading