From a1a9585acd9844657aa700fecdc0d024ef31284b Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 9 Sep 2014 21:42:36 -0700 Subject: [PATCH 1/2] perf($parse): removing binaryFn and valueFn wrappers from filter expressions --- src/ng/parse.js | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/ng/parse.js b/src/ng/parse.js index 69e71e201698..a5d3562695bf 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -112,7 +112,6 @@ var OPERATORS = extend(createMap(), { '/':function(self, locals, a,b){return a(self, locals)/b(self, locals);}, '%':function(self, locals, a,b){return a(self, locals)%b(self, locals);}, '^':function(self, locals, a,b){return a(self, locals)^b(self, locals);}, - '=':noop, '===':function(self, locals, a, b){return a(self, locals)===b(self, locals);}, '!==':function(self, locals, a, b){return a(self, locals)!==b(self, locals);}, '==':function(self, locals, a,b){return a(self, locals)==b(self, locals);}, @@ -125,8 +124,11 @@ var OPERATORS = extend(createMap(), { '||':function(self, locals, a,b){return a(self, locals)||b(self, locals);}, '&':function(self, locals, a,b){return a(self, locals)&b(self, locals);}, // '|':function(self, locals, a,b){return a|b;}, - '|':function(self, locals, a,b){return b(self, locals)(self, locals, a(self, locals));}, - '!':function(self, locals, a){return !a(self, locals);} + '!':function(self, locals, a){return !a(self, locals);}, + + //Tokenized as operators but parsed as assignment/filters + '=':true, + '|':true }); /* jshint bitwise: true */ var ESCAPE = {"n":"\n", "f":"\f", "r":"\r", "t":"\t", "v":"\v", "'":"'", '"':'"'}; @@ -537,12 +539,12 @@ Parser.prototype = { var left = this.expression(); var token; while ((token = this.expect('|'))) { - left = this.binaryFn(left, token.fn, this.filter()); + left = this.filter(left); } return left; }, - filter: function() { + filter: function(inputFn) { var token = this.expect(); var fn = this.$filter(token.text); var argsFn; @@ -556,7 +558,8 @@ Parser.prototype = { } } - return valueFn(function $parseFilter(self, locals, input) { + return function $parseFilter(self, locals) { + var input = inputFn(self, locals); if (args) { args[0] = input; @@ -569,7 +572,7 @@ Parser.prototype = { } return fn(input); - }); + }; }, expression: function() { @@ -786,7 +789,7 @@ Parser.prototype = { }, object: function () { - var keyValues = []; + var keys = [], values = []; var allConstant = true; if (this.peekToken().text !== '}') { do { @@ -794,11 +797,11 @@ Parser.prototype = { // Support trailing commas per ES5.1. break; } - var token = this.expect(), - key = token.string || token.text; + var token = this.expect(); + keys.push(token.string || token.text); this.consume(':'); var value = this.expression(); - keyValues.push({key: key, value: value}); + values.push(value); if (!value.constant) { allConstant = false; } @@ -808,9 +811,8 @@ Parser.prototype = { return extend(function $parseObjectLiteral(self, locals) { var object = {}; - for (var i = 0, ii = keyValues.length; i < ii; i++) { - var keyValue = keyValues[i]; - object[keyValue.key] = keyValue.value(self, locals); + for (var i = 0, ii = values.length; i < ii; i++) { + object[keys[i]] = values[i](self, locals); } return object; }, { From c6a6dd91ba5d56925e73e43b682cf44c193769b0 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Tue, 9 Sep 2014 22:03:10 -0700 Subject: [PATCH 2/2] WIP: perf($parse): only execute watched expressions when the inputs change --- benchmarks/parsed-expressions-bp/main.html | 16 +++ src/ng/compile.js | 6 +- src/ng/parse.js | 137 ++++++++++++++++--- src/ng/rootScope.js | 2 + test/ng/directive/ngRepeatSpec.js | 10 +- test/ng/parseSpec.js | 147 +++++++++++++++++++++ 6 files changed, 293 insertions(+), 25 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 f0d3fe197bfd..b6e813fea17b 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1699,7 +1699,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)) { @@ -1711,7 +1711,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } } return lastValue = parentValue; - }), null, parentGet.literal); + }; + parentValueWatch.externalInput = 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 a5d3562695bf..ff34a1120689 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -377,6 +377,10 @@ Lexer.prototype = { }; +function isConstant(exp) { + return exp.constant; +} + /** * @constructor */ @@ -494,7 +498,8 @@ Parser.prototype = { return extend(function(self, locals) { return fn(self, locals, right); }, { - constant:right.constant + constant:right.constant, + inputs: [right] }); }, @@ -502,15 +507,16 @@ Parser.prototype = { return extend(function(self, locals){ return left(self, locals) ? middle(self, locals) : right(self, locals); }, { - constant: left.constant && middle.constant && right.constant + constant:left.constant && middle.constant && right.constant }); }, - 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] }); }, @@ -558,7 +564,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; @@ -572,7 +580,10 @@ Parser.prototype = { } return fn(input); - }; + }, { + constant: !fn.externalInput && inputs.every(isConstant), + inputs: !fn.externalInput && inputs + }); }, expression: function() { @@ -589,9 +600,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; }, @@ -616,7 +629,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; }, @@ -625,7 +638,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; }, @@ -760,7 +773,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(']')) { @@ -769,9 +781,6 @@ Parser.prototype = { } var elementFn = this.expression(); elementFns.push(elementFn); - if (!elementFn.constant) { - allConstant = false; - } } while (this.expect(',')); } this.consume(']'); @@ -784,13 +793,13 @@ Parser.prototype = { return array; }, { literal: true, - constant: allConstant + constant: elementFns.every(isConstant), + inputs: elementFns }); }, object: function () { var keys = [], values = []; - var allConstant = true; if (this.peekToken().text !== '}') { do { if (this.peek('}')) { @@ -802,9 +811,6 @@ Parser.prototype = { this.consume(':'); var value = this.expression(); values.push(value); - if (!value.constant) { - allConstant = false; - } } while (this.expect(',')); } this.consume('}'); @@ -817,7 +823,8 @@ Parser.prototype = { return object; }, { literal: true, - constant: allConstant + constant: values.every(isConstant), + inputs: values }); } }; @@ -1045,6 +1052,9 @@ function $ParseProvider() { parsedExpression.$$watchDelegate = parsedExpression.literal ? oneTimeLiteralWatchDelegate : oneTimeWatchDelegate; } + else if (parsedExpression.inputs) { + parsedExpression.$$watchDelegate = inputsWatchDelegate; + } cache[cacheKey] = parsedExpression; } @@ -1058,6 +1068,81 @@ 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 (-1 === list.indexOf(input)) { + list.push(input); + } + } + } + + return list; + } + + function simpleEquals(o1, o2) { + if (o1 == null || o2 == null) return o1 === o2; // null/undefined + + if (typeof o1 === "object") { + //The same object is not supported because it may have been mutated + if (o1 === o2) return false; + + if (typeof o2 !== "object") return false; + + //Convert to primitive if possible + o1 = o1.valueOf(); + o2 = o2.valueOf(); + + //If the type became a non-object then we can use the primitive check below + if (typeof o1 === "object") return false; + } + + //Primitive or NaN + return o1 === o2 || (o1 !== o1 && o2 !== o2); + } + + function inputsWatchDelegate(scope, listener, objectEquality, parsedExpression) { + var inputExpressions = parsedExpression.$$inputs || + (parsedExpression.$$inputs = collectExpressionInputs(parsedExpression.inputs, [])); + + var inputs = [simpleEquals/*=something that will never equal an evaluated input*/]; + var lastResult; + + if (1 === inputExpressions.length) { + inputs = inputs[0]; + inputExpressions = inputExpressions[0]; + return scope.$watch(function expressionInputWatch(scope) { + var newVal = inputExpressions(scope); + if (!simpleEquals(newVal, inputs)) { + lastResult = parsedExpression(scope); + inputs = newVal; + } + return lastResult; + }, listener, objectEquality); + } + + return scope.$watch(function expressionInputsWatch(scope) { + var changed = false; + + for (var i=0, ii=inputExpressions.length; i'); - 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 618149cee2fe..03eac03e1119 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -1311,6 +1311,153 @@ 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 externalInput', function() { + var filterCalled = false; + $filterProvider.register('foo', valueFn(extend(function(input) { + filterCalled = true; + return input; + }, {externalInput: 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 invoke interceptorFns if the yare marked as having externalInput', inject(function($parse) { + var called = false; + function interceptor() { + called = true; + } + interceptor.externalInput = 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); + })); + it('should treat constants inputted to filters 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 not treat constants passed to filters with externalInput as constants', inject(function($parse) { + + })); + }); + describe('locals', function() { it('should expose local variables', inject(function($parse) { expect($parse('a')({a: 0}, {a: 1})).toEqual(1);