From fca6be71274e537c7df86ae9e27a3bd1597e9ffa Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 9 Sep 2014 22:03:10 -0700 Subject: [PATCH] perf($parse): execute watched expressions only when the inputs change 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 --- benchmarks/parsed-expressions-bp/main.html | 16 ++ src/ng/compile.js | 6 +- src/ng/parse.js | 142 +++++++++++-- src/ng/rootScope.js | 2 + test/ng/directive/ngRepeatSpec.js | 10 +- test/ng/parseSpec.js | 234 +++++++++++++++++++++ 6 files changed, 386 insertions(+), 24 deletions(-) diff --git a/benchmarks/parsed-expressions-bp/main.html b/benchmarks/parsed-expressions-bp/main.html index d7f65b742ce4..9884da94e6bc 100755 --- a/benchmarks/parsed-expressions-bp/main.html +++ b/benchmarks/parsed-expressions-bp/main.html @@ -31,6 +31,11 @@ +
  • + + +
    • +
  • @@ -134,6 +139,17 @@
    • +
  • + + + + + + + + +
    • +
  • diff --git a/src/ng/compile.js b/src/ng/compile.js index cff8fb38516c..f75cb7393f11 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -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)) { @@ -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; diff --git a/src/ng/parse.js b/src/ng/parse.js index d09cc516a0db..eb6ef739edba 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -376,6 +376,10 @@ Lexer.prototype = { }; +function isConstant(exp) { + return exp.constant; +} + /** * @constructor */ @@ -493,7 +497,8 @@ Parser.prototype = { return extend(function(self, locals) { return fn(self, locals, right); }, { - constant:right.constant + constant:right.constant, + inputs: [right] }); }, @@ -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] }); }, @@ -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; @@ -571,7 +579,10 @@ Parser.prototype = { } return fn(input); - }; + }, { + constant: !fn.$stateful && inputs.every(isConstant), + inputs: !fn.$stateful && inputs + }); }, expression: function() { @@ -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; }, @@ -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; }, @@ -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; }, @@ -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(']')) { @@ -768,9 +780,6 @@ Parser.prototype = { } var elementFn = this.expression(); elementFns.push(elementFn); - if (!elementFn.constant) { - allConstant = false; - } } while (this.expect(',')); } this.consume(']'); @@ -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('}')) { @@ -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('}'); @@ -816,7 +822,8 @@ Parser.prototype = { return object; }, { literal: true, - constant: allConstant + constant: valueFns.every(isConstant), + inputs: valueFns }); } }; @@ -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; @@ -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) { @@ -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; } }]; diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 77cba19968ab..34e69b1bd325 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -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; diff --git a/test/ng/directive/ngRepeatSpec.js b/test/ng/directive/ngRepeatSpec.js index 6ee1cbd06bd8..609d232b6f0f 100644 --- a/test/ng/directive/ngRepeatSpec.js +++ b/test/ng/directive/ngRepeatSpec.js @@ -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(''); - expect(logs).toEqual([1, 2, 1, 2]); + expect(logs.length).toBe(0); })); @@ -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('--'); - expect(logs).toEqual([1, 2, 1, 2]); + expect(logs.length).toBe(0); })); diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index 122422d4e302..b77a76b7ad5a 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -1304,6 +1304,240 @@ describe('parser', function() { }); }); + + describe('watched $parse expressions', function() { + + it('should respect short-circuiting AND if it could have side effects', function() { + var bCalled = 0; + scope.b = function() { bCalled++; }; + + scope.$watch("a && b()"); + scope.$digest(); + scope.$digest(); + expect(bCalled).toBe(0); + + scope.a = true; + scope.$digest(); + expect(bCalled).toBe(1); + scope.$digest(); + expect(bCalled).toBe(2); + }); + + it('should respect short-circuiting OR if it could have side effects', function() { + var bCalled = false; + scope.b = function() { bCalled = true; }; + + scope.$watch("a || b()"); + scope.$digest(); + expect(bCalled).toBe(true); + + bCalled = false; + scope.a = true; + scope.$digest(); + expect(bCalled).toBe(false); + }); + + it('should respect the branching ternary operator if it could have side effects', function() { + var bCalled = false; + scope.b = function() { bCalled = true; }; + + scope.$watch("a ? b() : 1"); + scope.$digest(); + expect(bCalled).toBe(false); + + scope.a = true; + scope.$digest(); + expect(bCalled).toBe(true); + }); + + it('should not invoke filters unless the input/arguments change', function() { + var filterCalled = false; + $filterProvider.register('foo', valueFn(function(input) { + filterCalled = true; + return input; + })); + + scope.$watch("a | foo:b:1"); + scope.a = 0; + scope.$digest(); + expect(filterCalled).toBe(true); + + filterCalled = false; + scope.$digest(); + expect(filterCalled).toBe(false); + + scope.a++; + scope.$digest(); + expect(filterCalled).toBe(true); + }); + + it('should invoke filters if they are marked as having $stateful', function() { + var filterCalled = false; + $filterProvider.register('foo', valueFn(extend(function(input) { + filterCalled = true; + return input; + }, {$stateful: true}))); + + scope.$watch("a | foo:b:1"); + scope.a = 0; + scope.$digest(); + expect(filterCalled).toBe(true); + + filterCalled = false; + scope.$digest(); + expect(filterCalled).toBe(true); + }); + + it('should not invoke interceptorFns unless the input changes', inject(function($parse) { + var called = false; + function interceptor(v) { + called = true; + return v; + } + scope.$watch($parse("a", interceptor)); + scope.a = scope.b = 0; + scope.$digest(); + expect(called).toBe(true); + + called = false; + scope.$digest(); + expect(called).toBe(false); + + scope.a++; + scope.$digest(); + expect(called).toBe(true); + })); + + it('should treat filters with constant input as constants', inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + var parsed = $parse('{x: 1} | foo:1'); + + expect(parsed.constant).toBe(true); + + var watcherCalls = 0; + scope.$watch(parsed, function(input) { + expect(input).toEqual({x:1}); + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + })); + + it("should always reevaluate filters with non-primitive input that doesn't support valueOf()", + inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + var parsed = $parse('obj | foo'); + var obj = scope.obj = {}; + + var watcherCalls = 0; + scope.$watch(parsed, function(input) { + expect(input).toBe(obj); + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(2); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(3); + expect(watcherCalls).toBe(1); + })); + + it("should not reevaluate filters with non-primitive input that does support valueOf()", + inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + var parsed = $parse('date | foo'); + var date = scope.date = new Date(); + + var watcherCalls = 0; + scope.$watch(parsed, function(input) { + expect(input).toBe(date); + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + })); + + it("should reevaluate filters with non-primitive input that does support valueOf() when" + + "valueOf() value changes", inject(function($parse) { + var filterCalls = 0; + $filterProvider.register('foo', valueFn(function(input) { + filterCalls++; + return input; + })); + + var parsed = $parse('date | foo'); + var date = scope.date = new Date(); + + var watcherCalls = 0; + scope.$watch(parsed, function(input) { + expect(input).toBe(date); + watcherCalls++; + }); + + scope.$digest(); + expect(filterCalls).toBe(1); + expect(watcherCalls).toBe(1); + + date.setYear(1901); + + scope.$digest(); + expect(filterCalls).toBe(2); + expect(watcherCalls).toBe(1); + })); + + it('should invoke interceptorFns if they are flagged as having $stateful', + inject(function($parse) { + var called = false; + function interceptor() { + called = true; + } + interceptor.$stateful = true; + + scope.$watch($parse("a", interceptor)); + scope.a = 0; + scope.$digest(); + expect(called).toBe(true); + + called = false; + scope.$digest(); + expect(called).toBe(true); + + scope.a++; + called = false; + scope.$digest(); + expect(called).toBe(true); + })); + }); + describe('locals', function() { it('should expose local variables', inject(function($parse) { expect($parse('a')({a: 0}, {a: 1})).toEqual(1);