Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

WIP: Filter cache #8942

Closed
wants to merge 4 commits into from
Closed

WIP: Filter cache #8942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a7965d7
test(parseSpec): group filter specs into a single describe
IgorMinar Sep 3, 2014
9cf02ae
test(parseSpec): enable commented out asserts
IgorMinar Sep 3, 2014
65caaaa
perf($parse): cache filter results for primitive inputs
IgorMinar Sep 4, 2014
dd0b9e6
WIP: perf optimizations
IgorMinar Sep 5, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,12 @@ function isPromiseLike(obj) {
}


function isPrimitive(value) {
var valueType;
return (value == null || (valueType = typeof value) === 'string' || valueType === 'number');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed boolean here

}


var trim = function(value) {
return isString(value) ? value.trim() : value;
};
Expand Down
44 changes: 41 additions & 3 deletions src/ng/parse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = filterName + '_i_' + expression;
var filterCacheKeyResult = filterName + '_r_' + expression;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the filter has no parameters, you put an array of the values and if does not, the raw value. I wonder if this can be an issue as filterName + '_i_' + expression is not unique. This is, given the expression foo | filter:bar && foo | filter, both filter will have filterCacheKeyInput == 'filter_i_foo | filter:bar && foo | filter', but one will expect filterCache[filterCacheKeyInput] to be an array and the other the raw value. Anyhow, the solution would be to change this to

var filterIndexAtExpression = this.index;
var filterCacheKeyInput = filterName + '_' + filterIndexAtExpression +  '_i_' + expression;
var filterCacheKeyResult = filterName + '_' + filterIndexAtExpression +  '_r_' + expression;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point :)

var fn = this.$filter(filterName);
var argsFn;
var args;

Expand All @@ -572,6 +576,9 @@ Parser.prototype = {
}

return valueFn(function $parseFilter(self, locals, input) {
var result;
var filterCache = self.$$filterCache;

if (args) {
args[0] = input;

Expand All @@ -580,10 +587,41 @@ 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 (filterCacheKeyInput in filterCache &&
equals(filterCache[filterCacheKeyInput], args)) {
result = filterCache[filterCacheKeyResult];
} else {
result = fn.apply(undefined, args);
filterCache[filterCacheKeyInput] = shallowCopy(args, []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the assumption that filterCache will be there, this is, that the result of $parse will always be executed on something that is a scope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. I don't know if anyone uses $parse without scope, but we can guard against the filtercache not being present

filterCache[filterCacheKeyResult] = result;
}
} else {
result = fn.apply(undefined, args);
}

return result;
}

if (isPrimitive(input)) {
if (filterCache[filterCacheKeyInput] === input && (input === undefined || filterCacheKeyInput in filterCache)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am reading this wrong of if this is the first time the filter is executed and input is undefined then the result will be undefined and not the value from the filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

darn it Lucas, how do you always find these corner-cases. you rock!

result = filterCache[filterCacheKeyResult];
} else {
result = fn(input);
filterCache[filterCacheKeyInput] = input;
filterCache[filterCacheKeyResult] = result;
}
} else {
result = fn(input);
}

return fn(input);
return result;
});
},

Expand Down
2 changes: 2 additions & 0 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ function $RootScopeProvider(){
this.$$listenerCount = {};
this.$$isolateBindings = {};
this.$$applyAsyncQueue = [];
this.$$filterCache = createMap();
}

/**
Expand Down Expand Up @@ -207,6 +208,7 @@ function $RootScopeProvider(){
this.$$listenerCount = {};
this.$id = nextUid();
this.$$ChildScope = null;
this.$$filterCache = createMap();
};
this.$$ChildScope.prototype = this;
}
Expand Down
215 changes: 192 additions & 23 deletions test/ng/parseSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -585,15 +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");
});

it('should evaluate grouped filters', function() {
scope.name = 'MISKO';
expect(scope.$eval('n = (name|lowercase)')).toEqual('misko');
expect(scope.$eval('n')).toEqual('misko');
expect(scope.$eval('books[1] = "moby"')).toEqual("moby");
expect(scope.$eval('books[1]')).toEqual("moby");
});

it('should evaluate remainder', function() {
Expand Down Expand Up @@ -677,6 +656,196 @@ 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() {

var log;

beforeEach(inject(function (_log_) {
log = _log_;
}));


it('should parse filters', function() {
registerFilter(substring);

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('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;
}
});


describe('sandboxing', function() {
describe('Function constructor', function() {
it('should NOT allow access to Function constructor in getter', function() {
Expand Down