WIP: perf($parse): only execute watched expressions when the inputs change

Author: jbedard

benchmarks/parsed-expressions-bp/main.html

@@ -31,6 +31,11 @@
           <label for="operators">Binary/Unary operators</label>
         </li>
 
+        <li>
+          <input type="radio" ng-model="expressionType" value="shortCircuitingOperators" id="shortCircuitingOperators">
+          <label for="shortCircuitingOperators">AND/OR short-circuiting operators</label>
+        </li>
+
         <li>
           <input type="radio" ng-model="expressionType" value="filters" id="filters">
           <label for="filters">Filters</label>
@@ -134,6 +139,17 @@
           <span bm-pe-watch="-rowIdx * 2 * rowIdx + rowIdx / rowIdx + 1"></span>
         </li>
 
+        <li ng-switch-when="shortCircuitingOperators" ng-repeat="(rowIdx, row) in ::data">
+          <span bm-pe-watch="rowIdx && row.odd"></span>
+          <span bm-pe-watch="row.odd && row.even"></span>
+          <span bm-pe-watch="row.odd && !row.even"></span>
+          <span bm-pe-watch="row.odd || row.even"></span>
+          <span bm-pe-watch="row.odd || row.even || row.index"></span>
+          <span bm-pe-watch="row.index === 1 || row.index === 2"></span>
+          <span bm-pe-watch="row.num0 < row.num1 && row.num1 < row.num2"></span>
+          <span bm-pe-watch="row.num0 < row.num1 || row.num1 < row.num2"></span>
+        </li>
+
         <li ng-switch-when="filters" ng-repeat="(rowIdx, row) in ::data">
           <span bm-pe-watch="rowIdx | noop"></span>
           <span bm-pe-watch="rowIdx | noop"></span>

src/ng/parse.js

@@ -506,17 +506,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,
-      inputs: [left, middle, right]
+      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,
-      inputs: [left, right]
+      constant: left.constant && right.constant,
+      inputs: !isBranching && [left, right]
     });
   },
 
@@ -564,7 +563,7 @@ Parser.prototype = {
       }
     }
 
-    var inputs = !fn.externalInput && [inputFn].concat(argsFn || []);
+    var inputs = [inputFn].concat(argsFn || []);
 
     return extend(function $parseFilter(self, locals) {
       var input = inputFn(self, locals);
@@ -581,8 +580,8 @@ Parser.prototype = {
 
       return fn(input);
     }, {
-      constant: inputs && inputs.every(isConstant),
-      inputs: inputs
+      constant: !fn.externalInput && inputs.every(isConstant),
+      inputs: !fn.externalInput && inputs
     });
   },
 
@@ -630,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;
   },
@@ -639,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;
   },
@@ -1215,13 +1214,17 @@ function $ParseProvider() {
         return isDefined(value) ? result : value;
       };
 
-      if (parsedExpression.$$watchDelegate) {
-        //Only transfer the inputsWatchDelegate for interceptors not marked as having externalInput
-        if (!(parsedExpression.$$watchDelegate === inputsWatchDelegate && interceptorFn.externalInput)) {
-          fn.inputs = parsedExpression.inputs;
-          fn.$$watchDelegate = parsedExpression.$$watchDelegate;
-        }
+      // Propagate $$watchDelegates other then inputsWatchDelegate
+      if (parsedExpression.$$watchDelegate &&
+          parsedExpression.$$watchDelegate !== inputsWatchDelegate) {
+        fn.$$watchDelegate = parsedExpression.$$watchDelegate;
+      } else if (!interceptorFn.externalInput) {
+        // 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 externalInput
+        fn.$$watchDelegate = inputsWatchDelegate;
+        fn.inputs = [parsedExpression];
       }
+
       return fn;
     }
   }];

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.externalInput = true;
+
         var self = this;
         // the current value, updated on each dirty-check run
         var newValue;

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('<!-- ngRepeat: i in items -->');
-      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('<span>-</span><!-- ngRepeat: i in items --><span>-</span>');
-      expect(logs).toEqual([1, 2, 1, 2]);
+      expect(logs.length).toBe(0);
     }));
 
 

test/ng/parseSpec.js

@@ -1324,6 +1324,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);