scope.$watchGroup + $interpolation speedup #7158
Conversation
|
@rodyhaddad what do you think about these changes? |
Given an array of expressions, if any one expression changes then the listener function fires
with an arrays of old and new values.
$scope.watchGroup([expression1, expression2, expression3], function(newVals, oldVals) {
// newVals and oldVals are arrays of values corresponding to expression1..3
...
});
Port of dart-archive/angular.dart@a3c31ce
BREAKING CHANGE: the function returned by $interpolate no longer has a `.parts` array set on it. It has been replaced by two arrays: * `.expressions`, an array of the expressions in the interpolated text. The expressions are parsed with $parse, with an extra layer converting them to strings when computed * `.separators`, an array of strings representing the separations between interpolations in the text. This array is **always** 1 item longer than the `.expressions` array for easy merging with it
| concat.length = separators.length + expressions.length; | ||
|
|
||
| var lastValues = []; | ||
| var lastResult; |
There was a problem hiding this comment.
Having these variables be "per interpolationFn" has the following effect:
If an interpolationFn gets watched on multiple scopes, the optimization disappears. The inputsChanged below will always be true, making it re-compute things like before.
The frequency of this scenario is relatively high. The textInterpolateDirective calls $interpolate once and watches it every time it's linked to a scope.
This implies that the optimization we're after no longer has an effect for {{ }} in text nodes. when they're in a DOM that gets linked to more than 1 scope at the same time. I believe ng-repeat is the only directive that does that.
Example:
<div ng-repeat="val in [0, 1]">
{{ $index }}
</div>The lastValues will keep switching from [0] to [1] and vice-versa on every digest. This won't cause a 10 $digest error, because interpolationFn will still return the correct string for each scope. But it would be re-computing it (like before this PR), maybe making it a bit worse because there's 2 for-loops in play now (speculation, they're smaller now)
A quick way to solve this is by calling $interpolate again every time the textInterpolateDirective gets linked. I'm not sure how you feel about that.
There was a problem hiding this comment.
interesting observation! using watchGroup keeps the state associated with the scope so then we don't have this problem. But watchGroup also means that you moving the responsibility of watching for changes onto the client of $interpolate which means that it's an awkward opt-in optimization.
I wonder if we could keep a local catch of values in $interpolate that would be associate the values with scopes identified via scope ids.
I'll try to write this up now.
|
Is the breaking change regarding a proper Scope instance necessary? Why not do This also means we can hide the |
| @@ -777,6 +777,89 @@ describe('Scope', function() { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('$watchGroup', function() { | |||
There was a problem hiding this comment.
What do you think of these tests instead.
Although they don't test that the api works with 1 expression in the array, they use the log service, and spies for the deregistration test, instead of a log string variable.
You can take this commit and squash it to my first one. It also removes the single expression optimization, and makes use of self = this for nicer code. (I had added the commit to my last PR. I'm guessing you pulled it before I pushed)
There was a problem hiding this comment.
I already made other changes to those tests, so I just rewrote them to use log service.
|
I change the branch to use $parse directly instead of relying on scope. This has additional tiny benefit of being able to cache parsed getter functions closer to $interpolate, but I don't think that make a big difference. |
|
@rodyhaddad alright, so I made all the changes + rebased on top of master and push to my branch the only thing that remains is the issue with ngRepeat that you mentioned earlier. I'm looking into that now |
|
in 0b6cbf7 I also added per-scope cache, so I think we should be good now. the implementation is not ideal, but at this point I don't think that an ideal solution exists that is not a huge breaking change. |
|
the cache is per interpolation + per scope, so I don't think that the problem you describe is really a problem. but I'll take another look. |
|
Oh! my bad Actually this is great! |
|
yeah. I think it's fine. I'm landing it now |
|
landed as 21f9316...546cb42 |
|
Thanks rody! |
|
Thank you Igor! |
This is my refactoring of @rodyhaddad's PR #6598
I tried several approaches to clean it up and ended up doing a full circle by keeping the $interpolate optimization that Rody did for parsing but then reimplementing the
$watchGrouplogic inside of the function returned by $interpolate. This resulted in almost no breaking change (the only difference is that fn returned by $interpolate now expects a proper Scope instance rather than an object), removal of all hacks and still preserving the performance boost.There are three minor todos left before this can be merged, but I'm throwing it out here so that others can start reviewing.
Todos left:
computefn currently exposed as a property of the fn returned by$interpolate. this requires changes to 17 tests.