From 7e92d2e6b7e77fb019a147e4728c034968d82bdb Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Fri, 16 Oct 2015 10:55:21 -0700 Subject: [PATCH] feat(ChangeDetector): Add support for short-circuiting --- .../change_detection_jit_generator.ts | 100 ++++++++-- .../change_detection/change_detection_util.ts | 2 - .../src/core/change_detection/coalesce.ts | 185 +++++++++++++----- .../dynamic_change_detector.ts | 44 ++++- .../change_detection/proto_change_detector.ts | 52 +++-- .../src/core/change_detection/proto_record.ts | 15 +- .../change_detector_config.ts | 12 +- .../change_detection/change_detector_spec.ts | 89 ++++++++- .../core/change_detection/coalesce_spec.ts | 113 ++++++++++- .../change_detector_codegen.dart | 83 +++++++- 10 files changed, 582 insertions(+), 113 deletions(-) diff --git a/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts b/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts index 79473de4e6..883fadab96 100644 --- a/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts +++ b/modules/angular2/src/core/change_detection/change_detection_jit_generator.ts @@ -31,6 +31,7 @@ const CHANGES_LOCAL = "changes"; export class ChangeDetectorJITGenerator { private _logic: CodegenLogicUtil; private _names: CodegenNameUtil; + private _endOfBlockIdxs: number[]; private id: string; private changeDetectionStrategy: ChangeDetectionStrategy; private records: ProtoRecord[]; @@ -91,7 +92,7 @@ export class ChangeDetectorJITGenerator { var ${IS_CHANGED_LOCAL} = false; var ${CHANGES_LOCAL} = null; - ${this.records.map((r) => this._genRecord(r)).join("\n")} + ${this._genAllRecords(this.records)} } ${this._maybeGenHandleEventInternal()} @@ -144,10 +145,28 @@ export class ChangeDetectorJITGenerator { /** @internal */ _genEventBinding(eb: EventBinding): string { - var recs = eb.records.map(r => this._genEventBindingEval(eb, r)).join("\n"); + let codes: String[] = []; + this._endOfBlockIdxs = []; + + ListWrapper.forEachWithIndex(eb.records, (r, i) => { + let code; + + if (r.isConditionalSkipRecord()) { + code = this._genConditionalSkip(r, this._names.getEventLocalName(eb, i)); + } else if (r.isUnconditionalSkipRecord()) { + code = this._genUnconditionalSkip(r); + } else { + code = this._genEventBindingEval(eb, r); + } + + code += this._genEndOfSkipBlock(i); + + codes.push(code); + }); + return ` if (eventName === "${eb.eventName}" && elIndex === ${eb.elIndex}) { - ${recs} + ${codes.join("\n")} }`; } @@ -235,20 +254,65 @@ export class ChangeDetectorJITGenerator { } /** @internal */ - _genRecord(r: ProtoRecord): string { - var rec; - if (r.isLifeCycleRecord()) { - rec = this._genDirectiveLifecycle(r); - } else if (r.isPipeRecord()) { - rec = this._genPipeCheck(r); - } else { - rec = this._genReferenceCheck(r); + _genAllRecords(rs: ProtoRecord[]): string { + var codes: String[] = []; + this._endOfBlockIdxs = []; + + for (let i = 0; i < rs.length; i++) { + let code; + let r = rs[i]; + + if (r.isLifeCycleRecord()) { + code = this._genDirectiveLifecycle(r); + } else if (r.isPipeRecord()) { + code = this._genPipeCheck(r); + } else if (r.isConditionalSkipRecord()) { + code = this._genConditionalSkip(r, this._names.getLocalName(r.contextIndex)); + } else if (r.isUnconditionalSkipRecord()) { + code = this._genUnconditionalSkip(r); + } else { + code = this._genReferenceCheck(r); + } + + code = ` + ${this._maybeFirstInBinding(r)} + ${code} + ${this._maybeGenLastInDirective(r)} + ${this._genEndOfSkipBlock(i)} + `; + + codes.push(code); } - return ` - ${this._maybeFirstInBinding(r)} - ${rec} - ${this._maybeGenLastInDirective(r)} - `; + + return codes.join("\n"); + } + + /** @internal */ + _genConditionalSkip(r: ProtoRecord, condition: string): string { + let maybeNegate = r.mode === RecordType.SkipRecordsIf ? '!' : ''; + this._endOfBlockIdxs.push(r.fixedArgs[0] - 1); + + return `if (${maybeNegate}${condition}) {`; + } + + /** @internal */ + _genUnconditionalSkip(r: ProtoRecord): string { + this._endOfBlockIdxs.pop(); + this._endOfBlockIdxs.push(r.fixedArgs[0] - 1); + return `} else {`; + } + + /** @internal */ + _genEndOfSkipBlock(protoIndex: number): string { + if (!ListWrapper.isEmpty(this._endOfBlockIdxs)) { + let endOfBlock = ListWrapper.last(this._endOfBlockIdxs); + if (protoIndex === endOfBlock) { + this._endOfBlockIdxs.pop(); + return '}'; + } + } + + return ''; } /** @internal */ @@ -401,8 +465,8 @@ export class ChangeDetectorJITGenerator { /** @internal */ _maybeFirstInBinding(r: ProtoRecord): string { var prev = ChangeDetectionUtil.protoByIndex(this.records, r.selfIndex - 1); - var firstInBindng = isBlank(prev) || prev.bindingRecord !== r.bindingRecord; - return firstInBindng && !r.bindingRecord.isDirectiveLifecycle() ? + var firstInBinding = isBlank(prev) || prev.bindingRecord !== r.bindingRecord; + return firstInBinding && !r.bindingRecord.isDirectiveLifecycle() ? `${this._names.getPropertyBindingIndex()} = ${r.propertyBindingIndex};` : ''; } diff --git a/modules/angular2/src/core/change_detection/change_detection_util.ts b/modules/angular2/src/core/change_detection/change_detection_util.ts index f115fa0869..386788b21a 100644 --- a/modules/angular2/src/core/change_detection/change_detection_util.ts +++ b/modules/angular2/src/core/change_detection/change_detection_util.ts @@ -126,8 +126,6 @@ export class ChangeDetectionUtil { static operation_greater_then(left, right): any { return left > right; } static operation_less_or_equals_then(left, right): any { return left <= right; } static operation_greater_or_equals_then(left, right): any { return left >= right; } - static operation_logical_and(left, right): any { return left && right; } - static operation_logical_or(left, right): any { return left || right; } static cond(cond, trueVal, falseVal): any { return cond ? trueVal : falseVal; } static mapFn(keys: any[]): any { diff --git a/modules/angular2/src/core/change_detection/coalesce.ts b/modules/angular2/src/core/change_detection/coalesce.ts index fcc6b07a53..19ef48380f 100644 --- a/modules/angular2/src/core/change_detection/coalesce.ts +++ b/modules/angular2/src/core/change_detection/coalesce.ts @@ -1,4 +1,4 @@ -import {isPresent, isBlank, looseIdentical, StringWrapper} from 'angular2/src/core/facade/lang'; +import {isPresent, isBlank, looseIdentical} from 'angular2/src/core/facade/lang'; import {ListWrapper, Map} from 'angular2/src/core/facade/collection'; import {RecordType, ProtoRecord} from './proto_record'; @@ -13,51 +13,158 @@ import {RecordType, ProtoRecord} from './proto_record'; * * @internal */ -export function coalesce(records: ProtoRecord[]): ProtoRecord[] { - var res: ProtoRecord[] = []; - var indexMap: Map = new Map(); +export function coalesce(srcRecords: ProtoRecord[]): ProtoRecord[] { + let dstRecords = []; + let excludedIdxs = []; + let indexMap: Map = new Map(); + let skipDepth = 0; + let skipSources: ProtoRecord[] = ListWrapper.createFixedSize(srcRecords.length); - for (var i = 0; i < records.length; ++i) { - var r = records[i]; - var record = _replaceIndices(r, res.length + 1, indexMap); - var matchingRecord = _findMatching(record, res); + for (let protoIndex = 0; protoIndex < srcRecords.length; protoIndex++) { + let skipRecord = skipSources[protoIndex]; + if (isPresent(skipRecord)) { + skipDepth--; + skipRecord.fixedArgs[0] = dstRecords.length; + } - if (isPresent(matchingRecord) && record.lastInBinding) { - res.push(_selfRecord(record, matchingRecord.selfIndex, res.length + 1)); - indexMap.set(r.selfIndex, matchingRecord.selfIndex); - matchingRecord.referencedBySelf = true; - - } else if (isPresent(matchingRecord) && !record.lastInBinding) { - if (record.argumentToPureFunction) { - matchingRecord.argumentToPureFunction = true; - } - - indexMap.set(r.selfIndex, matchingRecord.selfIndex); + let src = srcRecords[protoIndex]; + let dst = _cloneAndUpdateIndexes(src, dstRecords, indexMap); + if (dst.isSkipRecord()) { + dstRecords.push(dst); + skipDepth++; + skipSources[dst.fixedArgs[0]] = dst; } else { - res.push(record); - indexMap.set(r.selfIndex, record.selfIndex); + let record = _mayBeAddRecord(dst, dstRecords, excludedIdxs, skipDepth > 0); + indexMap.set(src.selfIndex, record.selfIndex); } } - return res; + return _optimizeSkips(dstRecords); } -function _selfRecord(r: ProtoRecord, contextIndex: number, selfIndex: number): ProtoRecord { +/** + * - Conditional skip of 1 record followed by an unconditional skip of N are replaced by a + * conditional skip of N with the negated condition, + * - Skips of 0 records are removed + */ +function _optimizeSkips(srcRecords: ProtoRecord[]): ProtoRecord[] { + let dstRecords = []; + let skipSources = ListWrapper.createFixedSize(srcRecords.length); + let indexMap: Map = new Map(); + + for (let protoIndex = 0; protoIndex < srcRecords.length; protoIndex++) { + let skipRecord = skipSources[protoIndex]; + if (isPresent(skipRecord)) { + skipRecord.fixedArgs[0] = dstRecords.length; + } + + let src = srcRecords[protoIndex]; + + if (src.isSkipRecord()) { + if (src.isConditionalSkipRecord() && src.fixedArgs[0] === protoIndex + 2 && + protoIndex < srcRecords.length - 1 && + srcRecords[protoIndex + 1].mode === RecordType.SkipRecords) { + src.mode = src.mode === RecordType.SkipRecordsIf ? RecordType.SkipRecordsIfNot : + RecordType.SkipRecordsIf; + src.fixedArgs[0] = srcRecords[protoIndex + 1].fixedArgs[0]; + protoIndex++; + } + + if (src.fixedArgs[0] > protoIndex + 1) { + let dst = _cloneAndUpdateIndexes(src, dstRecords, indexMap); + dstRecords.push(dst); + skipSources[dst.fixedArgs[0]] = dst; + } + + } else { + let dst = _cloneAndUpdateIndexes(src, dstRecords, indexMap); + dstRecords.push(dst); + indexMap.set(src.selfIndex, dst.selfIndex); + } + } + + return dstRecords; +} + +/** + * Add a new record or re-use one of the existing records. + */ +function _mayBeAddRecord(record: ProtoRecord, dstRecords: ProtoRecord[], excludedIdxs: number[], + excluded: boolean): ProtoRecord { + let match = _findFirstMatch(record, dstRecords, excludedIdxs); + + if (isPresent(match)) { + if (record.lastInBinding) { + dstRecords.push(_createSelfRecord(record, match.selfIndex, dstRecords.length + 1)); + match.referencedBySelf = true; + } else { + if (record.argumentToPureFunction) { + match.argumentToPureFunction = true; + } + } + + return match; + } + + if (excluded) { + excludedIdxs.push(record.selfIndex); + } + + dstRecords.push(record); + return record; +} + +/** + * Returns the first `ProtoRecord` that matches the record. + */ +function _findFirstMatch(record: ProtoRecord, dstRecords: ProtoRecord[], + excludedIdxs: number[]): ProtoRecord { + return ListWrapper.find( + dstRecords, + // TODO(vicb): optimize notReusableIndexes.indexOf (sorted array) + rr => excludedIdxs.indexOf(rr.selfIndex) == -1 && rr.mode !== RecordType.DirectiveLifecycle && + _haveSameDirIndex(rr, record) && rr.mode === record.mode && + looseIdentical(rr.funcOrValue, record.funcOrValue) && + rr.contextIndex === record.contextIndex && looseIdentical(rr.name, record.name) && + ListWrapper.equals(rr.args, record.args)); +} + +/** + * Clone the `ProtoRecord` and changes the indexes for the ones in the destination array for: + * - the arguments, + * - the context, + * - self + */ +function _cloneAndUpdateIndexes(record: ProtoRecord, dstRecords: ProtoRecord[], + indexMap: Map): ProtoRecord { + let args = record.args.map(src => _srcToDstSelfIndex(indexMap, src)); + let contextIndex = _srcToDstSelfIndex(indexMap, record.contextIndex); + let selfIndex = dstRecords.length + 1; + + return new ProtoRecord(record.mode, record.name, record.funcOrValue, args, record.fixedArgs, + contextIndex, record.directiveIndex, selfIndex, record.bindingRecord, + record.lastInBinding, record.lastInDirective, + record.argumentToPureFunction, record.referencedBySelf, + record.propertyBindingIndex); +} + +/** + * Returns the index in the destination array corresponding to the index in the src array. + * When the element is not present in the destination array, return the source index. + */ +function _srcToDstSelfIndex(indexMap: Map, srcIdx: number): number { + var dstIdx = indexMap.get(srcIdx); + return isPresent(dstIdx) ? dstIdx : srcIdx; +} + +function _createSelfRecord(r: ProtoRecord, contextIndex: number, selfIndex: number): ProtoRecord { return new ProtoRecord(RecordType.Self, "self", null, [], r.fixedArgs, contextIndex, r.directiveIndex, selfIndex, r.bindingRecord, r.lastInBinding, r.lastInDirective, false, false, r.propertyBindingIndex); } -function _findMatching(r: ProtoRecord, rs: ProtoRecord[]) { - return ListWrapper.find( - rs, (rr) => rr.mode !== RecordType.DirectiveLifecycle && _sameDirIndex(rr, r) && - rr.mode === r.mode && looseIdentical(rr.funcOrValue, r.funcOrValue) && - rr.contextIndex === r.contextIndex && StringWrapper.equals(rr.name, r.name) && - ListWrapper.equals(rr.args, r.args)); -} - -function _sameDirIndex(a: ProtoRecord, b: ProtoRecord): boolean { +function _haveSameDirIndex(a: ProtoRecord, b: ProtoRecord): boolean { var di1 = isBlank(a.directiveIndex) ? null : a.directiveIndex.directiveIndex; var ei1 = isBlank(a.directiveIndex) ? null : a.directiveIndex.elementIndex; @@ -66,17 +173,3 @@ function _sameDirIndex(a: ProtoRecord, b: ProtoRecord): boolean { return di1 === di2 && ei1 === ei2; } - -function _replaceIndices(r: ProtoRecord, selfIndex: number, indexMap: Map) { - var args = r.args.map(a => _map(indexMap, a)); - var contextIndex = _map(indexMap, r.contextIndex); - return new ProtoRecord(r.mode, r.name, r.funcOrValue, args, r.fixedArgs, contextIndex, - r.directiveIndex, selfIndex, r.bindingRecord, r.lastInBinding, - r.lastInDirective, r.argumentToPureFunction, r.referencedBySelf, - r.propertyBindingIndex); -} - -function _map(indexMap: Map, value: number) { - var r = indexMap.get(value); - return isPresent(r) ? r : value; -} diff --git a/modules/angular2/src/core/change_detection/dynamic_change_detector.ts b/modules/angular2/src/core/change_detection/dynamic_change_detector.ts index 361b558c06..7842da7935 100644 --- a/modules/angular2/src/core/change_detection/dynamic_change_detector.ts +++ b/modules/angular2/src/core/change_detection/dynamic_change_detector.ts @@ -54,20 +54,43 @@ export class DynamicChangeDetector extends AbstractChangeDetector { var values = ListWrapper.createFixedSize(eb.records.length); values[0] = this.values[0]; - for (var i = 0; i < eb.records.length; ++i) { - var proto = eb.records[i]; - var res = this._calculateCurrValue(proto, values, locals); - if (proto.lastInBinding) { - this._markPathAsCheckOnce(proto); - return res; + for (var protoIdx = 0; protoIdx < eb.records.length; ++protoIdx) { + var proto = eb.records[protoIdx]; + + if (proto.isSkipRecord()) { + protoIdx += this._computeSkipLength(protoIdx, proto, values); } else { - this._writeSelf(proto, res, values); + var res = this._calculateCurrValue(proto, values, locals); + if (proto.lastInBinding) { + this._markPathAsCheckOnce(proto); + return res; + } else { + this._writeSelf(proto, res, values); + } } } throw new BaseException("Cannot be reached"); } + private _computeSkipLength(protoIndex: number, proto: ProtoRecord, values: any[]): number { + if (proto.mode === RecordType.SkipRecords) { + return proto.fixedArgs[0] - protoIndex - 1; + } + + if (proto.mode === RecordType.SkipRecordsIf) { + let condition = this._readContext(proto, values); + return condition ? proto.fixedArgs[0] - protoIndex - 1 : 0; + } + + if (proto.mode === RecordType.SkipRecordsIfNot) { + let condition = this._readContext(proto, values); + return condition ? 0 : proto.fixedArgs[0] - protoIndex - 1; + } + + throw new BaseException("Cannot be reached"); + } + /** @internal */ _markPathAsCheckOnce(proto: ProtoRecord): void { if (!proto.bindingRecord.isDefaultChangeDetection()) { @@ -122,8 +145,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector { var changes = null; var isChanged = false; - for (var i = 0; i < protos.length; ++i) { - var proto: ProtoRecord = protos[i]; + for (var protoIdx = 0; protoIdx < protos.length; ++protoIdx) { + var proto: ProtoRecord = protos[protoIdx]; var bindingRecord = proto.bindingRecord; var directiveRecord = bindingRecord.directiveRecord; @@ -140,7 +163,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector { } else if (proto.name === "OnChanges" && isPresent(changes) && !throwOnChange) { this._getDirectiveFor(directiveRecord.directiveIndex).onChanges(changes); } - + } else if (proto.isSkipRecord()) { + protoIdx += this._computeSkipLength(protoIdx, proto, this.values); } else { var change = this._check(proto, throwOnChange, this.values, this.locals); if (isPresent(change)) { diff --git a/modules/angular2/src/core/change_detection/proto_change_detector.ts b/modules/angular2/src/core/change_detection/proto_change_detector.ts index 15f93f59da..d3f489e289 100644 --- a/modules/angular2/src/core/change_detection/proto_change_detector.ts +++ b/modules/angular2/src/core/change_detection/proto_change_detector.ts @@ -226,9 +226,28 @@ class _ConvertAstIntoProtoRecords implements AstVisitor { visitBinary(ast: Binary): number { var left = ast.left.visit(this); - var right = ast.right.visit(this); - return this._addRecord(RecordType.PrimitiveOp, _operationToPrimitiveName(ast.operation), - _operationToFunction(ast.operation), [left, right], null, 0); + switch (ast.operation) { + case '&&': + var branchEnd = [null]; + this._addRecord(RecordType.SkipRecordsIfNot, "SkipRecordsIfNot", null, [], branchEnd, left); + var right = ast.right.visit(this); + branchEnd[0] = right; + return this._addRecord(RecordType.PrimitiveOp, "cond", ChangeDetectionUtil.cond, + [left, right, left], null, 0); + + case '||': + var branchEnd = [null]; + this._addRecord(RecordType.SkipRecordsIf, "SkipRecordsIf", null, [], branchEnd, left); + var right = ast.right.visit(this); + branchEnd[0] = right; + return this._addRecord(RecordType.PrimitiveOp, "cond", ChangeDetectionUtil.cond, + [left, left, right], null, 0); + + default: + var right = ast.right.visit(this); + return this._addRecord(RecordType.PrimitiveOp, _operationToPrimitiveName(ast.operation), + _operationToFunction(ast.operation), [left, right], null, 0); + } } visitPrefixNot(ast: PrefixNot): number { @@ -238,11 +257,20 @@ class _ConvertAstIntoProtoRecords implements AstVisitor { } visitConditional(ast: Conditional): number { - var c = ast.condition.visit(this); - var t = ast.trueExp.visit(this); - var f = ast.falseExp.visit(this); - return this._addRecord(RecordType.PrimitiveOp, "cond", ChangeDetectionUtil.cond, [c, t, f], - null, 0); + var condition = ast.condition.visit(this); + var startOfFalseBranch = [null]; + var endOfFalseBranch = [null]; + this._addRecord(RecordType.SkipRecordsIfNot, "SkipRecordsIfNot", null, [], startOfFalseBranch, + condition); + var whenTrue = ast.trueExp.visit(this); + var skip = + this._addRecord(RecordType.SkipRecords, "SkipRecords", null, [], endOfFalseBranch, 0); + var whenFalse = ast.falseExp.visit(this); + startOfFalseBranch[0] = skip; + endOfFalseBranch[0] = whenFalse; + + return this._addRecord(RecordType.PrimitiveOp, "cond", ChangeDetectionUtil.cond, + [condition, whenTrue, whenFalse], null, 0); } visitPipe(ast: BindingPipe): number { @@ -350,10 +378,6 @@ function _operationToPrimitiveName(operation: string): string { return "operation_less_or_equals_then"; case '>=': return "operation_greater_or_equals_then"; - case '&&': - return "operation_logical_and"; - case '||': - return "operation_logical_or"; default: throw new BaseException(`Unsupported operation ${operation}`); } @@ -387,10 +411,6 @@ function _operationToFunction(operation: string): Function { return ChangeDetectionUtil.operation_less_or_equals_then; case '>=': return ChangeDetectionUtil.operation_greater_or_equals_then; - case '&&': - return ChangeDetectionUtil.operation_logical_and; - case '||': - return ChangeDetectionUtil.operation_logical_or; default: throw new BaseException(`Unsupported operation ${operation}`); } diff --git a/modules/angular2/src/core/change_detection/proto_record.ts b/modules/angular2/src/core/change_detection/proto_record.ts index e4c45a6940..f3fd76f906 100644 --- a/modules/angular2/src/core/change_detection/proto_record.ts +++ b/modules/angular2/src/core/change_detection/proto_record.ts @@ -18,7 +18,10 @@ export enum RecordType { CollectionLiteral, SafeMethodInvoke, DirectiveLifecycle, - Chain + Chain, + SkipRecordsIf, // Skip records when the condition is true + SkipRecordsIfNot, // Skip records when the condition is false + SkipRecords // Skip records unconditionally } export class ProtoRecord { @@ -42,5 +45,15 @@ export class ProtoRecord { isPipeRecord(): boolean { return this.mode === RecordType.Pipe; } + isConditionalSkipRecord(): boolean { + return this.mode === RecordType.SkipRecordsIfNot || this.mode === RecordType.SkipRecordsIf; + } + + isUnconditionalSkipRecord(): boolean { return this.mode === RecordType.SkipRecords; } + + isSkipRecord(): boolean { + return this.isConditionalSkipRecord() || this.isUnconditionalSkipRecord(); + } + isLifeCycleRecord(): boolean { return this.mode === RecordType.DirectiveLifecycle; } } diff --git a/modules/angular2/test/core/change_detection/change_detector_config.ts b/modules/angular2/test/core/change_detection/change_detector_config.ts index 8aeba561f5..bc9da2bfa2 100644 --- a/modules/angular2/test/core/change_detection/change_detector_config.ts +++ b/modules/angular2/test/core/change_detection/change_detector_config.ts @@ -417,7 +417,14 @@ var _availableDefinitions = [ 'a.sayHi("Jim")', 'passThrough([12])', 'invalidFn(1)', - 'age' + 'age', + 'true ? city : zipcode', + 'false ? city : zipcode', + 'getTrue() && getTrue()', + 'getFalse() && getTrue()', + 'getFalse() || getFalse()', + 'getTrue() || getFalse()', + 'name == "Victor" ? (true ? address.city : address.zipcode) : address.zipcode' ]; var _availableEventDefinitions = [ @@ -427,7 +434,8 @@ var _availableEventDefinitions = [ // '(event)="\$event=1"', '(event)="a=a+1; a=a+1;"', '(event)="false"', - '(event)="true"' + '(event)="true"', + '(event)="true ? a = a + 1 : a = a + 1"', ]; var _availableHostEventDefinitions = ['(host-event)="onEvent(\$event)"']; diff --git a/modules/angular2/test/core/change_detection/change_detector_spec.ts b/modules/angular2/test/core/change_detection/change_detector_spec.ts index a516c17677..ee509bc677 100644 --- a/modules/angular2/test/core/change_detection/change_detector_spec.ts +++ b/modules/angular2/test/core/change_detection/change_detector_spec.ts @@ -111,6 +111,54 @@ export function main() { return val.dispatcher.log; } + describe('short-circuit', () => { + it('should support short-circuit for the ternary operator', () => { + var address = new Address('Sunnyvale', '94085'); + expect(_bindSimpleValue('true ? city : zipcode', address)) + .toEqual(['propName=Sunnyvale']); + expect(address.cityGetterCalls).toEqual(1); + expect(address.zipCodeGetterCalls).toEqual(0); + + address = new Address('Sunnyvale', '94085'); + expect(_bindSimpleValue('false ? city : zipcode', address)).toEqual(['propName=94085']); + expect(address.cityGetterCalls).toEqual(0); + expect(address.zipCodeGetterCalls).toEqual(1); + }); + + it('should support short-circuit for the && operator', () => { + var logical = new Logical(); + expect(_bindSimpleValue('getTrue() && getTrue()', logical)).toEqual(['propName=true']); + expect(logical.trueCalls).toEqual(2); + + logical = new Logical(); + expect(_bindSimpleValue('getFalse() && getTrue()', logical)).toEqual(['propName=false']); + expect(logical.falseCalls).toEqual(1); + expect(logical.trueCalls).toEqual(0); + }); + + it('should support short-circuit for the || operator', () => { + var logical = new Logical(); + expect(_bindSimpleValue('getFalse() || getFalse()', logical)).toEqual(['propName=false']); + expect(logical.falseCalls).toEqual(2); + + logical = new Logical(); + expect(_bindSimpleValue('getTrue() || getFalse()', logical)).toEqual(['propName=true']); + expect(logical.falseCalls).toEqual(0); + expect(logical.trueCalls).toEqual(1); + }); + + it('should support nested short-circuits', () => { + var address = new Address('Sunnyvale', '94085'); + var person = new Person('Victor', address); + expect(_bindSimpleValue( + 'name == "Victor" ? (true ? address.city : address.zipcode) : address.zipcode', + person)) + .toEqual(['propName=Sunnyvale']); + expect(address.cityGetterCalls).toEqual(1); + expect(address.zipCodeGetterCalls).toEqual(0); + }); + }); + it('should support literals', () => { expect(_bindSimpleValue('10')).toEqual(['propName=10']); }); @@ -1299,6 +1347,13 @@ export function main() { res = val.changeDetector.handleEvent("event", 0, locals); expect(res).toBe(false); }); + + it('should support short-circuiting', () => { + d.a = 0; + var val = _createChangeDetector('(event)="true ? a = a + 1 : a = a + 1"', d, null); + val.changeDetector.handleEvent("event", 0, locals); + expect(d.a).toEqual(1); + }); }); }); }); @@ -1417,11 +1472,43 @@ class Person { } class Address { - constructor(public city: string) {} + cityGetterCalls: number = 0; + zipCodeGetterCalls: number = 0; + + constructor(public _city: string, public _zipcode = null) {} + + get city() { + this.cityGetterCalls++; + return this._city; + } + + get zipcode() { + this.zipCodeGetterCalls++; + return this._zipcode; + } + + set city(v) { this._city = v; } + + set zipcode(v) { this._zipcode = v; } toString(): string { return isBlank(this.city) ? '-' : this.city } } +class Logical { + trueCalls: number = 0; + falseCalls: number = 0; + + getTrue() { + this.trueCalls++; + return true; + } + + getFalse() { + this.falseCalls++; + return false; + } +} + class Uninitialized { value: any; } diff --git a/modules/angular2/test/core/change_detection/coalesce_spec.ts b/modules/angular2/test/core/change_detection/coalesce_spec.ts index ec6f5b03f4..9bfd24db77 100644 --- a/modules/angular2/test/core/change_detection/coalesce_spec.ts +++ b/modules/angular2/test/core/change_detection/coalesce_spec.ts @@ -16,28 +16,29 @@ import {DirectiveIndex} from 'angular2/src/core/change_detection/directive_recor export function main() { function r(funcOrValue, args, contextIndex, selfIndex, - {lastInBinding, mode, name, directiveIndex, argumentToPureFunction}: { + {lastInBinding, mode, name, directiveIndex, argumentToPureFunction, fixedArgs}: { lastInBinding?: any, mode?: any, name?: any, directiveIndex?: any, - argumentToPureFunction?: boolean + argumentToPureFunction?: boolean, + fixedArgs?: any[] } = {}) { if (isBlank(lastInBinding)) lastInBinding = false; if (isBlank(mode)) mode = RecordType.PropertyRead; if (isBlank(name)) name = "name"; if (isBlank(directiveIndex)) directiveIndex = null; if (isBlank(argumentToPureFunction)) argumentToPureFunction = false; + if (isBlank(fixedArgs)) fixedArgs = null; - return new ProtoRecord(mode, name, funcOrValue, args, null, contextIndex, directiveIndex, + return new ProtoRecord(mode, name, funcOrValue, args, fixedArgs, contextIndex, directiveIndex, selfIndex, null, lastInBinding, false, argumentToPureFunction, false, 0); } describe("change detection - coalesce", () => { it("should work with an empty list", () => { expect(coalesce([])).toEqual([]); }); - it("should remove non-terminal duplicate records" + - " and update the context indices referencing them", + it("should remove non-terminal duplicate records and update the context indices referencing them", () => { var rs = coalesce( [r("user", [], 0, 1), r("first", [], 1, 2), r("user", [], 0, 3), r("last", [], 3, 4)]); @@ -52,8 +53,7 @@ export function main() { expect(rs).toEqual([r("dup", [], 0, 1), r("user", [], 0, 2), r("first", [2], 2, 3)]); }); - it("should remove non-terminal duplicate records" + - " and update the args indices referencing them", + it("should remove non-terminal duplicate records and update the args indices referencing them", () => { var rs = coalesce([ r("user1", [], 0, 1), @@ -132,5 +132,104 @@ export function main() { expect(rs) .toEqual([r("user", [], 0, 1, {argumentToPureFunction: true}), r("name", [], 1, 2)]); }); + + describe('short-circuit', () => { + it('should not use short-circuitable records', () => { + var records = [ + r("sknot", [], 0, 1, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [3]}), + r("a", [], 0, 2), + r("sk", [], 0, 3, {mode: RecordType.SkipRecords, fixedArgs: [4]}), + r("b", [], 0, 4), + r("cond", [2, 4], 0, 5), + r("a", [], 0, 6), + r("b", [], 0, 7), + ]; + + expect(coalesce(records)).toEqual(records); + }); + + it('should not use short-circuitable records from nested short-circuits', () => { + var records = [ + r("sknot outer", [], 0, 1, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [7]}), + r("sknot inner", [], 0, 2, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [4]}), + r("a", [], 0, 3), + r("sk inner", [], 0, 4, {mode: RecordType.SkipRecords, fixedArgs: [5]}), + r("b", [], 0, 5), + r("cond-inner", [3, 5], 0, 6), + r("sk outer", [], 0, 7, {mode: RecordType.SkipRecords, fixedArgs: [8]}), + r("c", [], 0, 8), + r("cond-outer", [6, 8], 0, 9), + r("a", [], 0, 10), + r("b", [], 0, 11), + r("c", [], 0, 12), + ]; + + expect(coalesce(records)).toEqual(records); + }); + + it('should collapse the true branch', () => { + var rs = coalesce([ + r("a", [], 0, 1), + r("sknot", [], 0, 2, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [4]}), + r("a", [], 0, 3), + r("sk", [], 0, 4, {mode: RecordType.SkipRecords, fixedArgs: [6]}), + r("a", [], 0, 5), + r("b", [], 5, 6), + r("cond", [3, 6], 0, 7), + ]); + + expect(rs).toEqual([ + r("a", [], 0, 1), + r("sknot", [], 0, 2, {mode: RecordType.SkipRecordsIf, fixedArgs: [3]}), + r("b", [], 1, 3), + r("cond", [1, 3], 0, 4), + ]); + }); + + it('should collapse the false branch', () => { + var rs = coalesce([ + r("a", [], 0, 1), + r("sknot", [], 0, 2, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [5]}), + r("a", [], 0, 3), + r("b", [], 3, 4), + r("sk", [], 0, 5, {mode: RecordType.SkipRecords, fixedArgs: [6]}), + r("a", [], 0, 6), + r("cond", [4, 6], 0, 7), + ]); + + expect(rs).toEqual([ + r("a", [], 0, 1), + r("sknot", [], 0, 2, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [3]}), + r("b", [], 1, 3), + r("cond", [3, 1], 0, 4), + ]); + }); + + it('should optimize skips', () => { + var rs = coalesce([ + // skipIfNot(1) + skip(N) -> skipIf(+N) + r("sknot", [], 0, 1, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [2]}), + r("sk", [], 0, 2, {mode: RecordType.SkipRecords, fixedArgs: [3]}), + r("a", [], 0, 3), + // skipIf(1) + skip(N) -> skipIfNot(N) + r("skif", [], 0, 4, {mode: RecordType.SkipRecordsIf, fixedArgs: [5]}), + r("sk", [], 0, 5, {mode: RecordType.SkipRecords, fixedArgs: [6]}), + r("b", [], 0, 6), + // remove empty skips + r("sknot", [], 0, 7, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [7]}), + r("skif", [], 0, 8, {mode: RecordType.SkipRecordsIf, fixedArgs: [8]}), + r("sk", [], 0, 9, {mode: RecordType.SkipRecords, fixedArgs: [9]}), + r("end", [], 0, 10), + ]); + + expect(rs).toEqual([ + r("sknot", [], 0, 1, {mode: RecordType.SkipRecordsIf, fixedArgs: [2]}), + r("a", [], 0, 2), + r("skif", [], 0, 3, {mode: RecordType.SkipRecordsIfNot, fixedArgs: [4]}), + r("b", [], 0, 4), + r("end", [], 0, 5), + ]); + }); + }); }); } diff --git a/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart b/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart index 90424dd079..3709838d36 100644 --- a/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart +++ b/modules_dart/transform/lib/src/transform/template_compiler/change_detector_codegen.dart @@ -14,6 +14,7 @@ import 'package:angular2/src/core/change_detection/binding_record.dart'; import 'package:angular2/src/core/change_detection/codegen_facade.dart' show codify; import 'package:angular2/src/core/facade/exceptions.dart' show BaseException; +import 'package:angular2/src/core/facade/collection.dart' show ListWrapper; /// Responsible for generating change detector classes for Angular 2. /// @@ -90,6 +91,7 @@ class _CodegenState { final CodegenNameUtil _names; final ChangeDetectorGenConfig _genConfig; final List _propertyBindingTargets; + final List _endOfBlockIdxs = []; String get _changeDetectionStrategyAsCode => _changeDetectionStrategy == null ? 'null' @@ -156,7 +158,7 @@ class _CodegenState { var $_IS_CHANGED_LOCAL = false; var $_CHANGES_LOCAL = null; - ${_records.map(_genRecord).join('')} + ${_genAllRecords()} } ${_maybeGenHandleEventInternal()} @@ -186,6 +188,15 @@ class _CodegenState { '''); } + String _genAllRecords() { + _endOfBlockIdxs.clear(); + List res = []; + for (int i = 0; i < _records.length; i++) { + res.add(_genRecord(_records[i], i)); + } + return res.join(''); + } + String _genPropertyBindingTargets() { var targets = _logic.genPropertyBindingTargets( _propertyBindingTargets, this._genConfig.genDebugInfo); @@ -215,10 +226,29 @@ class _CodegenState { } String _genEventBinding(EventBinding eb) { - var recs = eb.records.map((r) => _genEventBindingEval(eb, r)).join("\n"); + List codes = []; + _endOfBlockIdxs.clear(); + + ListWrapper.forEachWithIndex(eb.records, (r, i) { + var code; + var r = eb.records[i]; + + if (r.isConditionalSkipRecord()) { + code = _genConditionalSkip(r, _names.getEventLocalName(eb, i)); + } else if (r.isUnconditionalSkipRecord()) { + code = _genUnconditionalSkip(r); + } else { + code = _genEventBindingEval(eb, r); + } + + code += this._genEndOfSkipBlock(i); + + codes.add(code); + }); + return ''' if (eventName == "${eb.eventName}" && elIndex == ${eb.elIndex}) { - ${recs} + ${codes.join("\n")} }'''; } @@ -315,20 +345,53 @@ class _CodegenState { return 'var ${declareNames.join(', ')};'; } - String _genRecord(ProtoRecord r) { - var rec = null; + String _genRecord(ProtoRecord r, int index) { + var code = null; if (r.isLifeCycleRecord()) { - rec = _genDirectiveLifecycle(r); + code = _genDirectiveLifecycle(r); } else if (r.isPipeRecord()) { - rec = _genPipeCheck(r); + code = _genPipeCheck(r); + } else if (r.isConditionalSkipRecord()) { + code = _genConditionalSkip(r, _names.getLocalName(r.contextIndex)); + } else if (r.isUnconditionalSkipRecord()) { + code = _genUnconditionalSkip(r); } else { - rec = _genReferenceCheck(r); + code = _genReferenceCheck(r); } - return ''' + + code = ''' ${this._maybeFirstInBinding(r)} - ${rec} + ${code} ${this._maybeGenLastInDirective(r)} + ${this._genEndOfSkipBlock(index)} '''; + + return code; + } + + String _genConditionalSkip(ProtoRecord r, String condition) { + var maybeNegate = r.mode == RecordType.SkipRecordsIf ? '!' : ''; + _endOfBlockIdxs.add(r.fixedArgs[0] - 1); + + return 'if ($maybeNegate$condition) {'; + } + + String _genUnconditionalSkip(ProtoRecord r) { + _endOfBlockIdxs.removeLast(); + _endOfBlockIdxs.add(r.fixedArgs[0] - 1); + return '} else {'; + } + + String _genEndOfSkipBlock(int protoIndex) { + if (!ListWrapper.isEmpty(this._endOfBlockIdxs)) { + var endOfBlock = ListWrapper.last(this._endOfBlockIdxs); + if (protoIndex == endOfBlock) { + this._endOfBlockIdxs.removeLast(); + return '}'; + } + } + + return ''; } String _genDirectiveLifecycle(ProtoRecord r) {