From 33de6093a8a1799f306439259b3d9802a9b6feb4 Mon Sep 17 00:00:00 2001 From: rodyhaddad Date: Sun, 20 Oct 2013 02:11:28 -0400 Subject: [PATCH 1/5] refactor($interpolate): optimize watched $interpolate functions perf --- src/ng/interpolate.js | 104 ++++++++++++++++++++++++++++--------- src/ng/rootScope.js | 3 ++ test/ng/interpolateSpec.js | 50 +++++++++++++++++- 3 files changed, 130 insertions(+), 27 deletions(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index b78f44a4c9c5..a193fc1b29a8 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -127,12 +127,11 @@ function $InterpolateProvider() { var startIndex, endIndex, index = 0, - parts = [], length = text.length, hasInterpolation = false, - fn, + fn = null, exp, - concat = []; + parts = []; while(index < length) { if ( ((startIndex = text.indexOf(startSymbol, index)) != -1) && @@ -149,10 +148,9 @@ function $InterpolateProvider() { } } - if (!(length = parts.length)) { + if (!parts.length) { // we added, nothing, must have been an empty string. parts.push(''); - length = 1; } // Concatenating expressions makes it hard to reason about whether some combination of @@ -169,25 +167,29 @@ function $InterpolateProvider() { } if (!mustHaveExpression || hasInterpolation) { - concat.length = length; - fn = function(context) { + var concat = new Array(parts.length), + expressions = {}; + forEach(parts, function (value, index) { + if (isFunction(value)) { + expressions[index] = value; + concat[index] = ''; + } else { + concat[index] = value; + } + }); + // computes all the interpolations and returns the resulting string + // a specific index might already be computed (cz of the scope's dirty-checking), + // and so its expression shouldn't be executed a 2nd time + // also populates the lastValues of custom watchers for internal dirty-checking + var getTextValue = function (scope, computedIndex, computedValue, lastValues) { try { - for(var i = 0, ii = length, part; i Date: Thu, 19 Dec 2013 17:05:34 -0500 Subject: [PATCH 2/5] test($interpolate): add a test to address Igor's first concern which is point #1 in this PR comment https://github.com/angular/angular.js/pull/4556#issuecomment-30961742 also merged Igor's first code review https://github.com/IgorMinar/angular.js/commit/784dd4e9ffc52c834b698ab3d8b212a959ea3fe1 --- src/ng/interpolate.js | 31 ++++++++++++++++------------ src/ng/rootScope.js | 2 +- test/ng/interpolateSpec.js | 41 ++++++++++++++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index a193fc1b29a8..70ab66dac916 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -169,7 +169,8 @@ function $InterpolateProvider() { if (!mustHaveExpression || hasInterpolation) { var concat = new Array(parts.length), expressions = {}; - forEach(parts, function (value, index) { + + forEach(parts, function(value, index) { if (isFunction(value)) { expressions[index] = value; concat[index] = ''; @@ -177,49 +178,53 @@ function $InterpolateProvider() { concat[index] = value; } }); + // computes all the interpolations and returns the resulting string - // a specific index might already be computed (cz of the scope's dirty-checking), + // a specific index might already be computed (thanks to the scope's dirty-checking), // and so its expression shouldn't be executed a 2nd time // also populates the lastValues of custom watchers for internal dirty-checking - var getTextValue = function (scope, computedIndex, computedValue, lastValues) { + var getConcatValue = function(scope, computedIndex, computedValue, lastValues) { try { - forEach(expressions, function (expression, index) { - concat[index] = index == computedIndex + + forEach(expressions, function(expression, index) { + concat[index] = (index === computedIndex) ? computedValue : getStringValue(expression(scope)); if (lastValues) lastValues[index] = concat[index]; }); return concat.join(''); - } - catch(err) { + + } catch(err) { var newErr = $interpolateMinErr('interr', "Can't interpolate: {0}\n{1}", text, err.toString()); $exceptionHandler(newErr); } }; - var getStringValue = function (value) { + + var getStringValue = function(value) { value = trustedContext ? $sce.getTrusted(trustedContext, value) : $sce.valueOf(value); - if (value === null || isUndefined(value)) { + if (value == null) { return ''; } return isString(value) ? value : toJson(value); }; fn = function(scope) { - return getTextValue(scope); + // we don't want others to be able to pass more than the first argument + return getConcatValue(scope); }; fn.exp = text; fn.parts = parts; // watches each interpolation separately for performance - fn.$$beWatched = function (scope, origListener, objectEquality) { + fn.$$beWatched = function(scope, origListener, objectEquality) { var lastTextValue, lastValues = {}, watchersRm = []; - forEach(expressions, function (expression, index) { + forEach(expressions, function(expression, index) { watchersRm.push(scope.$watch(function watchInterpolatedExpr(scope) { try { return getStringValue(expression(scope)); @@ -241,7 +246,7 @@ function $InterpolateProvider() { // and ignore it when the listener of `b` gets triggered // (unless the value of `b` changes again since the last computation) if (value !== lastValues[index]) { - var textValue = getTextValue(scope, index, value, lastValues); + var textValue = getConcatValue(scope, index, value, lastValues); origListener.call(this, textValue, value === oldValue ? textValue : lastTextValue, scope); lastTextValue = textValue; diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index a975bc99081f..f771bce62246 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -315,7 +315,7 @@ function $RootScopeProvider(){ * @returns {function()} Returns a deregistration function for this listener. */ $watch: function(watchExp, listener, objectEquality) { - if (isFunction(watchExp.$$beWatched)) { + if (watchExp.$$beWatched) { return watchExp.$$beWatched(this, listener, objectEquality, watchExp); } var scope = this, diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js index 30a75903ea63..5512c2cc302b 100644 --- a/test/ng/interpolateSpec.js +++ b/test/ng/interpolateSpec.js @@ -2,13 +2,12 @@ describe('$interpolate', function() { - it('should return a function when there are no bindings and textOnly is undefined', + it('should return a function when there are no bindings and mustHaveExpression is undefined', inject(function($interpolate) { expect(typeof $interpolate('some text')).toBe('function'); })); - - it('should return undefined when there are no bindings and textOnly is set to true', + it('should return null when there are no bindings and mustHaveExpression is set to true', inject(function($interpolate) { expect($interpolate('some text', true)).toBeNull(); })); @@ -318,6 +317,40 @@ describe('$interpolate', function() { }); expect(nbCalls).toBe(4); })); - }) + it('should call the listener only once per whole text change', inject(function ($rootScope, $interpolate) { + var nbCalls = 0, value; + $rootScope.$watch($interpolate("{{a}}-{{b}}"), function (_value){ + nbCalls++; + value = _value; + }); + + $rootScope.$apply(); + expect(nbCalls).toBe(1); + expect(value).toBe('-'); + + $rootScope.$apply('a=1;b=1'); + expect(nbCalls).toBe(2); + expect(value).toBe('1-1'); + + // one changes + $rootScope.$apply('a=2'); + expect(nbCalls).toBe(3); + expect(value).toBe('2-1'); + + $rootScope.$apply('b=2'); + expect(nbCalls).toBe(4); + expect(value).toBe('2-2'); + + // both change + $rootScope.$apply('a=3;b=3'); + expect(nbCalls).toBe(5); + expect(value).toBe('3-3'); + + // nothing changes + $rootScope.$apply(); + expect(nbCalls).toBe(5); + expect(value).toBe('3-3'); + })); + }); }); From 8386ff6e0c453fb8f2e58fb1f2a03798d3a4ea78 Mon Sep 17 00:00:00 2001 From: rodyhaddad Date: Fri, 10 Jan 2014 07:56:20 -0500 Subject: [PATCH 3/5] refactor($interpolation): small attempt to add clarity --- src/ng/interpolate.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index 70ab66dac916..b5fac09cee91 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -173,7 +173,6 @@ function $InterpolateProvider() { forEach(parts, function(value, index) { if (isFunction(value)) { expressions[index] = value; - concat[index] = ''; } else { concat[index] = value; } @@ -214,8 +213,7 @@ function $InterpolateProvider() { }; fn = function(scope) { - // we don't want others to be able to pass more than the first argument - return getConcatValue(scope); + return getConcatValue(scope /*, undefined, ...*/); }; fn.exp = text; fn.parts = parts; @@ -225,7 +223,12 @@ function $InterpolateProvider() { var lastTextValue, lastValues = {}, watchersRm = []; forEach(expressions, function(expression, index) { - watchersRm.push(scope.$watch(function watchInterpolatedExpr(scope) { + watchersRm.push( + scope.$watch(watcherOf(expression), listenerOf(index), objectEquality)); + }); + + function watcherOf(expression) { + return function interpolatedExprWatcher(scope) { try { return getStringValue(expression(scope)); } catch (err) { @@ -233,18 +236,17 @@ function $InterpolateProvider() { text, err.toString()); $exceptionHandler(newErr); } - }, listenerOf(index), objectEquality)); - }); + } + } function listenerOf(index) { return function interpolatedExprListener(value, oldValue) { // we only invoke the origListener if the current value // is not equal to the last computed value - // ex: if in `{{a}}-{{b}}` both values change in a digest, - // the listener of `a` gets invoked first, we compute the string - // and invoke the origListener once, - // and ignore it when the listener of `b` gets triggered - // (unless the value of `b` changes again since the last computation) + // ex: if in `{{a}}-{{b}}` both values change in a digest, the listener of + // `a` gets invoked first, we compute the string and call the origListener + // and don't invoke it again when the listener of `b` gets triggered + // (unless the value of `b` changes again since the last computation!) if (value !== lastValues[index]) { var textValue = getConcatValue(scope, index, value, lastValues); origListener.call(this, textValue, From 5be8ed78a16158c61f583b360c8194073a411176 Mon Sep 17 00:00:00 2001 From: rodyhaddad Date: Wed, 22 Jan 2014 01:07:42 -0500 Subject: [PATCH 4/5] refactor(interpolateSpec): rename nbCalls to count and make the first test a bit stricter --- test/ng/interpolateSpec.js | 52 ++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/test/ng/interpolateSpec.js b/test/ng/interpolateSpec.js index 5512c2cc302b..a02201fd2c98 100644 --- a/test/ng/interpolateSpec.js +++ b/test/ng/interpolateSpec.js @@ -276,80 +276,76 @@ describe('$interpolate', function() { describe('custom $$beWatched', function () { it('should call the listener correctly when values change during digest', inject(function ($rootScope, $interpolate) { - var nbCalls = 0, value; + var count = 0, value; $rootScope.$watch($interpolate('{{a}}-{{b}}'), function (_value) { - value = _value; - switch(++nbCalls) { + switch(++count) { case 1: case 2: - $rootScope.b++; + $rootScope.a++; break; case 3: case 4: - $rootScope.a++; + $rootScope.b++; + break; + case 5: + value = _value; break; } }); - $rootScope.$apply(function () { - $rootScope.a = $rootScope.b = 0; - }); + $rootScope.$apply("a=0;b=0"); expect(value).toBe("2-2"); - expect(nbCalls).toBe(5); + expect(count).toBe(5); })); it('should call the listener correctly when the interpolation is watched multiple times', inject(function ($rootScope, $interpolate) { - var interpolateFn = $interpolate('{{a}}-{{b}}'), nbCalls = 0; + var interpolateFn = $interpolate('{{a}}-{{b}}'), count = 0; $rootScope.$watch(interpolateFn, function(){ - nbCalls++; + count++; }); $rootScope.$watch(interpolateFn, function(){ - nbCalls++; + count++; }); - $rootScope.$apply(function () { - $rootScope.a = $rootScope.b = 0; - }); - expect(nbCalls).toBe(2); + $rootScope.$apply("a=0;b=0"); + expect(count).toBe(2); - $rootScope.$apply(function () { - $rootScope.a = $rootScope.b = 1; - }); - expect(nbCalls).toBe(4); + $rootScope.$apply("a=1;b=1"); + expect(count).toBe(4); })); it('should call the listener only once per whole text change', inject(function ($rootScope, $interpolate) { - var nbCalls = 0, value; + var count = 0, value; $rootScope.$watch($interpolate("{{a}}-{{b}}"), function (_value){ - nbCalls++; + count++; value = _value; }); $rootScope.$apply(); - expect(nbCalls).toBe(1); + expect(count).toBe(1); expect(value).toBe('-'); $rootScope.$apply('a=1;b=1'); - expect(nbCalls).toBe(2); + expect(count).toBe(2); expect(value).toBe('1-1'); // one changes $rootScope.$apply('a=2'); - expect(nbCalls).toBe(3); + expect(count).toBe(3); expect(value).toBe('2-1'); $rootScope.$apply('b=2'); - expect(nbCalls).toBe(4); + expect(count).toBe(4); expect(value).toBe('2-2'); // both change $rootScope.$apply('a=3;b=3'); - expect(nbCalls).toBe(5); + expect(count).toBe(5); expect(value).toBe('3-3'); // nothing changes $rootScope.$apply(); - expect(nbCalls).toBe(5); + expect(count).toBe(5); expect(value).toBe('3-3'); })); }); From bacd185185edfe5ec9731767d13d0aadf3ee45c9 Mon Sep 17 00:00:00 2001 From: rodyhaddad Date: Thu, 23 Jan 2014 13:13:23 -0500 Subject: [PATCH 5/5] chore($interpolate): add missing semicolon for jshint not sure how I missed that --- src/ng/interpolate.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index b5fac09cee91..e0495163b512 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -236,7 +236,7 @@ function $InterpolateProvider() { text, err.toString()); $exceptionHandler(newErr); } - } + }; } function listenerOf(index) {