WIP - only watch the inputs of parsed expressions #9006
Conversation
|
So, what you are saying is that this change makes some expressions faster but others slower? If so, you should also add benchmarks for the stuff that gets slower, so it's easier to see if the change makes sense overall (factoring in how common these expression are). The externalInput flag seems necessary if this goes in. |
|
This is very interesting, but I am wondering if it should be able to handle expressions within function calls. This is expressions like |
|
Currently function calls are considered an input so they are reevaluated every time (along with their arguments). If |
|
Hi, it('should calculate the literal every single time', inject(function($parse) {
$filterProvider.register('foo', valueFn(function(input) {
return input;
}));
scope.$watch($parse('{x: 1} | foo'), function(input) {
expect(input).toEqual({x:1});
});
scope.$digest();
})); |
|
There are a couple issues with this PR right now. I almost have an update ready which will also fix the unit tests. I'll try to update this tonight and I'll look into your test case. One question I have (for anyone): should interceptor functions depend on external inputs or only depend on the output of the $parsed expression? Currently the isolated scope reference interceptor has external state that it depends on, so even if the watched $parse statement has not changed it must be called anyway. I think this is wrong and the isolated scope reference watcher should be updated, which would make this PR easier and could avoid the interceptorFn being called per digest (only call it when inputs change...). I think this would be a 2 line change to make the isolated scope watcher a normal function watcher instead of $parser + interceptorFn... |
|
At |
|
@lgalfaso Thanks, that's good to know. So I think I'll treat interceptors like filters and add the same Your test case is also interesting. That expression needs to be marked as a constant now that we are assuming filters (unless marked with |
jbedard
force-pushed
the
parse-watch-children
branch
2 times, most recently
from
0f025c8 to
e656a78
Compare
| for (var i=0, ii=inputExpressions.length; i<ii; i++) { | ||
| var valI = inputExpressions[i](scope); | ||
| if (changed || (changed = !simpleEquals(valI, inputs[i]))) { | ||
| inputs[i] = valI; |
There was a problem hiding this comment.
I'm thinking of putting a break here. This may cause a couple extra parseExpression calls the first few digests until all the inputs have been evaluated once, but this will reduce the overhead when inputs are non-primitive. Or only break if non-primitive (maybe simpleEquals returns null instead of true/false to indicate a non-primitive), this might actually be the best for both cases...
|
Updated to pass all the tests and handle the case @lgalfaso pointed out. The Also changed the term "children" in the code to "inputs" like I've been using in the PR. I'd still like to handle the main two issues I mentioned in the PR. I think expression nodes having a "has-side-effect" flag could handle one of the issues. Hopefully the comment I made in |
jbedard
force-pushed
the
parse-watch-children
branch
from
e656a78 to
c6a6dd9
Compare
|
I think that executing I'm thinking of the best way to get this in, and it really comes down to identifying all the breaking changes and making them now. We are already in RCs and I'm not keen on any further breaking changes, but I'm willing to make an exception for this particular change because of the benefits it brings. Am I correct that the only intentional breaking change is that filters are now treated as pure functions? Is there anything else? If that's it then we should make a change that will treat filters in this new way and then take our time to fine-tune this PR. |
|
Updated again. Added some tests and another perf test case. Removed the input-watching for branching expressions which fixes what you just mentioned with I slightly changed how the interceptorFn works so that Breaking changes:
I think that's it, but I'm in a rush so I might be forgetting one :P I agree that the breaking change should be made asap, then we can work on the implementation more. I think the One other thought I had was treating this flag like an input to the filter/interceptor. Then for things such as translation filters this value can be the language and $parse can watch it like any other argument to the filter. Then changing the language will cause the filter to be re-executed... |
|
?! |
| expect(filterCalls).toBe(1); | ||
| expect(watcherCalls).toBe(1); | ||
| })); | ||
| it('should not treat constants passed to filters with externalInput as constants', inject(function($parse) { |
There was a problem hiding this comment.
are you planning on writing this test?
There was a problem hiding this comment.
Was a bit rushed this morning but wanted to post an update so I may have missed a few things...
|
I rebased this PR and added some changes. see #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 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
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
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa to
fd2d6c0
Compare
This is a newer version of jbedard@844d04c which was discussed as an alternative to #8942.
The first commit should be good either way and makes
a | noopabout 20% faster in the benchmakrs by avoiding 2 unnecessary wrapper functions (binaryFn and valueFn). I can put this in a separate PR if wanted, but the main change depends on it.The main change:
a + -a * b -b + 5will watchaandbfoo().field), object index (obj[sub-exp]) and function calls (and ; separated statements, at least for now)externalInputflag that filters can set if they have some external state, then those filters will be considered inputsIn the expression watching benchmarks (which do not have the issues listed below):
Issues:
$rootScope.$watch('isLoggedIn || loggedOutAction()'). In this case, with the current implementation,isLoggedInandloggedOutAction()would be considered inputs and executed in the watcher.I might try other solutions to fix the two issues - maybe something simpler that only supports filters/literals for now which will get the best performance gains with the easiest workaround for those 2 issues.