From af41fa9ac400d0bce9510697384c1296baa85cc1 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Mon, 26 Jan 2015 16:16:17 -0800 Subject: [PATCH] feat(change_detection): modify change detectors to recompute pure functions only when their args change --- .../src/change_detection_jit_generator.es6 | 50 ++++++++++++++++--- .../src/dynamic_change_detector.js | 31 ++++++++++++ .../src/proto_change_detector.js | 6 +++ .../test/change_detection_spec.js | 19 +++++++ 4 files changed, 99 insertions(+), 7 deletions(-) diff --git a/modules/change_detection/src/change_detection_jit_generator.es6 b/modules/change_detection/src/change_detection_jit_generator.es6 index dc9282c185..8b97e22f0d 100644 --- a/modules/change_detection/src/change_detection_jit_generator.es6 +++ b/modules/change_detection/src/change_detection_jit_generator.es6 @@ -138,9 +138,10 @@ ${type}.prototype.detectChangesInRecords = function(throwOnChange) { } -function bodyTemplate(localDefinitions:string, records:string):string { +function bodyTemplate(localDefinitions:string, changeDefinitions:string, records:string):string { return ` ${localDefinitions} +${changeDefinitions} var ${TEMP_LOCAL}; var ${CHANGE_LOCAL}; var ${CHANGES_LOCAL} = []; @@ -172,10 +173,11 @@ ${notify} `; } -function referenceCheckTemplate(assignment, newValue, oldValue, addRecord, notify) { +function referenceCheckTemplate(assignment, newValue, oldValue, change, addRecord, notify) { return ` ${assignment} if (${newValue} !== ${oldValue} || (${newValue} !== ${newValue}) && (${oldValue} !== ${oldValue})) { + ${change} = true; ${addRecord} ${oldValue} = ${newValue}; } @@ -202,10 +204,23 @@ function localDefinitionsTemplate(names:List):string { return names.map((n) => `var ${n};`).join("\n"); } +function changeDefinitionsTemplate(names:List):string { + return names.map((n) => `var ${n} = false;`).join("\n"); +} + function fieldDefinitionsTemplate(names:List):string { return names.map((n) => `${n} = ${UTIL}.unitialized();`).join("\n"); } +function ifChangedGuardTemplate(changeNames:List, body:string):string { + var cond = changeNames.join(" || "); + return ` +if (${cond}) { + ${body} +} +`; +} + function addSimpleChangeRecordTemplate(protoIndex:number, oldValue:string, newValue:string) { return `${CHANGES_LOCAL}.push(${UTIL}.simpleChangeRecord(${PROTOS_ACCESSOR}[${protoIndex}].bindingMemento, ${oldValue}, ${newValue}));`; } @@ -215,6 +230,7 @@ export class ChangeDetectorJITGenerator { typeName:string; records:List; localNames:List; + changeNames:List; fieldNames:List; constructor(typeName:string, records:List) { @@ -222,6 +238,7 @@ export class ChangeDetectorJITGenerator { this.records = records; this.localNames = this.getLocalNames(records); + this.changeNames = this.getChangeNames(this.localNames); this.fieldNames = this.getFieldNames(this.localNames); } @@ -234,6 +251,10 @@ export class ChangeDetectorJITGenerator { return ["context"].concat(names); } + getChangeNames(localNames:List):List { + return localNames.map((n) => `change_${n}`); + } + getFieldNames(localNames:List):List { return localNames.map((n) => `this.${n}`); } @@ -259,13 +280,17 @@ export class ChangeDetectorJITGenerator { genBody():string { var rec = this.records.map((r) => this.genRecord(r)).join("\n"); - return bodyTemplate(this.genLocalDefinitions(), rec); + return bodyTemplate(this.genLocalDefinitions(), this.genChangeDefinitions(), rec); } genLocalDefinitions():string { return localDefinitionsTemplate(this.localNames); } + genChangeDefinitions():string { + return changeDefinitionsTemplate(this.changeNames); + } + genRecord(r:ProtoRecord):string { if (r.mode == RECORD_TYPE_STRUCTURAL_CHECK) { return this.getStructuralCheck(r); @@ -283,10 +308,17 @@ export class ChangeDetectorJITGenerator { genReferenceCheck(r:ProtoRecord):string { var newValue = this.localNames[r.selfIndex]; var oldValue = this.fieldNames[r.selfIndex]; + var change = this.changeNames[r.selfIndex]; var assignment = this.genUpdateCurrentValue(r); var addRecord = addSimpleChangeRecordTemplate(r.selfIndex - 1, oldValue, newValue); var notify = this.genNotify(r); - return referenceCheckTemplate(assignment, newValue, oldValue, r.lastInBinding ? addRecord : '', notify); + + var check = referenceCheckTemplate(assignment, newValue, oldValue, change, r.lastInBinding ? addRecord : '', notify);; + if (r.isPureFunction()) { + return this.ifChangedGuard(r, check); + } else { + return check; + } } genUpdateCurrentValue(r:ProtoRecord):string { @@ -320,18 +352,22 @@ export class ChangeDetectorJITGenerator { case RECORD_TYPE_INTERPOLATE: return assignmentTemplate(newValue, this.genInterpolation(r)); + case RECORD_TYPE_INVOKE_FORMATTER: + return assignmentTemplate(newValue, `${FORMATTERS_ACCESSOR}.get("${r.name}")(${args})`); + case RECORD_TYPE_KEYED_ACCESS: var key = this.localNames[r.args[0]]; return assignmentTemplate(newValue, `${context}[${key}]`); - case RECORD_TYPE_INVOKE_FORMATTER: - return assignmentTemplate(newValue, `${FORMATTERS_ACCESSOR}.get("${r.name}")(${args})`); - default: throw new BaseException(`Unknown operation ${r.mode}`); } } + ifChangedGuard(r:ProtoRecord, body:string):string { + return ifChangedGuardTemplate(r.args.map((a) => this.changeNames[a]), body); + } + genInterpolation(r:ProtoRecord):string{ var res = ""; for (var i = 0; i < r.args.length; ++i) { diff --git a/modules/change_detection/src/dynamic_change_detector.js b/modules/change_detection/src/dynamic_change_detector.js index 8bbbf41421..2223b10dc1 100644 --- a/modules/change_detection/src/dynamic_change_detector.js +++ b/modules/change_detection/src/dynamic_change_detector.js @@ -30,14 +30,19 @@ export class DynamicChangeDetector extends AbstractChangeDetector { dispatcher:any; formatters:Map; values:List; + changes:List; protos:List; constructor(dispatcher:any, formatters:Map, protoRecords:List) { super(); this.dispatcher = dispatcher; this.formatters = formatters; + this.values = ListWrapper.createFixedSize(protoRecords.length + 1); ListWrapper.fill(this.values, uninitialized); + + this.changes = ListWrapper.createFixedSize(protoRecords.length + 1); + this.protos = protoRecords; } @@ -82,17 +87,25 @@ export class DynamicChangeDetector extends AbstractChangeDetector { } _referenceCheck(proto:ProtoRecord) { + if (this._pureFuncAndArgsDidNotChange(proto)) { + this._setChanged(proto, false); + return null; + } + var prevValue = this._readSelf(proto); var currValue = this._calculateCurrValue(proto); if (!isSame(prevValue, currValue)) { this._writeSelf(proto, currValue); + this._setChanged(proto, true); + if (proto.lastInBinding) { return new SimpleChange(prevValue, currValue); } else { return null; } } else { + this._setChanged(proto, false); return null; } } @@ -179,6 +192,24 @@ export class DynamicChangeDetector extends AbstractChangeDetector { this.values[proto.selfIndex] = value; } + _setChanged(proto:ProtoRecord, value:boolean) { + this.changes[proto.selfIndex] = value; + } + + _pureFuncAndArgsDidNotChange(proto:ProtoRecord):boolean { + return proto.isPureFunction() && !this._argsChanged(proto); + } + + _argsChanged(proto:ProtoRecord):boolean { + var args = proto.args; + for(var i = 0; i < args.length; ++i) { + if (this.changes[args[i]]) { + return true; + } + } + return false; + } + _readArgs(proto:ProtoRecord) { var res = ListWrapper.createFixedSize(proto.args.length); var args = proto.args; diff --git a/modules/change_detection/src/proto_change_detector.js b/modules/change_detection/src/proto_change_detector.js index 0c139627d6..7a96953790 100644 --- a/modules/change_detection/src/proto_change_detector.js +++ b/modules/change_detection/src/proto_change_detector.js @@ -84,6 +84,12 @@ export class ProtoRecord { this.lastInGroup = lastInGroup; this.expressionAsString = expressionAsString; } + + isPureFunction():boolean { + return this.mode === RECORD_TYPE_INTERPOLATE || + this.mode === RECORD_TYPE_INVOKE_FORMATTER || + this.mode === RECORD_TYPE_PRIMITIVE_OP; + } } export class ProtoChangeDetector { diff --git a/modules/change_detection/test/change_detection_spec.js b/modules/change_detection/test/change_detection_spec.js index 905f665be6..195aa00ce8 100644 --- a/modules/change_detection/test/change_detection_spec.js +++ b/modules/change_detection/test/change_detection_spec.js @@ -443,6 +443,25 @@ export function main() { }); }); }); + + describe("optimizations", () => { + it("should not rerun formatters when args did not change", () => { + var count = 0; + var formatters = MapWrapper.createFromPairs([ + ['count', (v) => {count ++; "value"}]]); + + var c = createChangeDetector('a', 'a | count', new TestData(null), formatters); + var cd = c["changeDetector"]; + + cd.detectChanges(); + + expect(count).toEqual(1); + + cd.detectChanges(); + + expect(count).toEqual(1); + }); + }); }); }); }