perf(Scope): use remove the need for the extra watch in $watchGroup #8396
Conversation
Instead of using a counter and an extra watch, just schedule the reaction function via . This gives us the same/similar ordering and coalecsing of updates as counter without the extra overhead. Also the code is easier to read. Since interpolation uses watchGroup, this change additionally improves performance of interpolation. In large table benchmark digest cost went down by 15-20% for interpolation. Closes angular#8396
|
@caitp, @rodyhaddad can you please review? |
|
|
||
| return function deregisterWatchGroup() { | ||
| while (deregisterFns.length) { | ||
| deregisterFns[0](); |
There was a problem hiding this comment.
I don't know how this used to work. It wouldn't it result in an infinite loop?
There was a problem hiding this comment.
I was going to ask, because I don't see it splicing the array in the diff (but I might just be missing it).
However, is it really good to reverse the order of deregistration, assuming it's working currently? Should this be shift() instead of pop()?
There was a problem hiding this comment.
It worked previously because of this: https://github.com/angular/angular.js/blob/master/src/ng/rootScope.js#L358-L364
There was a problem hiding this comment.
The removing used to happen here. It was confusing.
Since we no longer need to track which watchers are getting deregistered, this is a lot better :-)
I doubt changing pop to shift would actually change much for this.
There was a problem hiding this comment.
Just be aware that the order is changing here, and we don't really know if that will matter
There was a problem hiding this comment.
It would change things for deregisterNotifiers in $watch, but I believe this PR should be followed by one that removes deregisterNotifiers completely.
$watchGroup was the only place they were getting used, and we don't have a reason to support them in the future.
This will make the watcherDeregister functions only splice the $$watchers array, so order wouldn't matter
There was a problem hiding this comment.
I am all for removing deregisterNotifiers
There was a problem hiding this comment.
+1 for removing deregisterNotifiers in a separate commit.
I realized that the order has changed, but didn't think that it could cause problems. I'll make a change to keep the original order.
IgorMinar
changed the title
Instead of using a counter and an extra watch, just schedule the reaction function via . This gives us the same/similar ordering and coalecsing of updates as counter without the extra overhead. Also the code is easier to read. Since interpolation uses watchGroup, this change additionally improves performance of interpolation. In large table benchmark digest cost went down by 15-20% for interpolation. Closes angular#8396
|
LGTM 👍 |
|
any other comments? I'll make the changes on the flight home tonight. |
|
Yeah it generally looks good to me |
|
Would it be possible to add |
Instead of using a counter and an extra watch, just schedule the reaction function via . This gives us the same/similar ordering and coalecsing of updates as counter without the extra overhead. Also the code is easier to read. Since interpolation uses watchGroup, this change additionally improves performance of interpolation. In large table benchmark digest cost went down by 15-20% for interpolation. Closes angular#8396
We no longer have a need for this feature that was added to primarily support $watchGroup (see previous commit). BREAKING CHANGE: deregisterNotifier callback for $watch is no longer available This api was available only in the last few 1.3 beta versions and is not very useful for applications, so we don't expect that anyone will be affected by this change.
|
I changed the deregistration fn order and added a commit to remove the deregister notifier so we should be all set. @petebacondarwin we don't want to encourage people to use slow deep watching. |
|
This caused some problems, reproduced in http://plnkr.co/edit/6vAF3sBLbGsYWTGTS6bv?p=preview (basically with this SHA their $watch listener is never invoked) #8542 |
|
It never got invoked because the parseFns of $interpolate would be an empty array. Without this optimization, the I'm trying to think of a good way to handle the situation when the array is empty. This optimization is worth an extra |
|
I agree --- but I think it might have to wait for next week :( |
|
assuming we are shipping today --- I'm not sure if we're retesting g3 or what |
Instead of using a counter and an extra watch, just schedule the reaction function via . This gives us the same/similar ordering and coalecsing of updates as counter without the extra overhead. Also the code is easier to read. Since interpolation uses watchGroup, this change additionally improves performance of interpolation. In large table benchmark digest cost went down by 15-20% for interpolation. Closes angular#8396
|
Landed again with a fix as 1a05daf...bf0e837 |
Instead of using a counter and an extra watch, just schedule the reaction function via .
This gives us the same/similar ordering and coalecsing of updates as counter without the extra
overhead. Also the code is easier to read.
Since interpolation uses watchGroup, this change additionally improves performance of interpolation.
In large table benchmark digest cost went down by 15-20% for interpolation.