x5 optimization: do not run watchers on isolated scopes if their isolated bindings hasn't changed

[kostat]

This optimization reduces number of running watchers by x3 - x10 in apps with big number of isolated scopes.

How?

Do not run the isolated scope watchers if its scope definition bindings (e.g. {label: '@'}) hadn't changed.

Motivation

The common use case for directives with isolate scope is to create kind of a widget. With isolate scope bindings it "connects" to its "public" interface and should implement its internal functionality using ONLY these fields, model-wise. In other words, any inner watchers should entirely depend (directly or indirectly) on the "public" interface. Any direct watchers to parental scopes is discouraged.
Let's evaluate scenarios of what happens then:

• Templating: the mapped field is used inside a template, e.g. there is a mapping like label: '@' and somewhere in the template {{label}}. While the template is linked, it installs a watch that evaluates the expression. It's clear that if the outer @label is not changed it's useless to evaluate the expression since it's not changed. Consider that the expression can be more complex: {{label}}: {{getFullName()}}. Currently, all this runs several times during $digest cycle for nothing.
• Custom watches: the user installs a watch using a function(){...}. The function must return a value , in many cases the value is computed and the computation can be untrivial. The sad part is that if the isolated bindings hasn't changed, this computation should return the same value! So again, it will run for nothing...

Now consider an app that makes an intensive use of "widgets", like a typical LOB app. This optimization substantially boosts its performance (x5-x10).

Cons

Unfortunately this change can break an existing code. If the user creates watches to parental scopes, the system might behave abnormally: if the callback will change some scope values, the UI can be not updated.

• In the first place, this kind of watchers is discouraged and can be considered as a bug in user code
• I provide a method scope.$setDirty() that can be used to "fix" the problem

In any case, this issue makes this PR inappropriate for minor release. So there are 2 options:

1. The directive will mark somehow that a new isolated scope behavior can be used, in this case no old code will be broken and this can be merged.
2. Wait for a major release.

In my opinion (1) is better: like in Google Guava, there is a concept of @Beta features. This way the users might test the new functionality and provide feedback before it's released.

---

I created a demo app, which logs "skipped" scopes and # of watcher checks. In original angular number of checks is x3 - x10.

http://ngforms.azurewebsites.net/

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[kostat]

Linked issue is #10140

[kostat]

@googlebot: CLA signed

[kostat]

Just saw that the tests passed, but the coding conventions failed. Let me know when this will be the last issue to go merge, and I'll resubmit the PR.

[caitp]

I'm not sure this patch is realistic... I may not be in the best state of mind to review this right now, so it would be good if @lgalfaso or @petebacondarwin could take a look --- but I feel like this is adding another footgun for minimal usefulness. The administrative cost of determining this is likely greater than the savings of not evaluating watches (similar to bind-once bindings).

[jbedard]

Won't this also break basic scope functionality on isolated scopes? You can't $watch anything anymore..?

[kostat]

@caitp: I updated the initial comment above with Motivation and Cons sections. Hope this addresses your concerns.

[caitp]

I'm still not convinced, for the reasons mentioned above. administrative overhead tends to make these things feel more broken and feel less like an optimization. I would rather see this measured on real applications.

[kostat]

@caitp:

1. Can you be more specific and provide an example of "administrative overhead" - I'm not sure I with you to give you a good answer.
2. I made an app http://ngforms.azurewebsites.net/, that clearly shows the improvement - it logs # of skipped scopes to console. Would you like me to make another one with an original angular, so you could compare side by side?

[Narretz]

Is it possible that there's some confusion about how this behavior is applied? From the code it looks like the "isolation" is always applied for isolated scopes.

@kostat Can you please fix the indentation in the PR? It's really distracting that your changes are differently indented.

[kostat]

@Narretz: yes, the "isolation" is applied on isolated scopes only - see motivation above. Indentation fixed.

[lgalfaso]

This is one of those PRs that I feel that is doing the right thing for 50% of the applications out there and for the other 50%, will cause sever headaches (and just too many WTF moments)

[pkozlowski-opensource]

Yeh, I must say that I'm also not feeling comfortable with this PR... It might bring some gain for certain use-cases but does so at the expense of additional complexity / new corner cases etc. This would be -1 from me.

[kostat]

@lgalfaso, @pkozlowski-opensource: dear friends, I propose by no doubt a huge optimization. First I would like to hear from collaborators a feedback on the number. Second, if the number is correct, it's definitely a time to think how we get it in. This is a x5 optimization, not just nice code. It makes a big difference for a large app!
And if you want to talk about use cases, please be more specific. You reject a x5 optimization, please provide some serious and concrete argumentation against!

[jbedard]

Won't this break watching anything not declared on the isolated scope? myIsolatedScope.$watch(...) won't work...

[kostat]

@jbedard: no, it will work. But these watchers will run only if some of the "public" bindings has changed. Please read the Motivation section above why I consider that this is the right thing to do.

[jbedard]

I see, sorry I missed that. I think that point is very debatable and probably too big of a change to ever make though...

[kostat]

Guys, through this thread everyone tells that the point is debatable, but no concrete arguments against were provided... Give your arguments! The prize is high - x5 optimization!

[caitp]

I don't think this is realistically x5 optimization... where are you getting that number from? What did you measure? How are you so convinced that this extra administrative flag is going to be managed properly, not break other people, and not break in the future?

We have bind-once now, which has some use cases, but it doesn't really make sense as an optimization --- as an optimization, the administrative steps that need to be taken overrides any of the perf usefulness (so people instead just use it because the behaviour makes sense, rather than the speed). But in this case, there's no real behavioural incentive, it actually breaks more use cases than it enables. So I am just not seeing it.

[jbedard]

I want watchers to work? That's my argument...

Something as simple as ng-click="myVar = 123" will be broken...

[caitp]

Now seriously, when I go arguments (like these) with someone, I always ask/provide a code snippet that is fine, correct, worked before and broke after. Otherwise this is endless and does not lead to anything. So please fill your $watch() with something that finally proves your point. If so many apps will be broken, can you give one specific and concrete example?!

You have received exactly such a snippet, so this entire argument is becoming silly. $scope.$watch works now, would not work after this change, so... there you go, breaking change.

[kostat]

@caitp: I never said it's not a breaking change: see Cons at the top... Yet you said:

Seriously, this is a bad idea --- even as an opt-in solution

So if it provides a huge optimization for these who does it right and does not affect others, why it's a bad idea as an opt-in solution?

[caitp]

There's no doubt that this would be helpful for you, but it's questionable whether it's helpful for angular users in general. You're claiming that you're "doing it right" and that it will only break people who are "doing it wrong", but the real question is, who is to say who is doing it right or wrong? The API is exposed, and people are using it --- if it stops working for them, we broke them, their code worked before. If we make it opt-in, people will use it without understanding it, and then freak out when their third-party modules stop working. There's no real "good" way to do this, other than a well-documented breaking change in a subsequent major release --- and I'm not even sure it would be a good idea then, because it's not a real performance fix.

Being a brilliant software engineer, would you not rather look at ways to improve the data structures and algorithms used for dirty checking in general, in order to speed them up? Doing that will speed things up without breaking people.

[kostat]

@caitp: In software engineering the concepts of isolation, encapsulation, public/private interface are well known. Breaking these concepts lead to well known issues, so there is way to "do it right".

If we make it opt-in, people will use it without understanding it...

Why will they use without understanding? How all this relates to third-party modules? Why they will understand in major release?
See Google Guava project with @beta features. This is the real way to test new stuff! You have something fantastic - x5 speedup. You should fight to get it in, not out!

Instead you claim that your users will turn it on, use it wrongly and then file you wrong bugs. Is this what Google thinks about AngularJS developers?

P.S. What is the speedup potential of improving dirty checking? 10%, 20%? I'm giving 500% and I have scenarios with even 1000%! As a software engineer I'm looking first to improve a complexity class and not k-optimizations.

[caitp]

I am done arguing about this --- my position is that we should not break the API for illusory performance gains, but I'll let Pete worry about this.

[lgalfaso]

This PR would break an angular app for too many people, so unless we implement something like Zone.js or something similar, then this cannot land. If you still think that there is some value that would help with this, then please send an email to the angular forum, explain the idea and create a document explaining all the side-effects. Thanks

[petebacondarwin]

@kostat - thank you for all the time that you have put into this feature. We do appreciate any effort to improve the performance of Angular.

Just to be clear, I had a quick play with this too. The PR needed a bit of rebase-love (petebacondarwin/angular.js@angular:master...petebacondarwin:pr10221) but now we do indeed have a unit test failure, which is related to the new bindToController feature.

This test fails:

    it('should update @-bindings on controller when bindToController and attribute change observed', function() {
      module(function($compileProvider) {
        $compileProvider.directive('atBinding', valueFn({
          template: '<p>{{At.text}}</p>',
          scope: {
            text: '@atBinding'
          },
          controller: function($scope) {},
          bindToController: true,
          controllerAs: 'At'
        }));
      });

      inject(function($compile, $rootScope) {
        element = $compile('<div at-binding="Test: {{text}}"></div>')($rootScope);
        var p = element.find('p');
        $rootScope.$digest();
        expect(p.text()).toBe('Test: ');

        $rootScope.text = 'Kittens';
        $rootScope.$digest();
        expect(p.text()).toBe('Test: Kittens');
      });
    });

With the following error:

Expected 'Test: ' to be 'Test: Kittens'.

Also for an additional working broken example see the following pair of plunkers:

• No optimisation: http://plnkr.co/edit/TzdAbvIj0DkrAX0gQa6v?p=preview
• With optimisation: http://plnkr.co/edit/DhTGVsErIGpIWmoDdvAJ?p=preview

The application code is pretty simple:

// Code goes here

angular.module('app', [])

.factory('auth', function() {
  var loggedIn = false;
  return {
    isLoggedIn: function() {
      return loggedIn;
    },
    login: function() {
      loggedIn = true;
    },
    logout: function() {
      loggedIn = false;
    }
  };
})

.directive('loginview', function(auth) {
  return {
    restrict: 'E',
    scope: {},
    template: 'LoginView: <div ng-show="isLoggedIn">Logged In</div>',
    link: function($scope) {
      $scope.$watch(function() { return auth.isLoggedIn(); }, function(isLoggedIn) {
        $scope.isLoggedIn = isLoggedIn;
      });
    }
  };
})


.run(function($rootScope, auth) {
  $rootScope.login = function() {
    auth.login();
  };
  $rootScope.logout = function() {
    auth.logout();
  };
});

The thing to note is that we are watching an external service from inside our isolated scope. This is a common pattern in Angular applications and with the optimisation in place, we never get any update to the isolated scope when the auth service changes.

[kostat]

@petebacondarwin: thanks for your comments, it was a nice surprise to see that someone actually looks into this.
Let me state again the problem and hopefully contribute to sharpening it, as folks are trying to fix something in this area, like #10658.

The problem: as the application gets larger, number of $watchers running in each $digest increases non linearly (more $watchers -> more chances for multiple cycles) making it to run slow. But the real problem is that most of them run for nothing and there is no mechanism to detect this in advance.

Therefore I think that #10658 tries to solve the very same problem in a different way.

Now let me tell the logic behind the solution I propose. The goal was simple: "How to make it in a way to not break stuff subtly without really knowing why". While designing the feature, I set several requirements:

1. Declarative. No API like $suspend/$resume as this will make every case special. Ideally it must be a flag/indication on DDO.
2. Integrates well into angular architecture. So I can explain to the developers what to do and what not to do and it will be easily understood.
3. Effective - must prevent most of unneeded $watcher checks.
4. Opt-in. (It's not part of my PR, as there is no convention for this; clearly it can be easily added).

When I first encountered directives with isolated scope, I immediately compared them with controls/widgets existing in other UI toolkits (not only JS based). In all systems I know, displaying data that comes not from widgets properties (i.e. public widget interface) is discouraged. (In most cases it's not even possible since widgets come from 3rd party vendors - jquery, teleric, infragistics, openfaces - I cannot remember a component that displays anything that is not passed via public API).

Due to unknown (to me) reason this is OK in angular, and you refer this as a broken use case... in loginview. To fix it, I would recommend to make 'auth' a dependency for controller, have a method isLoggedIn() on it that will delegate to 'auth' service. Then the directive will look like this and work:

.directive('loginview', function() {
  return {
    restrict: 'E',
    scope: {
       visible: '=',
    },
    template: 'LoginView: <div ng-show="visible">Logged In</div>'
  };
})

And used as <loginview ng-controller="myController as ctrl" visible="ctrl.isLoggedIn()" />.

In any case, it was a very clear message to developers - display data/$watch things that come from isolated scope definition only. So we did not encountered 'subtle breakages' while using this feature.

P.S. of course it's easy to fix the bindToController test failure if there will be an interest in this PR.

[lgalfaso]

Hi, it looks clear that #10221 (comment) #10221 (comment) show different realities of what a directive is. The later makes the assumption that a directive only reacts based on a change from a bind on the parent scope. The former does not make this assumption and directive can change based on whatever event.
As Pete said, Angular developers do not make the assumptions made in the PR. People expect directive to have some smartness and logic in them. I would also vote against making this an opt-in, the reason being that for this to work all of your directives need to work this way, and as soon as any third party module is added, you have to be sure that it works in this specific way. This is a burden just too big.

[kostat]

@lgalfaso:

for this to work all of your directives need to work this way

This is not true. This can be safely work per directive, provided the opt-in indication is on DDO.

[petebacondarwin]

If it were opt-in then indeed it can and should be on a per-directive basis.

Let's identify all the possible cases that would break, even on an opt-in directive - then we can either ensure that they are all prevented or agree what would be an acceptable documented limitation.

One case that @lgalfaso has brought up is proper resolution of promises inside an isolated scope. @kostat - could you take a look at how this could be handled? I don't think that this would be an acceptable limitation; whereas not watching things outside the isolated scope could be.

[googlebot]

CLAs look good, thanks!

[kostat]

@petebacondarwin: resolution of promises: one option would be to expect a promise from the listener. if yes, we can subscribe to it and fix the situation. So in the @lgalfaso use case the user will write:

function FooContoller($scope, $q) {
  $scope.$watch('foo', function() {
    return $q.when().then(function() {
      $scope.bar = $scope.foo;
    });
  });
}

Another option is to require the user to invalidate the scope explicitly:

function FooContoller($scope, $q) {
  $scope.$watch('foo', function() {
    $q.when().then(function() {
      $scope.bar = $scope.foo;
      $scope.$setDirty();
    });
  });
}

My take is that both should be supported.

---

Regarding the broken use cases - I think it would be not easy to identify all of them or the list might be long with many esoteric things like ancestors scopes, services etc. Instead, I suggest to say what to do, since the message is very simple: listener code/template will be updated only if one of "public" bindings has changed. + the promises use case above.

[googlebot]

CLAs look good, thanks!

[lgalfaso]

In Angular 1, we lack the machinery to detect when promises are resolved and when timers executed (and in what context/scope), so this cannot land. Angular 2 adds some machinery that makes this possible (and adds the concept of a pure directive), but this machinery is too heavy to be just added to Angular 1. Closing this as there is no easy way for Angular 1 to have this machinery without too many breaking changes.