From db2c017ed475247382c93a465bb2fd270abcb9c1 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Thu, 30 Mar 2017 13:11:21 +0300 Subject: [PATCH 1/2] fix(scopes): correctly meassure perf impact of one-time expressions Previously, one-time expressions were supposed to not trigger perf timers. There were a couple of issues with the implementation: 1. One-time expressions were not detected as such if there was whitespace before the `::`. 2. It depended on internal AngularJS implementation details in a way that would fail to identify certain usecases (e.g. multiple interpolations in a single attribute value or node text). 3. Time spent on the initial evaluation of the one-time expressions would not be accounted for, but could still impact the duration of the first `$digest`. 4. While it is not common, there are cases were one-time expressions don't settle immediately. In such cases, their impact on the `$digest` should not be ignored. This commit fixes the above issues by ensuring that only the "final" call to `$watch` is intercepted (in contrast to intermediate calls that might call a `$$watchDelegate`, which will then create the actual `$watch`er). One-time expressions are intercepted normally and stop triggering perf times as soon as their `unwatch` function is called. Although the dependency on internal AngularJS implementation details could not be avoided, the new implementation is a little more robust, as it relies on fewer and less brittle details :D Fixes #109 Closes #122 --- src/modules/scopes.js | 103 ++++++++++++++------------------------- test/scopes.spec.js | 109 +++++++++++++++++++++++++++++++----------- 2 files changed, 119 insertions(+), 93 deletions(-) diff --git a/src/modules/scopes.js b/src/modules/scopes.js index 0cce42ac..7d4efcdc 100644 --- a/src/modules/scopes.js +++ b/src/modules/scopes.js @@ -79,37 +79,20 @@ function decorateRootScope($delegate, $parse) { var _digestEvents = []; var skipNextPerfWatchers = false; scopePrototype.$watch = function (watchExpression, reactionFunction) { - // if `skipNextPerfWatchers` is true, this means the previous run of the - // `$watch` decorator was a one time binding expression and this invocation - // of the $watch function has the `oneTimeInterceptedExpression` (internal angular function) - // as the `watchExpression` parameter. If we decorate it with the performance - // timers function this will cause us to invoke `oneTimeInterceptedExpression` - // on subsequent digest loops and will update the one time bindings - // if anything mutated the property. - if (skipNextPerfWatchers) { - skipNextPerfWatchers = false; - return _watch.apply(this, arguments); - } - - if (typeof watchExpression === 'string' && - isOneTimeBindExp(watchExpression)) { - skipNextPerfWatchers = true; - return _watch.apply(this, arguments); - } - var watchStr = humanReadableWatchExpression(watchExpression); - var scopeId = this.$id; - var expressions = null; - if (typeof watchExpression === 'function') { - expressions = watchExpression.expressions; - if (Object.prototype.toString.call(expressions) === '[object Array]' && - expressions.some(isOneTimeBindExp)) { - skipNextPerfWatchers = true; - return _watch.apply(this, arguments); - } - - arguments[0] = function () { + // Convert the `watchExpression` to a function (if not already one). + // This is also the first thing `Scope.$watch()` does. + var parsedExpression = $parse(watchExpression); + + // Only intercept this call if there is no `$$watchDelegate`. + // (With `$$watchDelegate` there will be subsequent calls to `$watch` (if necessary)). + if (!parsedExpression.$$watchDelegate) { + var scopeId = this.$id; + var watchStr = humanReadableWatchExpression(watchExpression); + + // Intercept the `watchExpression` (if any). + arguments[0] = simpleExtend(function() { var start = perf.now(); - var ret = watchExpression.apply(this, arguments); + var ret = parsedExpression.apply(this, arguments); var end = perf.now(); _digestEvents.push({ eventType: 'scope:watch', @@ -118,36 +101,23 @@ function decorateRootScope($delegate, $parse) { time: end - start }); return ret; - }; - } else { - var thatScope = this; - arguments[0] = function () { - var start = perf.now(); - var ret = thatScope.$eval(watchExpression); - var end = perf.now(); - _digestEvents.push({ - eventType: 'scope:watch', - id: scopeId, - watch: watchStr, - time: end - start - }); - return ret; - }; - } - - if (typeof reactionFunction === 'function') { - arguments[1] = function () { - var start = perf.now(); - var ret = reactionFunction.apply(this, arguments); - var end = perf.now(); - _digestEvents.push({ - eventType: 'scope:reaction', - id: scopeId, - watch: watchStr, - time: end - start - }); - return ret; - }; + }, parsedExpression); + + // Intercept the `reactionFunction` (if any). + if (typeof reactionFunction === 'function') { + arguments[1] = function() { + var start = perf.now(); + var ret = reactionFunction.apply(this, arguments); + var end = perf.now(); + _digestEvents.push({ + eventType: 'scope:reaction', + id: scopeId, + watch: watchStr, + time: end - start + }); + return ret; + }; + } } return _watch.apply(this, arguments); @@ -355,12 +325,13 @@ function humanReadableWatchExpression (fn) { return fn.toString(); } -function isOneTimeBindExp(exp) { - // this is the same code angular 1.3.15 has to check - // for a one time bind expression - return exp.charAt(0) === ':' && exp.charAt(1) === ':'; -} - function convertIdToOriginalType(scopeId) { return (angular.version.minor < 3) ? scopeId : parseInt(scopeId, 10); } + +function simpleExtend(dst, src) { + Object.keys(src).forEach(function(key) { + dst[key] = src[key]; + }); + return dst; +} diff --git a/test/scopes.spec.js b/test/scopes.spec.js index f229c60d..0f203e28 100644 --- a/test/scopes.spec.js +++ b/test/scopes.spec.js @@ -72,33 +72,88 @@ describe('ngHintScopes', function() { }); if (angular.version.minor >= 3) { - it('should not run perf timers for one time bind expressions passed to watch', function() { - var calls = hint.emit.calls; - scope.$watch('::a.b', function() {}); - expect(calls.count()).toBe(0); - - scope.$apply(); - var evt = calls.mostRecent().args[1].events[0]; - expect(calls.count()).toBe(1); - expect(evt).toBeUndefined(); - }); - - it('should not run perf timers for one time template bindings', function() { - var elt = angular.element( - '
' + - '{{::a}}' + - '' + - '
' - ); - scope.a = 'foo'; - var view = $compile(elt)(scope); - scope.$apply(); - var $binding = view.find('span'); - var $button = view.find('button'); - - $button.triggerHandler('click'); - scope.$apply(); - expect($binding.text()).toBe('foo'); + describe('one-time expressions', function() { + // Helpers + function countWatchEventsFor(callIdx) { + var args = hint.emit.calls.argsFor(callIdx); + var events = args[1].events; + var watchEvents = events.filter(function(evt) { + return evt.eventType === 'scope:watch'; + }); + + return watchEvents.length; + } + + it('should correctly trigger perf timers when passed to `$watch`', function() { + var calls = hint.emit.calls; + + var reactions = [0, 0, 0]; + var exps = [ + '::c.d', + ' ::c.d ', + ' :: c.d ' + ].forEach(function(exp, idx) { + scope.$watch(exp, function(v) { reactions[idx]++; }); + }); + + expect(calls.count()).toBe(0); + + scope.$apply(); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(6); // First $digest: 2 loops + expect(reactions).toEqual([1, 1, 1]); + + calls.reset(); + scope.$apply(); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(3); // No change: 1 loop + expect(reactions).toEqual([1, 1, 1]); + + calls.reset(); + scope.$apply('c.d = "foo"'); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(6); // Change: 2 loops + expect(reactions).toEqual([2, 2, 2]); + + calls.reset(); + scope.$apply('c.d = "bar"'); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(0); // Already settled: 0 loops + expect(reactions).toEqual([2, 2, 2]); + }); + + it('should correctly trigger perf timers when used in template bindings', function() { + var calls = hint.emit.calls; + + $compile( + '
' + + '{{::c.d}}' + + '{{ ::c.d }}' + + '{{ :: c.d }}' + + '{{::c.d}}{{ ::c.d }}{{ :: c.d }}' + + '
' + )(scope); + + calls.reset(); + scope.$apply(); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(12); // First $digest: 2 loops + + calls.reset(); + scope.$apply(); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(6); // No change: 1 loop + + calls.reset(); + scope.$apply('c.d = "foo"'); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(12); // Change: 2 loops + + calls.reset(); + scope.$apply('c.d = "bar"'); + expect(calls.count()).toBe(1); + expect(countWatchEventsFor(0)).toBe(0); // Already settled: 0 loops + }); }); } From 8a5f7c4b9c540fb9ce04d47ae2219fe7af96e453 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Thu, 30 Mar 2017 18:39:26 +0300 Subject: [PATCH 2/2] fixup! fix(scopes): correctly meassure perf impact of one-time expressions --- test/scopes.spec.js | 76 ++++++++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 22 deletions(-) diff --git a/test/scopes.spec.js b/test/scopes.spec.js index 0f203e28..b6b5ca7a 100644 --- a/test/scopes.spec.js +++ b/test/scopes.spec.js @@ -74,8 +74,16 @@ describe('ngHintScopes', function() { if (angular.version.minor >= 3) { describe('one-time expressions', function() { // Helpers - function countWatchEventsFor(callIdx) { - var args = hint.emit.calls.argsFor(callIdx); + function getDigestCallArgs() { + var allArgs = hint.emit.calls.allArgs(); + var digestCallArgs = allArgs.filter(function(args) { + return args[0] === 'scope:digest'; + }); + + return digestCallArgs; + } + + function countWatchEvents(args) { var events = args[1].events; var watchEvents = events.filter(function(evt) { return evt.eventType === 'scope:watch'; @@ -86,6 +94,7 @@ describe('ngHintScopes', function() { it('should correctly trigger perf timers when passed to `$watch`', function() { var calls = hint.emit.calls; + var args; var reactions = [0, 0, 0]; var exps = [ @@ -99,60 +108,83 @@ describe('ngHintScopes', function() { expect(calls.count()).toBe(0); scope.$apply(); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(6); // First $digest: 2 loops - expect(reactions).toEqual([1, 1, 1]); + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(6); // Initial $digest: 2 loops + expect(reactions).toEqual([1, 1, 1]); // Initial $digest always calls listeners calls.reset(); scope.$apply(); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(3); // No change: 1 loop - expect(reactions).toEqual([1, 1, 1]); + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(3); // No change: 1 loop + expect(reactions).toEqual([1, 1, 1]); // No change: listeners not called calls.reset(); scope.$apply('c.d = "foo"'); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(6); // Change: 2 loops - expect(reactions).toEqual([2, 2, 2]); + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(6); // First change: 2 loops + expect(reactions).toEqual([2, 2, 2]); // First change to defined value calls listeners calls.reset(); scope.$apply('c.d = "bar"'); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(0); // Already settled: 0 loops - expect(reactions).toEqual([2, 2, 2]); + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(0); // Already settled: 0 loops + expect(reactions).toEqual([2, 2, 2]); // Already settled: listeners not called }); it('should correctly trigger perf timers when used in template bindings', function() { var calls = hint.emit.calls; + var args; $compile( '
' + + // Interpolation in node text: 6 bindings (1 + 1 + 1 + 3) '{{::c.d}}' + '{{ ::c.d }}' + '{{ :: c.d }}' + '{{::c.d}}{{ ::c.d }}{{ :: c.d }}' + + + // Interpolation in attribute value: 6 bindings (1 + 1 + 1 + 3) + '' + + '' + + '' + + '' + + + // Expressions: 3 bindings (1 + 1 + 1) + '' + + '' + + '' + + + // Total: 15 watchers (6 + 6 + 3) '
' )(scope); calls.reset(); scope.$apply(); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(12); // First $digest: 2 loops + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(30); // Initial $digest: 2 loops calls.reset(); scope.$apply(); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(6); // No change: 1 loop + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(15); // No change: 1 loop calls.reset(); scope.$apply('c.d = "foo"'); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(12); // Change: 2 loops + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(30); // First change: 2 loops calls.reset(); scope.$apply('c.d = "bar"'); - expect(calls.count()).toBe(1); - expect(countWatchEventsFor(0)).toBe(0); // Already settled: 0 loops + args = getDigestCallArgs(); + expect(args.length).toBe(1); + expect(countWatchEvents(args[0])).toBe(0); // Already settled: 0 loops }); }); }