From 8f151f05a682ed0969dc299700f9c7ada6fb9562 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Tue, 29 Jul 2014 10:46:00 -0700 Subject: [PATCH 1/2] perf(Scope): use remove the need for the extra watch in $watchGroup Instead of using a counter and an extra watch, just schedule the reaction function via . This gives us the same/similar ordering and coalecsing of updates as counter without the extra overhead. Also the code is easier to read. Since interpolation uses watchGroup, this change additionally improves performance of interpolation. In large table benchmark digest cost went down by 15-20% for interpolation. Closes #8396 --- src/ng/rootScope.js | 34 +++++++++++++++++---------------- test/ng/compileSpec.js | 28 +++++++++++++-------------- test/ng/directive/ngBindSpec.js | 4 ++-- test/ng/rootScopeSpec.js | 6 +++--- 4 files changed, 37 insertions(+), 35 deletions(-) diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index a52baf637634..b92f96b4cec2 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -393,9 +393,9 @@ function $RootScopeProvider(){ var oldValues = new Array(watchExpressions.length); var newValues = new Array(watchExpressions.length); var deregisterFns = []; - var changeCount = 0; var self = this; - var masterUnwatch; + var changeReactionScheduled = false; + var firstRun = true; if (watchExpressions.length === 1) { // Special case size of one @@ -407,29 +407,31 @@ function $RootScopeProvider(){ } forEach(watchExpressions, function (expr, i) { - var unwatch = self.$watch(expr, function watchGroupSubAction(value, oldValue) { + var unwatchFn = self.$watch(expr, function watchGroupSubAction(value, oldValue) { newValues[i] = value; oldValues[i] = oldValue; - changeCount++; - }, false, function watchGroupDeregNotifier() { - arrayRemove(deregisterFns, unwatch); - if (!deregisterFns.length) { - masterUnwatch(); + if (!changeReactionScheduled) { + changeReactionScheduled = true; + self.$evalAsync(watchGroupAction); } }); + deregisterFns.push(unwatchFn); + }); - deregisterFns.push(unwatch); - }, this); + function watchGroupAction() { + changeReactionScheduled = false; - masterUnwatch = self.$watch(function watchGroupChangeWatch() { - return changeCount; - }, function watchGroupChangeAction(value, oldValue) { - listener(newValues, (value === oldValue) ? newValues : oldValues, self); - }); + if (firstRun) { + firstRun = false; + listener(newValues, newValues, self); + } else { + listener(newValues, oldValues, self); + } + } return function deregisterWatchGroup() { while (deregisterFns.length) { - deregisterFns[0](); + deregisterFns.shift()(); } }; }, diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index fb5dc525ded6..d2f1aec74b5a 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -2893,21 +2893,21 @@ describe('$compile', function() { inject(function($rootScope) { compile('
'); - expect(countWatches($rootScope)).toEqual(7); // 5 -> template watch group, 2 -> '=' + expect(countWatches($rootScope)).toEqual(6); // 4 -> template watch group, 2 -> '=' $rootScope.$digest(); expect(element.html()).toBe('1:;2:;3:;4:'); - expect(countWatches($rootScope)).toEqual(7); + expect(countWatches($rootScope)).toEqual(6); $rootScope.foo = 'foo'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:;3:foo;4:'); - expect(countWatches($rootScope)).toEqual(5); + expect(countWatches($rootScope)).toEqual(4); $rootScope.foo = 'baz'; $rootScope.bar = 'bar'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:bar;3:foo;4:bar'); - expect(countWatches($rootScope)).toEqual(4); + expect(countWatches($rootScope)).toEqual(3); $rootScope.bar = 'baz'; $rootScope.$digest(); @@ -2927,21 +2927,21 @@ describe('$compile', function() { inject(function($rootScope) { compile('
'); - expect(countWatches($rootScope)).toEqual(7); // 5 -> template watch group, 2 -> {{ }} + expect(countWatches($rootScope)).toEqual(6); // 4 -> template watch group, 2 -> {{ }} $rootScope.$digest(); expect(element.html()).toBe('1:;2:;3:;4:'); - expect(countWatches($rootScope)).toEqual(5); // (- 2) -> bind-once in template + expect(countWatches($rootScope)).toEqual(4); // (- 2) -> bind-once in template $rootScope.foo = 'foo'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:;3:;4:'); - expect(countWatches($rootScope)).toEqual(4); + expect(countWatches($rootScope)).toEqual(3); $rootScope.foo = 'baz'; $rootScope.bar = 'bar'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:bar;3:;4:'); - expect(countWatches($rootScope)).toEqual(4); + expect(countWatches($rootScope)).toEqual(3); $rootScope.bar = 'baz'; $rootScope.$digest(); @@ -2964,18 +2964,18 @@ describe('$compile', function() { compile('
'); $rootScope.$digest(); expect(element.html()).toBe('1:;2:;3:;4:'); - expect(countWatches($rootScope)).toEqual(7); // 5 -> template watch group, 2 -> '=' + expect(countWatches($rootScope)).toEqual(6); // 4 -> template watch group, 2 -> '=' $rootScope.foo = 'foo'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:;3:foo;4:'); - expect(countWatches($rootScope)).toEqual(5); + expect(countWatches($rootScope)).toEqual(4); $rootScope.foo = 'baz'; $rootScope.bar = 'bar'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:bar;3:foo;4:bar'); - expect(countWatches($rootScope)).toEqual(4); + expect(countWatches($rootScope)).toEqual(3); $rootScope.bar = 'baz'; $rootScope.$digest(); @@ -2998,18 +2998,18 @@ describe('$compile', function() { compile('
'); $rootScope.$digest(); expect(element.html()).toBe('1:;2:;3:;4:'); - expect(countWatches($rootScope)).toEqual(5); // (5 - 2) -> template watch group, 2 -> {{ }} + expect(countWatches($rootScope)).toEqual(4); // (4 - 2) -> template watch group, 2 -> {{ }} $rootScope.foo = 'foo'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:;3:;4:'); - expect(countWatches($rootScope)).toEqual(4); + expect(countWatches($rootScope)).toEqual(3); $rootScope.foo = 'baz'; $rootScope.bar = 'bar'; $rootScope.$digest(); expect(element.html()).toBe('1:foo;2:bar;3:;4:'); - expect(countWatches($rootScope)).toEqual(4); + expect(countWatches($rootScope)).toEqual(3); $rootScope.bar = 'baz'; $rootScope.$digest(); diff --git a/test/ng/directive/ngBindSpec.js b/test/ng/directive/ngBindSpec.js index 166fdb705a4b..704932b09085 100644 --- a/test/ng/directive/ngBindSpec.js +++ b/test/ng/directive/ngBindSpec.js @@ -99,11 +99,11 @@ describe('ngBind*', function() { it('should one-time bind the expressions that start with ::', inject(function($rootScope, $compile) { element = $compile('
')($rootScope); $rootScope.name = 'Misko'; - expect($rootScope.$$watchers.length).toEqual(3); + expect($rootScope.$$watchers.length).toEqual(2); $rootScope.$digest(); expect(element.hasClass('ng-binding')).toEqual(true); expect(element.text()).toEqual(' Misko!'); - expect($rootScope.$$watchers.length).toEqual(2); + expect($rootScope.$$watchers.length).toEqual(1); $rootScope.hello = 'Hello'; $rootScope.name = 'Lucas'; $rootScope.$digest(); diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index 1c26bed7b676..a6aa91ea12b4 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -149,14 +149,14 @@ describe('Scope', function() { it('should clean up stable watches from $watchGroup', inject(function($rootScope) { $rootScope.$watchGroup(['::foo', '::bar'], function() {}); - expect($rootScope.$$watchers.length).toEqual(3); + expect($rootScope.$$watchers.length).toEqual(2); $rootScope.$digest(); - expect($rootScope.$$watchers.length).toEqual(3); + expect($rootScope.$$watchers.length).toEqual(2); $rootScope.foo = 'foo'; $rootScope.$digest(); - expect($rootScope.$$watchers.length).toEqual(2); + expect($rootScope.$$watchers.length).toEqual(1); $rootScope.bar = 'bar'; $rootScope.$digest(); From 26c4a719ce89837ea82c8e89b9c1fdc9201cb412 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Wed, 30 Jul 2014 11:39:37 -0700 Subject: [PATCH 2/2] chore(Scope): remove deregisterNotifier feature for $watch We no longer have a need for this feature that was added to primarily support $watchGroup (see previous commit). BREAKING CHANGE: deregisterNotifier callback for $watch is no longer available This api was available only in the last few 1.3 beta versions and is not very useful for applications, so we don't expect that anyone will be affected by this change. --- src/ng/interpolate.js | 4 ++-- src/ng/parse.js | 8 ++++---- src/ng/rootScope.js | 9 ++------- test/ng/rootScopeSpec.js | 28 ---------------------------- 4 files changed, 8 insertions(+), 41 deletions(-) diff --git a/src/ng/interpolate.js b/src/ng/interpolate.js index 016fdfac77e9..e03aa961dcf4 100644 --- a/src/ng/interpolate.js +++ b/src/ng/interpolate.js @@ -301,7 +301,7 @@ function $InterpolateProvider() { exp: text, //just for compatibility with regular watchers created via $watch separators: separators, expressions: expressions, - $$watchDelegate: function (scope, listener, objectEquality, deregisterNotifier) { + $$watchDelegate: function (scope, listener, objectEquality) { var lastValue; return scope.$watchGroup(parseFns, function interpolateFnWatcher(values, oldValues) { var currValue = compute(values); @@ -309,7 +309,7 @@ function $InterpolateProvider() { listener.call(this, currValue, values !== oldValues ? lastValue : currValue, scope); } lastValue = currValue; - }, objectEquality, deregisterNotifier); + }, objectEquality); } }); } diff --git a/src/ng/parse.js b/src/ng/parse.js index de073f95e469..d1766b9a6425 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -1031,7 +1031,7 @@ function $ParseProvider() { } }; - function oneTimeWatch(scope, listener, objectEquality, deregisterNotifier, parsedExpression) { + function oneTimeWatch(scope, listener, objectEquality, parsedExpression) { var unwatch, lastValue; return unwatch = scope.$watch(function oneTimeWatch(scope) { return parsedExpression(scope); @@ -1047,10 +1047,10 @@ function $ParseProvider() { } }); } - }, objectEquality, deregisterNotifier); + }, objectEquality); } - function constantWatch(scope, listener, objectEquality, deregisterNotifier, parsedExpression) { + function constantWatch(scope, listener, objectEquality, parsedExpression) { var unwatch; return unwatch = scope.$watch(function constantWatch(scope) { return parsedExpression(scope); @@ -1059,7 +1059,7 @@ function $ParseProvider() { listener.apply(this, arguments); } unwatch(); - }, objectEquality, deregisterNotifier); + }, objectEquality); } function addInterceptor(parsedExpression, interceptorFn) { diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index b92f96b4cec2..24b80a36ed4a 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -322,15 +322,13 @@ function $RootScopeProvider(){ * - `scope` refers to the current scope * @param {boolean=} objectEquality Compare for object equality using {@link angular.equals} instead of * comparing for reference equality. - * @param {function()=} deregisterNotifier Function to call when the deregistration function - * get called. * @returns {function()} Returns a deregistration function for this listener. */ - $watch: function(watchExp, listener, objectEquality, deregisterNotifier) { + $watch: function(watchExp, listener, objectEquality) { var get = compileToFn(watchExp, 'watch'); if (get.$$watchDelegate) { - return get.$$watchDelegate(this, listener, objectEquality, deregisterNotifier, get); + return get.$$watchDelegate(this, listener, objectEquality, get); } var scope = this, array = scope.$$watchers, @@ -358,9 +356,6 @@ function $RootScopeProvider(){ return function deregisterWatch() { arrayRemove(array, watcher); lastDirtyWatch = null; - if (isFunction(deregisterNotifier)) { - deregisterNotifier(); - } }; }, diff --git a/test/ng/rootScopeSpec.js b/test/ng/rootScopeSpec.js index a6aa91ea12b4..f2f699a86ec7 100644 --- a/test/ng/rootScopeSpec.js +++ b/test/ng/rootScopeSpec.js @@ -526,34 +526,6 @@ describe('Scope', function() { expect(log).toEqual(['watch1', 'watchAction1', 'watch2', 'watchAction2', 'watch3', 'watchAction3', 'watch2', 'watch3']); })); - - describe('deregisterNotifier', function () { - it('should call the deregisterNotifier when the watch is deregistered', inject( - function($rootScope) { - var notifier = jasmine.createSpy('deregisterNotifier'); - var listenerRemove = $rootScope.$watch('noop', noop, false, notifier); - - expect(notifier).not.toHaveBeenCalled(); - - listenerRemove(); - expect(notifier).toHaveBeenCalledOnce(); - })); - - - it('should call the deregisterNotifier when a one-time expression is stable', inject( - function($rootScope) { - var notifier = jasmine.createSpy('deregisterNotifier'); - $rootScope.$watch('::foo', noop, false, notifier); - - expect(notifier).not.toHaveBeenCalledOnce(); - $rootScope.$digest(); - expect(notifier).not.toHaveBeenCalledOnce(); - - $rootScope.foo = 'foo'; - $rootScope.$digest(); - expect(notifier).toHaveBeenCalledOnce(); - })); - }); });