From a7965d7305abaab39583059b4175d1530e222159 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 4 Sep 2014 00:23:40 +0200 Subject: [PATCH 1/4] test(parseSpec): group filter specs into a single describe --- test/ng/parseSpec.js | 46 +++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index 265f12c67b54..1fc24f3f1a1b 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -334,20 +334,6 @@ describe('parser', function() { expect(scope.$eval("'a' + 'b c'")).toEqual("ab c"); }); - it('should parse filters', function() { - $filterProvider.register('substring', valueFn(function(input, start, end) { - return input.substring(start, end); - })); - - expect(function() { - scope.$eval("1|nonexistent"); - }).toThrowMinErr('$injector', 'unpr', 'Unknown provider: nonexistentFilterProvider <- nonexistentFilter'); - - scope.offset = 3; - expect(scope.$eval("'abcd'|substring:1:offset")).toEqual("bc"); - expect(scope.$eval("'abcd'|substring:1:3|uppercase")).toEqual("BC"); - }); - it('should access scope', function() { scope.a = 123; scope.b = {c: 456}; @@ -590,12 +576,6 @@ describe('parser', function() { // expect(scope.$eval('books[1]')).toEqual("moby"); }); - it('should evaluate grouped filters', function() { - scope.name = 'MISKO'; - expect(scope.$eval('n = (name|lowercase)')).toEqual('misko'); - expect(scope.$eval('n')).toEqual('misko'); - }); - it('should evaluate remainder', function() { expect(scope.$eval('1%2')).toEqual(1); }); @@ -677,6 +657,32 @@ describe('parser', function() { expect(scope.$eval('a + \n b.c + \r "\td" + \t \r\n\r "\r\n\n"')).toEqual("abc\td\r\n\n"); }); + + describe('filters', function() { + + it('should parse filters', function() { + $filterProvider.register('substring', valueFn(function(input, start, end) { + return input.substring(start, end); + })); + + expect(function() { + scope.$eval("1|nonexistent"); + }).toThrowMinErr('$injector', 'unpr', 'Unknown provider: nonexistentFilterProvider <- nonexistentFilter'); + + scope.offset = 3; + expect(scope.$eval("'abcd'|substring:1:offset")).toEqual("bc"); + expect(scope.$eval("'abcd'|substring:1:3|uppercase")).toEqual("BC"); + }); + + + it('should evaluate grouped filters', function() { + scope.name = 'MISKO'; + expect(scope.$eval('n = (name|lowercase)')).toEqual('misko'); + expect(scope.$eval('n')).toEqual('misko'); + }); + }); + + describe('sandboxing', function() { describe('Function constructor', function() { it('should NOT allow access to Function constructor in getter', function() { From 9cf02aea65bde2d860a1f35e6427661e49a604cb Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 4 Sep 2014 00:24:21 +0200 Subject: [PATCH 2/4] test(parseSpec): enable commented out asserts they pass without any modifactions. --- test/ng/parseSpec.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index 1fc24f3f1a1b..d35582f9556f 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -571,9 +571,8 @@ describe('parser', function() { expect(scope.$eval('items[1] = "abc"')).toEqual("abc"); expect(scope.$eval('items[1]')).toEqual("abc"); - // Dont know how to make this work.... - // expect(scope.$eval('books[1] = "moby"')).toEqual("moby"); - // expect(scope.$eval('books[1]')).toEqual("moby"); + expect(scope.$eval('books[1] = "moby"')).toEqual("moby"); + expect(scope.$eval('books[1]')).toEqual("moby"); }); it('should evaluate remainder', function() { From 65caaaa285f476eb3b4dbd66b0f4568585b66a8b Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 5 Sep 2014 01:58:16 +0200 Subject: [PATCH 3/4] perf($parse): cache filter results for primitive inputs BREAKING CHANGE: filters that have hidden state won't be recalculated if the hidden state changes TODO: more info... --- src/Angular.js | 6 ++ src/ng/parse.js | 44 ++++++++++- test/ng/parseSpec.js | 170 ++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 214 insertions(+), 6 deletions(-) diff --git a/src/Angular.js b/src/Angular.js index 5177ece218c2..df66ca585e85 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -573,6 +573,12 @@ function isPromiseLike(obj) { } +function isPrimitive(value) { + var valueType; + return (value == null || (valueType = typeof value) === 'number' || valueType === 'string'); +} + + var trim = function(value) { return isString(value) ? value.trim() : value; }; diff --git a/src/ng/parse.js b/src/ng/parse.js index 8681cd0ed9ff..de7617621218 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -559,7 +559,11 @@ Parser.prototype = { filter: function() { var token = this.expect(); - var fn = this.$filter(token.text); + var filterName = token.text; + var expression = this.text; + var filterCacheKeyInput = '$$filterCache_i_' + filterName + '_' + expression; + var filterCacheKeyResult = '$$filterCache_r_' + filterName + '_' + expression; + var fn = this.$filter(filterName); var argsFn; var args; @@ -572,6 +576,8 @@ Parser.prototype = { } return valueFn(function $parseFilter(self, locals, input) { + var result; + if (args) { args[0] = input; @@ -580,10 +586,42 @@ Parser.prototype = { args[i + 1] = argsFn[i](self, locals); } - return fn.apply(undefined, args); + var primitiveInputs = true; + i = args.length; + while (primitiveInputs && i--) { + primitiveInputs = primitiveInputs && isPrimitive(args[i]); + } + + if (primitiveInputs) { + if (hasOwnProperty.call(self, filterCacheKeyInput) && + equals(self[filterCacheKeyInput], args)) { + result = self[filterCacheKeyResult]; + } else { + result = fn.apply(undefined, args); + self[filterCacheKeyInput] = shallowCopy(args, []); + self[filterCacheKeyResult] = result; + } + } else { + result = fn.apply(undefined, args); + } + + return result; + } + + if (isPrimitive(input)) { + if (hasOwnProperty.call(self, filterCacheKeyInput) && + self[filterCacheKeyInput] === input) { + result = self[filterCacheKeyResult]; + } else { + result = fn(input); + self[filterCacheKeyInput] = input; + self[filterCacheKeyResult] = result; + } + } else { + result = fn(input); } - return fn(input); + return result; }); }, diff --git a/test/ng/parseSpec.js b/test/ng/parseSpec.js index d35582f9556f..1eec7f32f48e 100644 --- a/test/ng/parseSpec.js +++ b/test/ng/parseSpec.js @@ -659,10 +659,15 @@ describe('parser', function() { describe('filters', function() { + var log; + + beforeEach(inject(function (_log_) { + log = _log_; + })); + + it('should parse filters', function() { - $filterProvider.register('substring', valueFn(function(input, start, end) { - return input.substring(start, end); - })); + registerFilter(substring); expect(function() { scope.$eval("1|nonexistent"); @@ -679,6 +684,165 @@ describe('parser', function() { expect(scope.$eval('n = (name|lowercase)')).toEqual('misko'); expect(scope.$eval('n')).toEqual('misko'); }); + + + describe('caching', function() { + + it("should cache the last value and not re-execute filters if inputs don't change", function() { + registerFilter(starlog); + + scope.message = 'harder'; + expect(scope.$eval('message | starlog')).toBe('*harder*'); + expect(log.empty()).toEqual(['*harder*']); + + expect(scope.$eval('message | starlog')).toBe('*harder*'); + expect(log.empty()).toEqual([]); + + scope.message = 'better'; + expect(scope.$eval('message | starlog')).toBe('*better*'); + expect(log.empty()).toEqual(['*better*']); + }); + + + it("should associate the cache with the evaluation scope", function() { + registerFilter(starlog); + + var childScope1 = scope.$new(); + var childScope2 = scope.$new(); + + childScope1.message = 'harder'; + childScope2.message = 'harder'; + + expect(childScope1.$eval('message | starlog')).toBe('*harder*'); + expect(log.empty()).toEqual(['*harder*']); + expect(childScope2.$eval('message | starlog')).toBe('*harder*'); + expect(log.empty()).toEqual(['*harder*']); + }); + + + it("should cache just the last value", function() { + registerFilter(starlog); + + scope.message = 'harder'; + expect(scope.$eval('message | starlog')).toBe('*harder*'); + expect(log.empty()).toEqual(['*harder*']); + + scope.message = 'better'; + expect(scope.$eval('message | starlog')).toBe('*better*'); + expect(log.empty()).toEqual(['*better*']); + + scope.message = 'harder'; + expect(scope.$eval('message | starlog')).toBe('*harder*'); + expect(log.empty()).toEqual(['*harder*']); + }); + + + it("should not cache results for object inputs", function() { + registerFilter(starlog); + + scope.obj = {}; + expect(scope.$eval('obj | starlog')).toBe('*{}*'); + expect(log.empty()).toEqual(['*{}*']); + + expect(scope.$eval('obj | starlog')).toBe('*{}*'); + expect(log.empty()).toEqual(['*{}*']); + }); + + + it("should not cache results for array inputs", function() { + registerFilter(starlog); + + scope.array = []; + expect(scope.$eval('array | starlog')).toBe('*[]*'); + expect(log.empty()).toEqual(['*[]*']); + + expect(scope.$eval('array | starlog')).toBe('*[]*'); + expect(log.empty()).toEqual(['*[]*']); + }); + + + it("should cache filters with extra params", function() { + registerFilter(starlog); + + scope.message = 'harder'; + scope.stardate = '333'; + expect(scope.$eval('message | starlog:stardate')).toBe('*harder [333] *'); + expect(log.empty()).toEqual(['*harder [333] *']); + + expect(scope.$eval('message | starlog:stardate')).toBe('*harder [333] *'); + expect(log.empty()).toEqual([]); + }); + + + it('should recalc the result if extra param changes', function() { + registerFilter(starlog); + + scope.message = 'harder'; + scope.stardate = '333'; + expect(scope.$eval('message | starlog:stardate')).toBe('*harder [333] *'); + expect(log.empty()).toEqual(['*harder [333] *']); + + scope.stardate = '111'; + expect(scope.$eval('message | starlog:stardate')).toBe('*harder [111] *'); + expect(log.empty()).toEqual(['*harder [111] *']); + + expect(scope.$eval('message | starlog:stardate')).toBe('*harder [111] *'); + expect(log.empty()).toEqual([]); + }); + + + it("should not cache results for object params", function() { + registerFilter(starlog); + + scope.obj = {}; + expect(scope.$eval('1 | starlog:obj')).toBe('*1 [{}] *'); + expect(log.empty()).toEqual(['*1 [{}] *']); + + expect(scope.$eval('1 | starlog:obj')).toBe('*1 [{}] *'); + expect(log.empty()).toEqual(['*1 [{}] *']); + }); + + + it("should not cache results for array params", function() { + registerFilter(starlog); + + scope.array = []; + expect(scope.$eval('1 | starlog:array')).toBe('*1 [[]] *'); + expect(log.empty()).toEqual(['*1 [[]] *']); + + expect(scope.$eval('1 | starlog:array')).toBe('*1 [[]] *'); + expect(log.empty()).toEqual(['*1 [[]] *']); + }); + }); + + + function registerFilter(filter) { + $filterProvider.register(filter.name, valueFn(filter)); + } + + function substring(input, start, end) { + return input.substring(start, end); + } + + function starlog(input, param) { + input = isString(input) ? input : toJson(input); + + if (param) { + param = ' [' + (isString(param) ? param : toJson(param)) + '] '; + } else { + param = ''; + } + + var result = '*' + input + param + '*'; + log(result); + return result; + } + + function dashlog(input) { + var result = '-' + input + '-'; + log(result); + return result; + } }); From dd0b9e6097dcbae3a2fae4848efa58f2c8889e11 Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Fri, 5 Sep 2014 02:48:58 +0200 Subject: [PATCH 4/4] WIP: perf optimizations avoid hasOwnProperty calls and more --- src/Angular.js | 2 +- src/ng/parse.js | 24 ++++++++++++------------ src/ng/rootScope.js | 2 ++ 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/Angular.js b/src/Angular.js index df66ca585e85..c37c1d97c696 100644 --- a/src/Angular.js +++ b/src/Angular.js @@ -575,7 +575,7 @@ function isPromiseLike(obj) { function isPrimitive(value) { var valueType; - return (value == null || (valueType = typeof value) === 'number' || valueType === 'string'); + return (value == null || (valueType = typeof value) === 'string' || valueType === 'number'); } diff --git a/src/ng/parse.js b/src/ng/parse.js index de7617621218..90aa51ce810d 100644 --- a/src/ng/parse.js +++ b/src/ng/parse.js @@ -561,8 +561,8 @@ Parser.prototype = { var token = this.expect(); var filterName = token.text; var expression = this.text; - var filterCacheKeyInput = '$$filterCache_i_' + filterName + '_' + expression; - var filterCacheKeyResult = '$$filterCache_r_' + filterName + '_' + expression; + var filterCacheKeyInput = filterName + '_i_' + expression; + var filterCacheKeyResult = filterName + '_r_' + expression; var fn = this.$filter(filterName); var argsFn; var args; @@ -577,6 +577,7 @@ Parser.prototype = { return valueFn(function $parseFilter(self, locals, input) { var result; + var filterCache = self.$$filterCache; if (args) { args[0] = input; @@ -593,13 +594,13 @@ Parser.prototype = { } if (primitiveInputs) { - if (hasOwnProperty.call(self, filterCacheKeyInput) && - equals(self[filterCacheKeyInput], args)) { - result = self[filterCacheKeyResult]; + if (filterCacheKeyInput in filterCache && + equals(filterCache[filterCacheKeyInput], args)) { + result = filterCache[filterCacheKeyResult]; } else { result = fn.apply(undefined, args); - self[filterCacheKeyInput] = shallowCopy(args, []); - self[filterCacheKeyResult] = result; + filterCache[filterCacheKeyInput] = shallowCopy(args, []); + filterCache[filterCacheKeyResult] = result; } } else { result = fn.apply(undefined, args); @@ -609,13 +610,12 @@ Parser.prototype = { } if (isPrimitive(input)) { - if (hasOwnProperty.call(self, filterCacheKeyInput) && - self[filterCacheKeyInput] === input) { - result = self[filterCacheKeyResult]; + if (filterCache[filterCacheKeyInput] === input && (input === undefined || filterCacheKeyInput in filterCache)) { + result = filterCache[filterCacheKeyResult]; } else { result = fn(input); - self[filterCacheKeyInput] = input; - self[filterCacheKeyResult] = result; + filterCache[filterCacheKeyInput] = input; + filterCache[filterCacheKeyResult] = result; } } else { result = fn(input); diff --git a/src/ng/rootScope.js b/src/ng/rootScope.js index 998c4ce9ce67..9d951c966e3a 100644 --- a/src/ng/rootScope.js +++ b/src/ng/rootScope.js @@ -136,6 +136,7 @@ function $RootScopeProvider(){ this.$$listenerCount = {}; this.$$isolateBindings = {}; this.$$applyAsyncQueue = []; + this.$$filterCache = createMap(); } /** @@ -207,6 +208,7 @@ function $RootScopeProvider(){ this.$$listenerCount = {}; this.$id = nextUid(); this.$$ChildScope = null; + this.$$filterCache = createMap(); }; this.$$ChildScope.prototype = this; }