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

Commit

Permalink
perf($parse): execute watched expressions only when the inputs change
Browse files Browse the repository at this point in the history 
With this change, expressions like "firstName + ' ' + lastName | uppercase"
will be analyzed and only the inputs for the expression will be watched
(in this case "firstName" and "lastName"). Only when at least one of the inputs
change, the expression will be evaluated.

This change speeds up simple expressions like `firstName | noop` by ~15%
and more complex expressions like `startDate | date` by ~2500%.

BREAKING CHANGE: all filters are assumed to be stateless functions

Previously it was a good practice to make all filters stateless, but now
it's a requirement in order for the model change-observation to pick up
all changes.

If an existing filter is statefull, it can be flagged as such but keep in
mind that this will result in a significant performance-penalty (or rather
lost opportunity to benefit from a major perf improvement) that will
affect the $digest duration.

To flag a filter as stateful do the following:

myApp.filter('myFilter', function() {
  function myFilter(input) { ... };
  myFilter.$stateful = true;
  return myFilter;
});

Closes #9006
Closes #9082
  • Loading branch information
@jbedard @IgorMinar
jbedard authored and IgorMinar committed Sep 16, 2014
1 parent ec9c0d7 commit fca6be7
Show file tree
Hide file tree
Showing 6 changed files with 386 additions and 24 deletions.
16 changes: 16 additions & 0 deletions benchmarks/parsed-expressions-bp/main.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@
<label for="operators">Binary/Unary operators</label>
</li>

<li>
<input type="radio" ng-model="expressionType" value="shortCircuitingOperators" id="shortCircuitingOperators">
<label for="shortCircuitingOperators">AND/OR short-circuiting operators</label>
</li>

<li>
<input type="radio" ng-model="expressionType" value="filters" id="filters">
<label for="filters">Filters</label>
Expand Down Expand Up @@ -134,6 +139,17 @@
<span bm-pe-watch="-rowIdx * 2 * rowIdx + rowIdx / rowIdx + 1"></span>
</li>

<li ng-switch-when="shortCircuitingOperators" ng-repeat="(rowIdx, row) in ::data">
<span bm-pe-watch="rowIdx && row.odd"></span>
<span bm-pe-watch="row.odd && row.even"></span>
<span bm-pe-watch="row.odd && !row.even"></span>
<span bm-pe-watch="row.odd || row.even"></span>
<span bm-pe-watch="row.odd || row.even || row.index"></span>
<span bm-pe-watch="row.index === 1 || row.index === 2"></span>
<span bm-pe-watch="row.num0 < row.num1 && row.num1 < row.num2"></span>
<span bm-pe-watch="row.num0 < row.num1 || row.num1 < row.num2"></span>
</li>

<li ng-switch-when="filters" ng-repeat="(rowIdx, row) in ::data">
<span bm-pe-watch="rowIdx | noop"></span>
<span bm-pe-watch="rowIdx | noop"></span>
Expand Down
6 changes: 4 additions & 2 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1725,7 +1725,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
attrs[attrName], newIsolateScopeDirective.name);
};
lastValue = isolateBindingContext[scopeName] = parentGet(scope);
var unwatch = scope.$watch($parse(attrs[attrName], function parentValueWatch(parentValue) {
var parentValueWatch = function parentValueWatch(parentValue) {
if (!compare(parentValue, isolateBindingContext[scopeName])) {
// we are out of sync and need to copy
if (!compare(parentValue, lastValue)) {
Expand All @@ -1737,7 +1737,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
}
return lastValue = parentValue;
}), null, parentGet.literal);
};
parentValueWatch.$stateful = true;
var unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal);
isolateScope.$on('$destroy', unwatch);
break;

Expand Down
142 changes: 122 additions & 20 deletions src/ng/parse.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,10 @@ Lexer.prototype = {
};


function isConstant(exp) {
return exp.constant;
}

/**
* @constructor
*/
Expand Down Expand Up @@ -493,7 +497,8 @@ Parser.prototype = {
return extend(function(self, locals) {
return fn(self, locals, right);
}, {
constant:right.constant
constant:right.constant,
inputs: [right]
});
},

Expand All @@ -505,11 +510,12 @@ Parser.prototype = {
});
},

binaryFn: function(left, fn, right) {
binaryFn: function(left, fn, right, isBranching) {
return extend(function(self, locals) {
return fn(self, locals, left, right);
}, {
constant:left.constant && right.constant
constant: left.constant && right.constant,
inputs: !isBranching && [left, right]
});
},

Expand Down Expand Up @@ -557,7 +563,9 @@ Parser.prototype = {
}
}

return function $parseFilter(self, locals) {
var inputs = [inputFn].concat(argsFn || []);

return extend(function $parseFilter(self, locals) {
var input = inputFn(self, locals);
if (args) {
args[0] = input;
Expand All @@ -571,7 +579,10 @@ Parser.prototype = {
}

return fn(input);
};
}, {
constant: !fn.$stateful && inputs.every(isConstant),
inputs: !fn.$stateful && inputs
});
},

expression: function() {
Expand All @@ -588,9 +599,11 @@ Parser.prototype = {
this.text.substring(0, token.index) + '] can not be assigned to', token);
}
right = this.ternary();
return function $parseAssignment(scope, locals) {
return extend(function $parseAssignment(scope, locals) {
return left.assign(scope, right(scope, locals), locals);
};
}, {
inputs: [left, right]
});
}
return left;
},
Expand All @@ -615,7 +628,7 @@ Parser.prototype = {
var left = this.logicalAND();
var token;
while ((token = this.expect('||'))) {
left = this.binaryFn(left, token.fn, this.logicalAND());
left = this.binaryFn(left, token.fn, this.logicalAND(), true);
}
return left;
},
Expand All @@ -624,7 +637,7 @@ Parser.prototype = {
var left = this.equality();
var token;
if ((token = this.expect('&&'))) {
left = this.binaryFn(left, token.fn, this.logicalAND());
left = this.binaryFn(left, token.fn, this.logicalAND(), true);
}
return left;
},
Expand Down Expand Up @@ -759,7 +772,6 @@ Parser.prototype = {
// This is used with json array declaration
arrayDeclaration: function () {
var elementFns = [];
var allConstant = true;
if (this.peekToken().text !== ']') {
do {
if (this.peek(']')) {
Expand All @@ -768,9 +780,6 @@ Parser.prototype = {
}
var elementFn = this.expression();
elementFns.push(elementFn);
if (!elementFn.constant) {
allConstant = false;
}
} while (this.expect(','));
}
this.consume(']');
Expand All @@ -783,13 +792,13 @@ Parser.prototype = {
return array;
}, {
literal: true,
constant: allConstant
constant: elementFns.every(isConstant),
inputs: elementFns
});
},

object: function () {
var keys = [], valueFns = [];
var allConstant = true;
if (this.peekToken().text !== '}') {
do {
if (this.peek('}')) {
Expand All @@ -801,9 +810,6 @@ Parser.prototype = {
this.consume(':');
var value = this.expression();
valueFns.push(value);
if (!value.constant) {
allConstant = false;
}
} while (this.expect(','));
}
this.consume('}');
Expand All @@ -816,7 +822,8 @@ Parser.prototype = {
return object;
}, {
literal: true,
constant: allConstant
constant: valueFns.every(isConstant),
inputs: valueFns
});
}
};
Expand Down Expand Up @@ -1043,6 +1050,8 @@ function $ParseProvider() {
parsedExpression = wrapSharedExpression(parsedExpression);
parsedExpression.$$watchDelegate = parsedExpression.literal ?
oneTimeLiteralWatchDelegate : oneTimeWatchDelegate;
} else if (parsedExpression.inputs) {
parsedExpression.$$watchDelegate = inputsWatchDelegate;
}

cache[cacheKey] = parsedExpression;
Expand All @@ -1057,6 +1066,88 @@ function $ParseProvider() {
}
};

function collectExpressionInputs(inputs, list) {
for (var i = 0, ii = inputs.length; i < ii; i++) {
var input = inputs[i];
if (!input.constant) {
if (input.inputs) {
collectExpressionInputs(input.inputs, list);
} else if (list.indexOf(input) === -1) { // TODO(perf) can we do better?
list.push(input);
}
}
}

return list;
}

function expressionInputDirtyCheck(newValue, oldValueOfValue) {

if (newValue == null || oldValueOfValue == null) { // null/undefined
return newValue === oldValueOfValue;
}

if (typeof newValue === 'object') {

// attempt to convert the value to a primitive type
// TODO(docs): add a note to docs that by implementing valueOf even objects and arrays can
// be cheaply dirty-checked
newValue = newValue.valueOf();

if (typeof newValue === 'object') {
// objects/arrays are not supported - deep-watching them would be too expensive
return false;
}

// fall-through to the primitive equality check
}

//Primitive or NaN
return newValue === oldValueOfValue || (newValue !== newValue && oldValueOfValue !== oldValueOfValue);
}

function inputsWatchDelegate(scope, listener, objectEquality, parsedExpression) {
var inputExpressions = parsedExpression.$$inputs ||
(parsedExpression.$$inputs = collectExpressionInputs(parsedExpression.inputs, []));

var lastResult;

if (inputExpressions.length === 1) {
var oldInputValue = expressionInputDirtyCheck; // init to something unique so that equals check fails
inputExpressions = inputExpressions[0];
return scope.$watch(function expressionInputWatch(scope) {
var newInputValue = inputExpressions(scope);
if (!expressionInputDirtyCheck(newInputValue, oldInputValue)) {
lastResult = parsedExpression(scope);
oldInputValue = newInputValue && newInputValue.valueOf();
}
return lastResult;
}, listener, objectEquality);
}

var oldInputValueOfValues = [];
for (var i = 0, ii = inputExpressions.length; i < ii; i++) {
oldInputValueOfValues[i] = expressionInputDirtyCheck; // init to something unique so that equals check fails
}

return scope.$watch(function expressionInputsWatch(scope) {
var changed = false;

for (var i = 0, ii = inputExpressions.length; i < ii; i++) {
var newInputValue = inputExpressions[i](scope);
if (changed || (changed = !expressionInputDirtyCheck(newInputValue, oldInputValueOfValues[i]))) {
oldInputValueOfValues[i] = newInputValue && newInputValue.valueOf();
}
}

if (changed) {
lastResult = parsedExpression(scope);
}

return lastResult;
}, listener, objectEquality);
}

function oneTimeWatchDelegate(scope, listener, objectEquality, parsedExpression) {
var unwatch, lastValue;
return unwatch = scope.$watch(function oneTimeWatch(scope) {
Expand Down Expand Up @@ -1122,7 +1213,18 @@ function $ParseProvider() {
// initial value is defined (for bind-once)
return isDefined(value) ? result : value;
};
fn.$$watchDelegate = parsedExpression.$$watchDelegate;

// Propagate $$watchDelegates other then inputsWatchDelegate
if (parsedExpression.$$watchDelegate &&
parsedExpression.$$watchDelegate !== inputsWatchDelegate) {
fn.$$watchDelegate = parsedExpression.$$watchDelegate;
} else if (!interceptorFn.$stateful) {
// If there is an interceptor, but no watchDelegate then treat the interceptor like
// we treat filters - it is assumed to be a pure function unless flagged with $stateful
fn.$$watchDelegate = inputsWatchDelegate;
fn.inputs = [parsedExpression];
}

return fn;
}
}];
Expand Down
2 changes: 2 additions & 0 deletions src/ng/rootScope.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,8 @@ function $RootScopeProvider(){
* de-registration function is executed, the internal watch operation is terminated.
*/
$watchCollection: function(obj, listener) {
$watchCollectionInterceptor.$stateful = true;

var self = this;
// the current value, updated on each dirty-check run
var newValue;
Expand Down
10 changes: 8 additions & 2 deletions test/ng/directive/ngRepeatSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,12 +867,15 @@ describe('ngRepeat', function() {
// This creates one item, but it has no parent so we can't get to it
$rootScope.items = [1, 2];
$rootScope.$apply();
expect(logs).toContain(1);
expect(logs).toContain(2);
logs.length = 0;

// This cleans up to prevent memory leak
$rootScope.items = [];
$rootScope.$apply();
expect(angular.mock.dump(element)).toBe('<!-- ngRepeat: i in items -->');
expect(logs).toEqual([1, 2, 1, 2]);
expect(logs.length).toBe(0);
}));


Expand All @@ -894,12 +897,15 @@ describe('ngRepeat', function() {
// This creates one item, but it has no parent so we can't get to it
$rootScope.items = [1, 2];
$rootScope.$apply();
expect(logs).toContain(1);
expect(logs).toContain(2);
logs.length = 0;

// This cleans up to prevent memory leak
$rootScope.items = [];
$rootScope.$apply();
expect(sortedHtml(element)).toBe('<span>-</span><!-- ngRepeat: i in items --><span>-</span>');
expect(logs).toEqual([1, 2, 1, 2]);
expect(logs.length).toBe(0);
}));


Expand Down
Loading

2 comments on commit fca6be7

@MilanLempera
Copy link

Choose a reason for hiding this comment

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

Hello,
if I am correct, this change increases count of expression evaluations when expression value is changed.

For example
with angular 1.3.0-rc1 (http://jsbin.com/yahuge/10/edit?html,js,console,output) - scope method is called twice per $digest
with angular 1.3.0-rc2 (http://jsbin.com/yahuge/11/edit?html,js,console,output) - method is called three times per $digest, and even four times, when returned value is an object (http://jsbin.com/yahuge/12/edit?html,js,console,output)

Is this a wanted behavior?

@IgorMinar
Copy link
Contributor

Choose a reason for hiding this comment

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

yes. it's a minor perf penalty we pay when model changes in order to make cost of observation when nothing changes (much more common) cheap. it's possible to fix this, but that would require a bigger refactoring which we might do in the future when this becomes a bottleneck.

Please sign in to comment.