Expression caching #9082

IgorMinar · 2014-09-14T22:36:24Z

improved version of #9006 (which was an improved version of #8942)

IgorMinar · 2014-09-14T22:39:26Z

@jbedard I rebased your PR #9006 and made several improvements and fixed some small bugs.

I think this is pretty close to being mergable. The missing pieces are:

rename externalInput to something more intuitive?? and prefix it with $
better docs and commit messages

Please take a look at my changes and let me know if you see anything odd

jbedard · 2014-09-14T22:59:20Z

Looks like you didn't rebase the latest commit? I had a couple additional
changes to support short-circuiting operators and interceptors better, also
added tests and fixed a watchCollection issue...

IgorMinar · 2014-09-15T10:07:29Z

Yup. You are right. I'll update my PR later. I'm on cell so it's hard to read your diff but from what I've seen, I'm not quite sure how does the isBranching check make things any better. Can you shed some light on this?

caitp · 2014-09-15T14:18:12Z

test/ng/parseSpec.js

+            expect(watcherCalls).toBe(1);
+            scope.$digest();
+            expect(filterCalls).toBe(1);
+            expect(watcherCalls).toBe(1);


I don't really understand this test --- "should calculate the literal every single time", but asserting that the filter and watcher was only called once in two digests.

What are we actually testing here? That, because the inputs didn't change, we don't call the filter / consider the watch to have changed?

I think I pointed out the same. I'm going to take a closer look at the
tests before merging.
On Sep 15, 2014 4:18 PM, "Caitlin Potter" notifications@github.com wrote:

In test/ng/parseSpec.js:

$filterProvider.register('foo', valueFn(function(input) {

filterCalls++;

return input;

}));

var watcherCalls = 0;

scope.$watch($parse('{x: 1} | foo'), function(input) {

expect(input).toEqual({x:1});

watcherCalls++;

});

scope.$digest();

expect(filterCalls).toBe(1);

expect(watcherCalls).toBe(1);

scope.$digest();

expect(filterCalls).toBe(1);

expect(watcherCalls).toBe(1);

I don't really understand this test --- "should calculate the literal
every single time", but asserting that the filter and watcher was only
called once in two digests.

What are we actually testing here? That, because the inputs didn't change,
we don't call the filter / consider the watch to have changed?

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/9082/files#r17543950.

The tests were also updated in the latest commit. Really I think all this test should do is...

expect($parse('{x: 1} | foo').constant).toBe(true);

This test got moved down after the merge/cherry-pick, although I think it should be simplified like I mentioned above.

I removed this dupe.

the other copy does contain the "constant" assertion. I kept the other assertions as they were. I think that they are useful.

jbedard · 2014-09-15T16:03:38Z

The isBranching is set for AND/OR operators and is mainly to fix short-circuiting such as foo || bar(). AND/OR will no longer be broken down to left/right inputs but instead the full expression is watched so the short-circuiting runs as normal. I also added a AND/OR operator benchpress case where this actually seemed faster then watching both foo and bar(), but the main reason for the isBranching flag was to fix short-circuiting so I didn't look into the performance too much. Expressions such as a == 1 || a == 2 | a == 3 | ... would probably improve by only watch the input a so this could be improved in the future (by detecting that the expression has no possible side-effects and then only watching the inputs?).

IgorMinar · 2014-09-15T20:59:13Z

@jbedard I pulled in the changes from your branch and cherry-picked them on top of this branch. The use of isBranching makes sense. I didn't make the connection when reading the code on tiny screen.

As far as valueOf goes, we do need a different strategy because both of our solutions are incorrect. I was also thinking of keeping the primitive value around and comparing that. I'll give it a shot.

jbedard · 2014-09-15T21:07:19Z

test/ng/parseSpec.js

+          expect(watcherCalls).toBe(1);
+        }));
+        it('should not treat constants passed to filters with externalInput as constants', inject(function($parse) {
+


And this one should just set the externalInput flag on the foo filter, and expect(parsed.constant).toBe(false).

such expression would result in infinite digest though because we'd recreate the constant object during each reevaluation.

this could be fixed in the future, but I'm not going to deal with it now. instead I'll just remove this single spec.

Right, didn't think of that. And it won't have the normal literal handling
because the literal is not the root...

In test/ng/parseSpec.js:

var watcherCalls = 0;

scope.$watch(parsed, function(input) {

expect(input).toEqual({x:1});

watcherCalls++;

});

scope.$digest();

expect(filterCalls).toBe(1);

expect(watcherCalls).toBe(1);

scope.$digest();

expect(filterCalls).toBe(1);

expect(watcherCalls).toBe(1);

}));

it('should not treat constants passed to filters with externalInput as constants', inject(function($parse) {

such expression would result in infinite digest though because we'd
recreate the constant object during each reevaluation.

this could be fixed in the future, but I'm not going to deal with it now.
instead I'll just remove this single spec.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/9082/files#r17571465.

such expression would result in infinite digest though because we'd
recreate the constant object during each reevaluation.

Actually, that sounds like a problem --- because it sounds like it could break peoples code. I might be missing something, but I don't think we want to not fix that

I think that is nothing new, but now it will only happen when the filter
also has the "externalInput" flag. I think the test was just bad...

For example $scope.$watch("[0] | filter") would always cause this because
the output is a literal (recreated each digest) but is not constant because
of the filter.

On 15 September 2014 15:21, Caitlin Potter notifications@github.com wrote:

In test/ng/parseSpec.js:

var watcherCalls = 0;

scope.$watch(parsed, function(input) {

expect(input).toEqual({x:1});

watcherCalls++;

});

scope.$digest();

expect(filterCalls).toBe(1);

expect(watcherCalls).toBe(1);

scope.$digest();

expect(filterCalls).toBe(1);

expect(watcherCalls).toBe(1);

}));

it('should not treat constants passed to filters with externalInput as constants', inject(function($parse) {

such expression would result in infinite digest though because we'd
recreate the constant object during each reevaluation.

Actually, that sounds like a problem --- because it sounds like it could
break peoples code. I might be missing something, but I don't think we want
to not fix that

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/9082/files#r17572834.

yeah. this has always been this way. so i don't think that it's worth fixing right now.

IgorMinar · 2014-09-15T23:10:50Z

@jbedard I fixed the valueOf issue and added some more tests. PTAL

jbedard · 2014-09-15T23:20:56Z

src/ng/parse.js

+          var newInputValue = inputExpressions(scope);
+          if (!expressionInputDirtyCheck(newInputValue, oldInputValue)) {
+            lastResult = parsedExpression(scope);
+            oldInputValue = newInputValue.valueOf();


Does this need newInputValue && to handle null/undefined like the one below?

jbedard · 2014-09-15T23:48:50Z

Other then the null/undefined comment everything LGTM. Except externalInput still needs a better public name (that's the hardest part :)

I had a few other ideas/questions but they could all wait:

Should the filter filter set externalInput since it almost always takes non-primitive input?
Should the loop in expressionInputsWatch break if it hits a non-primitive? That might reduce the overhead for expressions that always have non-primitives.
Should externalInput not be a flag but be a watched input/getter just like filter arguments? Then for things like translate filters it could be the language, so when the language changes the filter will be re-evaluated. But this would be a more complicated API then just a flag.
Currently expression inputs are recursed, combined, constants removed etc at $watch time and cached as $$inputs for any future watchers. But after $$inputs is calculated and cached all the inputs are left behind and never used again. Should those be cleaned up as we recurse (if !sharedGetter)? Or move the recursing/combining/removing-constants to parse time instead of watch time? Or is a small array of expressions not worth cleaning up (the expressions are all referenced elsewhere so only the array itself is can be GCed if we remove the reference)?

With this change, expressions like "firstName + ' ' + lastName | uppercase" will be analyzed and only the inputs for the expression will be watched (in this case "firstName" and "lastName"). Only when at least one of the inputs change, the expression will be evaluated. This change speeds up simple expressions like `firstName | noop` by 20% and more complex expressions like `startDate | date` by 2500%. BREAKING CHANGE: all filters are assumed to be stateless functions Previously it was a good practice to make all filters stateless, but now it's a requirement in order for the model change-observation to pick up all changes. If an existing filter is statefull, it can be flagged as such but keep in mind that this will result in a significant performance-penalty that will affect the $digest duration. To flag a filter as stateful do the following: myApp.filter('myFilter', function() { function myFilter(input) { ... }; myFilter.$stateful = true; return myFilter; }); Closes angular#9006 Closes angular#9082

IgorMinar · 2014-09-16T00:44:33Z

I cleaned up the commits, squashed most of them and wrote a better commit message.

To answer your questions

I'm hoping that filter filter could take advantage of custom valueOf() for objects and arrays which would turn it into a really fast filter
I thought of breaking out of the expressionInputsWatch faster but it looked like it would make things worse. We should profile some common scenarios and then decide.
externalInput is now $stateful flag. we can consider providing a way to watch external input later
I don't think that freeing up the inputs array would result in noticeable mem improvements for the reasons you mentioned.

With this change, expressions like "firstName + ' ' + lastName | uppercase" will be analyzed and only the inputs for the expression will be watched (in this case "firstName" and "lastName"). Only when at least one of the inputs change, the expression will be evaluated. This change speeds up simple expressions like `firstName | noop` by ~15% and more complex expressions like `startDate | date` by ~2500%. BREAKING CHANGE: all filters are assumed to be stateless functions Previously it was a good practice to make all filters stateless, but now it's a requirement in order for the model change-observation to pick up all changes. If an existing filter is statefull, it can be flagged as such but keep in mind that this will result in a significant performance-penalty that will affect the $digest duration. To flag a filter as stateful do the following: myApp.filter('myFilter', function() { function myFilter(input) { ... }; myFilter.$stateful = true; return myFilter; }); Closes angular#9006 Closes angular#9082

With this change, expressions like "firstName + ' ' + lastName | uppercase" will be analyzed and only the inputs for the expression will be watched (in this case "firstName" and "lastName"). Only when at least one of the inputs change, the expression will be evaluated. This change speeds up simple expressions like `firstName | noop` by ~15% and more complex expressions like `startDate | date` by ~2500%. BREAKING CHANGE: all filters are assumed to be stateless functions Previously it was a good practice to make all filters stateless, but now it's a requirement in order for the model change-observation to pick up all changes. If an existing filter is statefull, it can be flagged as such but keep in mind that this will result in a significant performance-penalty (or rather lost opportunity to benefit from a major perf improvement) that will affect the $digest duration. To flag a filter as stateful do the following: myApp.filter('myFilter', function() { function myFilter(input) { ... }; myFilter.$stateful = true; return myFilter; }); Closes angular#9006 Closes angular#9082

splitting the code into two chunks doesn't buy us anything and only makes the code harder to follow

IgorMinar mentioned this pull request Sep 14, 2014

WIP: Filter cache #8942

Closed

IgorMinar added this to the 1.3.0-rc.2 milestone Sep 14, 2014

IgorMinar mentioned this pull request Sep 14, 2014

WIP - only watch the inputs of parsed expressions #9006

Closed

mary-poppins added cla: yes and removed cla: no labels Sep 14, 2014

caitp reviewed Sep 15, 2014
View reviewed changes

tbosch modified the milestones: 1.3.0-rc.2, 1.3.0-rc.3 Sep 15, 2014

IgorMinar force-pushed the expression-caching branch from edb554a to d1b57df Compare September 15, 2014 20:56

jbedard reviewed Sep 15, 2014
View reviewed changes

IgorMinar force-pushed the expression-caching branch from d1b57df to 6bc090d Compare September 15, 2014 21:57

jbedard reviewed Sep 15, 2014
View reviewed changes

chore(largetable-bp): fix typo in main.html

e17a233

IgorMinar force-pushed the expression-caching branch from f1b25c4 to bbeeeb8 Compare September 16, 2014 00:38

IgorMinar force-pushed the expression-caching branch from bbeeeb8 to a9f87bd Compare September 16, 2014 00:51

jbedard and others added 3 commits September 16, 2014 12:17

refactor($parse): name anonymous fns for easier debugging and profiling

b4063f7

refactor($parse): merge ternary and ternaryFn methods

aaa8e7d

splitting the code into two chunks doesn't buy us anything and only makes the code harder to follow

IgorMinar force-pushed the expression-caching branch from a9f87bd to aaa8e7d Compare September 16, 2014 10:17

petebacondarwin force-pushed the master branch from 02dc2aa to fd2d6c0 Compare September 16, 2014 11:26

IgorMinar closed this in fca6be7 Sep 16, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expression caching #9082

Expression caching #9082

IgorMinar commented Sep 14, 2014

IgorMinar commented Sep 14, 2014

jbedard commented Sep 14, 2014

IgorMinar commented Sep 15, 2014

caitp Sep 15, 2014

IgorMinar Sep 15, 2014

jbedard Sep 15, 2014

jbedard Sep 15, 2014

IgorMinar Sep 15, 2014

jbedard commented Sep 15, 2014

IgorMinar commented Sep 15, 2014

jbedard Sep 15, 2014

IgorMinar Sep 15, 2014

jbedard Sep 15, 2014

caitp Sep 15, 2014

jbedard Sep 15, 2014

IgorMinar Sep 15, 2014

IgorMinar commented Sep 15, 2014

jbedard Sep 15, 2014

IgorMinar Sep 16, 2014

jbedard commented Sep 15, 2014

IgorMinar commented Sep 16, 2014

Expression caching #9082

Expression caching #9082

Conversation

IgorMinar commented Sep 14, 2014

IgorMinar commented Sep 14, 2014

jbedard commented Sep 14, 2014

IgorMinar commented Sep 15, 2014

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

jbedard commented Sep 15, 2014

IgorMinar commented Sep 15, 2014

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

IgorMinar commented Sep 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Sep 15, 2014

IgorMinar commented Sep 16, 2014