refactor($interpolate): optimize watched $interpolate functions performance

[rodyhaddad]

This PR improves the performance of watched $interpolate functions: http://jsperf.com/ng-interpolation-optimization/8

This is the related Google groups thread, I explain my PR there: https://groups.google.com/forum/#!msg/angular-dev/QmcwYVkfsbo/8Y_pMsal0kYJ

If any changes are unclear, point them out and I'd be happy to elaborate on them :)

Signed the CLA as Rodric Haddad

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[rodyhaddad]

For the curious:
The docs say it should return null, but now it returns undefined.
I changed the code instead of changing the docs since returning null made more sense IMHO.
I'm not sure if adding a Breaking Change note is necessary. I would imagine people testing if the returnValue of $interpolate is truthy (or is a function), instead of explicitly checking if it's undefined, specially since the docs say it should return null.

[IgorMinar]

thanks for the PR. I scheduled this PR to be reviewed and merged for v1.2.1.

[rodyhaddad]

ok, no problem 👍

Just as a side-note (something I thought of yesterday): the addition of $$beWatched would allow for #643 to be resolved elegantly :D, I would send a PR once this one is reviewed and merged

[rodyhaddad]

I rebased against master, and fixed a bug that I didn't catch in my initial PR (which also made the code a bit cleaner).

Here's the new jsperf http://jsperf.com/ng-interpolation-optimization/8

If there's any issues or questions with my changes, don't hesitate to add a comment!

[Narretz]

+1

[g00fy-]

+1

[IgorMinar]

@rodyhaddad please take a look at https://github.com/IgorMinar/angular.js/tree/pr-4556 I left a commit there for you that I created during code review of this change.

I think that this change is heading in the right direction but there are some major flaws:

1/ the benchmark doesn't take into consideration DOM operations and this change increases the number of DOM writes from 1 per interpolated string to n-times per interpolated string (where n is the number of interpolations in the string). I suspect that because of this, this change will actually make angular slower not faster. This is however fixable. you could just keep track of which parts have been dirty-checked and only the last part should trigger the watch action (which usually is the write).

2/ I also feel that the way the watching is set up is wrong. Why can't $compile use the $$beWatched fn of the result of calling $interpolate and pass that into $watch directly instead of $watch checking everything for the presence of $$beWatched?

I'm going to move this to 1.2.7 and if you can get these issues fixed, I'll be happy to review them again.

Please don't squash the changes in this PR, complex PRs like this one are easier to review if the iterative changes are not squashed. I'll squash everything before merging to master.

[rodyhaddad]

@IgorMinar thx for the reply :)

---

this change increases the number of DOM writes from 1 per interpolated string to n-times per interpolated string (where n is the number of interpolations in the string)

um, no, it doesn't
This change increases the number of watches, not DOM writes, DOM writes remain the same.

DOM writes happen in the origListener, which gets called the same amount of times as before this PR. I added a test to explicitly address this

I've had the same concern as you as I was writing this PR.
I could elaborate on how the code solves it, but I think the added tests prove that this behaves as wanted

you could just keep track of which parts have been dirty-checked and only the last part should trigger the watch action

I solve this stuff with lastValues

If I'm missing anything, a flaw you saw that my code doesn't cover, writing a failing test would be the most beneficial

---

Why can't $compile use the $$beWatched fn of the result of calling $interpolate and pass that into $watch directly

It's a design choice.
Keeping things modular is important IMO. $compile shouldn't be concerned whether $interpolate can handle being watched more efficiently. For all $compile knows, $interpolate has been overwritten by some 3rd party module adding more features (that's why angular's DI is awesome).

I can imagine someone providing a module that overwrites $parse, implementing it's own $$beWatched.
So that if you do $parse("prop"), it uses Object.observe($scope.prop),
but if you do $parse("fn() ? 1 + z : a.b.c"), it doesn't do anything.
And with $$beWatched being checked in $scope.$watch, no other service needs to be changed.
Does $parse suddenly know how to be watched more efficiently? then $parse should be the only one concerned
Same thing with $interpolate

I can image $$beWatched be used for something like bindonce, or for translations.
Allowing different parts to have more control on how they get watched (example: solving #643, and getter/setter functions in general)

instead of $watch checking everything for the presence of $$beWatched?

I'm guessing performance is the issue here? an extra if is negligible considering that $watch only gets called once on compilation (it's not in a digest). Also, it's nothing when you compare it to the other stuff that's happening in $watch :p

[IgorMinar]

note to myself: punted from ~1.2.6 - I need to get to this this week

[rodyhaddad]

@IgorMinar I'm often connected on the #angularjs irc channel during the day, so don't hesitate to message me if you have any questions/concerns about this PR that pop up and want a quick reply

[IgorMinar]

note to myself: gah! another punt

[IgorMinar]

@rodyhaddad thanks, I'll do it. it might need to wait until after ng-conf. this is a significant change and I don't want to break stuff before the conference ;-)

In the meantime, @caitp can you also please take a look at this one?

[rodyhaddad]

No problem, no need to rush, I already got my "Igor accepted my Pull Request" badge from another PR ;-P

[IgorMinar]

Hi Rody,

We thought about this a lot and decided not to merge this PR after all.

The solution we do want is already implemented in AngularDart: https://github.com/angular/angular.dart/blob/4bf9447cc64650d6c73b66c844fb5396b4a2ae27/lib/core/scope.dart#L278

Would you be interested in implementing that instead? We'd like to have that in v1.3.

[rodyhaddad]

Sounds good, I'll try to have the new PR as soon as possible 👍