Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

perf(*): more performant interpolation and lazy one-time binding #7700

Merged
merged 2 commits into from
Jul 15, 2014

Conversation

rodyhaddad
Copy link
Contributor

This PR implements a different approach to one-time binding, which also gives us a performant improvement for $interpolate by about 33%.

Here's a benchmark app using the current snapshot: plunkr
Here's the same benchmark with this PR: plunkr

Look at the difference between the two baseline interpolation. It goes from 30-35ms to 20-25ms.
This gets us closer the the baseline binding, which is about 15-20ms.
The other 3 benchmarks in the benchmark app remain unchanged.

What's behind this change:

Expressions can say how they want to be watched

This allows the following:

  • Given interFn = $interpolate('{{a}}{{b}}'), interFn can say that it wants to be watched by: watching each expression individually (i.e. scope.$watchGroup(['a', 'b'])
  • Given parseFn = $parse('1 + 1'), parseFn can say that it wants to be watched by: getting evaluated on the next $digest, and then unregistering itself since its constant anyway
  • Given parseFn = $parse('::foo'), parseFn can say that it wants to be watched by: getting evaluated on the $digest as usual, and unregistering itself when its value is no longer undefined (a.k.a. bind-once)

InterceptionFn

There are cases where the last scenario doesn't work out as we'd like.
If we're watching a function that calls parseFn inside it, we lose the benefits mentioned above. That's because parseFn isn't aware that it's being watched (indirectly). So I created an interceptorFn in $parse that goes around this issue. Interceptors work as filters and you can easily see their application in ngBind.js or sce.js


I could go into more details, but you should check the code.
The first commit just reverts the current implementation of bind-once, so checking the second commit on it's own can make reviewing easier
Suggested order of review:

  • rootScope.js
  • interpolate.js
  • parse.js
  • The rest

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7700)

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 added a commit to rodyhaddad/angular.js that referenced this pull request Jun 4, 2014
This reverts commit cee429f.

See angular#7700 for a more performant approach for bind-once.
@lgalfaso
Copy link
Contributor

lgalfaso commented Jun 5, 2014

I would like to know where is that we are spending this extra cycles in the current implementation (without thinking on one-time binding). Trying to tackle both things at the same time makes it a lot harder to understand the problem and what this patch is trying to solve

@caitp caitp self-assigned this Jun 9, 2014
@IgorMinar IgorMinar added this to the 1.3.0-beta.12 milestone Jun 9, 2014
cache[exp] = parsedExpression;
}
if (oneTime) parsedExpression.$$beWatched = oneTimeWatch;
if (parsedExpression.constant) parsedExpression.$$beWatched = constantWatch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (parsedExpression.constant) parsedExpression.$$beWatched = constantWatch;
else if (oneTime) parsedExpression.$$beWatched = oneTimeWatch;

saves a comparison for the majority of cases

IgorMinar referenced this pull request Jun 26, 2014
…pproach

This change undoes the use of watchGroup by code that uses $interpolate, by
moving the optimizations into the $interpolate itself. While this is not ideal,
it means that we are making the existing api faster rather than require people
to use $interpolate differently in order to benefit from the speed improvements.
rodyhaddad added a commit to rodyhaddad/angular.js that referenced this pull request Jun 30, 2014
This reverts commit cee429f.

See angular#7700 for a more performant approach for bind-once.
@caitp
Copy link
Contributor

caitp commented Jul 1, 2014

#8029 is fixed by this (if the build weren't broken, I mean).

Here's a bonus test case you can add to prevent that from being broken similarly in the future,

it('should NOT insert null option when blurred without selection with bind-once options', function() {
  scope.values = [{name: 'A'}, {name: 'B'}, {name: 'C'}];
  scope.selected = scope.values[0];
  createSelect({
    'ng-model': 'selected',
    'ng-options': 'value.name for value in ::values'
  });

  browserTrigger(element, 'blur');

  expect(element.children('option').length).toBe(3);
});

I guess the real test is more like this:

it('should use exact same values as values in scope with one-time bindings', function() {
  scope.values = [{name: 'A'}, {name: 'B'}];
  scope.selected = scope.values[0];
  createSelect({
    'ng-model': 'selected',
    'ng-options': 'value.name for value in ::values'
  });

  browserTrigger(element.find('option').eq(1));

  expect(scope.selected).toBe(scope.values[1]);
});

return (value || '').toString();
}
var parsed = $parse(attr.ngBindHtml),
changeDetector = $parse(attr.ngBindHtml, function getStringValue(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just fyi that we prefer multiple vars in new code as that makes refactoring easier.


forEach(watchExpressions, function (expr, i) {
var exprFn = $parse(expr);
deregisterFns.push(self.$watch(exprFn, function (value, oldValue) {
var unwatch = self.$watch(expr, function wgExpression(value, oldValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"wgExpression" -> "watchGroupSubAction" ?

rodyhaddad added a commit to rodyhaddad/angular.js that referenced this pull request Jul 7, 2014
This reverts commit cee429f.

See angular#7700 for a more performant approach for bind-once.
@Narretz
Copy link
Contributor

Narretz commented Jul 8, 2014

Will this make it to the next release? If not, can we add the support for one-time binding of object literals (ng-class etc.) to the current implementation? I know it was in this PR, but I can't seem to find it atm.

@rodyhaddad
Copy link
Contributor Author

@Narretz It was in a commit on my fork (rodyhaddad@c9ee324), and it was for #7663

@Narretz
Copy link
Contributor

Narretz commented Jul 9, 2014

@rodyhaddad So once this is merged, the other change can be merged, too, right?

@ealtenho ealtenho modified the milestones: 1.3.0-beta.16, 1.3.0-beta.15 Jul 11, 2014
@rodyhaddad rodyhaddad assigned IgorMinar and unassigned caitp Jul 14, 2014
rodyhaddad added a commit to rodyhaddad/angular.js that referenced this pull request Jul 15, 2014
This reverts commit cee429f.

See angular#7700 for a more performant approach for bind-once.
This reverts commit cee429f.

See angular#7700 for a more performant approach for bind-once.
BEAKING CHANGE:
Lazy-binding now happens on the scope watcher level.

What this means is that given `parseFn = $parse('::foo')`,
bind-once will only kick in when `parseFn` is being watched by a scope
(i.e. `scope.$watch(parseFn)`)

Bind-once will have no effect when directily invoking `parseFn` (i.e. `parseFn()`)
@rodyhaddad rodyhaddad merged commit 86d55c1 into angular:master Jul 15, 2014
rodyhaddad added a commit that referenced this pull request Jul 15, 2014
ckknight pushed a commit to ckknight/angular.js that referenced this pull request Jul 16, 2014
This reverts commit cee429f.

See angular#7700 for a more performant approach for bind-once.
ckknight pushed a commit to ckknight/angular.js that referenced this pull request Jul 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants