From 71ea19902aa9989d212f636828b83683051d4c92 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Thu, 30 Jul 2015 10:59:52 -0700 Subject: [PATCH] perf(change_detection): removed the currentProto property --- .../abstract_change_detector.ts | 53 +++++++++++++++---- .../change_detection_jit_generator.ts | 32 +++++------ .../change_detection/change_detection_util.ts | 21 +++----- .../src/change_detection/codegen_name_util.ts | 6 ++- .../dynamic_change_detector.ts | 35 ++++++------ .../change_detector_codegen.dart | 34 +++++------- .../expected/bar.ng_deps.dart | 11 ++-- .../template_compiler/all_tests.dart | 4 +- 8 files changed, 105 insertions(+), 91 deletions(-) diff --git a/modules/angular2/src/change_detection/abstract_change_detector.ts b/modules/angular2/src/change_detection/abstract_change_detector.ts index 8f365cbee7..644cafd8d7 100644 --- a/modules/angular2/src/change_detection/abstract_change_detector.ts +++ b/modules/angular2/src/change_detection/abstract_change_detector.ts @@ -1,11 +1,16 @@ -import {isPresent, BaseException} from 'angular2/src/facade/lang'; +import {isPresent, isBlank, BaseException} from 'angular2/src/facade/lang'; import {List, ListWrapper} from 'angular2/src/facade/collection'; import {ChangeDetectionUtil} from './change_detection_util'; import {ChangeDetectorRef} from './change_detector_ref'; import {DirectiveRecord} from './directive_record'; import {ChangeDetector, ChangeDispatcher} from './interfaces'; -import {ChangeDetectionError} from './exceptions'; +import { + ChangeDetectionError, + ExpressionChangedAfterItHasBeenCheckedException, + DehydratedException +} from './exceptions'; import {ProtoRecord} from './proto_record'; +import {BindingRecord} from './binding_record'; import {Locals} from './parser/locals'; import {Pipes} from './pipes/pipes'; import {CHECK_ALWAYS, CHECK_ONCE, CHECKED, DETACHED, ON_PUSH} from './constants'; @@ -26,12 +31,12 @@ export class AbstractChangeDetector implements ChangeDetector { // change detection will fail. alreadyChecked: any = false; context: T; - currentProto: ProtoRecord = null; directiveRecords: List; dispatcher: ChangeDispatcher; locals: Locals = null; mode: string = null; pipes: Pipes = null; + firstProtoInCurrentBinding: number; protos: List; constructor(public id: string, dispatcher: ChangeDispatcher, protos: List, @@ -80,24 +85,25 @@ export class AbstractChangeDetector implements ChangeDetector { // implementation of `detectChangesInRecordsInternal` which does the work of detecting changes // and which this method will call. // This method expects that `detectChangesInRecordsInternal` will set the property - // `this.currentProto` to whichever [ProtoRecord] is currently being processed. This is to + // `this.firstProtoInCurrentBinding` to the selfIndex of the first proto record. This is to // facilitate error reporting. detectChangesInRecords(throwOnChange: boolean): void { if (!this.hydrated()) { - ChangeDetectionUtil.throwDehydrated(); + this.throwDehydratedError(); } try { this.detectChangesInRecordsInternal(throwOnChange); } catch (e) { - this.throwError(this.currentProto, e, e.stack); + this._throwError(e, e.stack); } } // Subclasses should override this method to perform any work necessary to detect and report // changes. For example, changes should be reported via `ChangeDetectionUtil.addChange`, lifecycle // methods should be called, etc. - // This implementation should also set `this.currentProto` to whichever [ProtoRecord] is - // currently being processed to facilitate error reporting. See {@link #detectChangesInRecords}. + // This implementation should also set `this.firstProtoInCurrentBinding` to the selfIndex of the + // first proto record + // to facilitate error reporting. See {@link #detectChangesInRecords}. detectChangesInRecordsInternal(throwOnChange: boolean): void {} // This method is not intended to be overridden. Subclasses should instead provide an @@ -155,11 +161,40 @@ export class AbstractChangeDetector implements ChangeDetector { } } - throwError(proto: ProtoRecord, exception: any, stack: any): void { + protected notifyDispatcher(value: any): void { + this.dispatcher.notifyOnBinding(this._currentBinding(), value); + } + + protected addChange(changes: StringMap, oldValue: any, + newValue: any): StringMap { + if (isBlank(changes)) { + changes = {}; + } + changes[this._currentBinding().propertyName] = + ChangeDetectionUtil.simpleChange(oldValue, newValue); + return changes; + } + + private _throwError(exception: any, stack: any): void { + var proto = this._currentBindingProto(); var c = this.dispatcher.getDebugContext(proto.bindingRecord.elementIndex, proto.directiveIndex); var context = isPresent(c) ? new _Context(c.element, c.componentElement, c.directive, c.context, c.locals, c.injector, proto.expressionAsString) : null; throw new ChangeDetectionError(proto, exception, stack, context); } + + protected throwOnChangeError(oldValue: any, newValue: any): void { + var change = ChangeDetectionUtil.simpleChange(oldValue, newValue); + throw new ExpressionChangedAfterItHasBeenCheckedException(this._currentBindingProto(), change, + null); + } + + protected throwDehydratedError(): void { throw new DehydratedException(); } + + private _currentBinding(): BindingRecord { return this._currentBindingProto().bindingRecord; } + + private _currentBindingProto(): ProtoRecord { + return ChangeDetectionUtil.protoByIndex(this.protos, this.firstProtoInCurrentBinding); + } } diff --git a/modules/angular2/src/change_detection/change_detection_jit_generator.ts b/modules/angular2/src/change_detection/change_detection_jit_generator.ts index 6d2548981a..3d74321ecc 100644 --- a/modules/angular2/src/change_detection/change_detection_jit_generator.ts +++ b/modules/angular2/src/change_detection/change_detection_jit_generator.ts @@ -46,8 +46,6 @@ export class ChangeDetectorJITGenerator { ${this._typeName}.prototype = Object.create(${ABSTRACT_CHANGE_DETECTOR}.prototype); ${this._typeName}.prototype.detectChangesInRecordsInternal = function(throwOnChange) { - ${this._names.getCurrentProtoName()} = null; - ${this._names.genInitLocals()} var ${IS_CHANGED_LOCAL} = false; var ${CHANGES_LOCAL} = null; @@ -149,7 +147,11 @@ export class ChangeDetectorJITGenerator { } else { rec = this._genReferenceCheck(r); } - return `${rec}${this._maybeGenLastInDirective(r)}`; + return ` + ${this._maybeFirstInBinding(r)} + ${rec} + ${this._maybeGenLastInDirective(r)} + `; } _genDirectiveLifecycle(r: ProtoRecord): string { @@ -173,12 +175,9 @@ export class ChangeDetectorJITGenerator { var pipe = this._names.getPipeName(r.selfIndex); var cdRef = "this.ref"; - - var protoIndex = r.selfIndex - 1; var pipeType = r.name; var read = ` - ${this._names.getCurrentProtoName()} = ${this._names.getProtosName()}[${protoIndex}]; if (${pipe} === ${UTIL}.uninitialized) { ${pipe} = ${this._names.getPipesAccessorName()}.get('${pipeType}', ${context}, ${cdRef}); } else if (!${pipe}.supports(${context})) { @@ -204,11 +203,7 @@ export class ChangeDetectorJITGenerator { _genReferenceCheck(r: ProtoRecord): string { var oldValue = this._names.getFieldName(r.selfIndex); var newValue = this._names.getLocalName(r.selfIndex); - - var protoIndex = r.selfIndex - 1; - var read = ` - ${this._names.getCurrentProtoName()} = ${this._names.getProtosName()}[${protoIndex}]; ${this._genUpdateCurrentValue(r)} `; @@ -331,8 +326,7 @@ export class ChangeDetectorJITGenerator { } else { return ` ${this._genThrowOnChangeCheck(oldValue, newValue)} - ${this._names.getDispatcherName()}.notifyOnBinding( - ${this._names.getCurrentProtoName()}.bindingRecord, ${newValue}); + this.notifyDispatcher(${newValue}); `; } } @@ -341,7 +335,7 @@ export class ChangeDetectorJITGenerator { if (this.generateCheckNoChanges) { return ` if(throwOnChange) { - ${UTIL}.throwOnChange(${this._names.getCurrentProtoName()}, ${UTIL}.simpleChange(${oldValue}, ${newValue})); + this.throwOnChangeError(${oldValue}, ${newValue}); } `; } else { @@ -361,11 +355,13 @@ export class ChangeDetectorJITGenerator { var newValue = this._names.getLocalName(r.selfIndex); var oldValue = this._names.getFieldName(r.selfIndex); if (!r.bindingRecord.callOnChange()) return ""; - return ` - ${CHANGES_LOCAL} = ${UTIL}.addChange( - ${CHANGES_LOCAL}, ${this._names.getCurrentProtoName()}.bindingRecord.propertyName, - ${UTIL}.simpleChange(${oldValue}, ${newValue})); - `; + return `${CHANGES_LOCAL} = this.addChange(${CHANGES_LOCAL}, ${oldValue}, ${newValue});`; + } + + _maybeFirstInBinding(r: ProtoRecord): string { + var prev = ChangeDetectionUtil.protoByIndex(this.records, r.selfIndex - 1); + var firstInBindng = isBlank(prev) || prev.bindingRecord !== r.bindingRecord; + return firstInBindng ? `${this._names.getFirstProtoInCurrentBinding()} = ${r.selfIndex};` : ''; } _maybeGenLastInDirective(r: ProtoRecord): string { diff --git a/modules/angular2/src/change_detection/change_detection_util.ts b/modules/angular2/src/change_detection/change_detection_util.ts index 9881e82da7..d924ecea9d 100644 --- a/modules/angular2/src/change_detection/change_detection_util.ts +++ b/modules/angular2/src/change_detection/change_detection_util.ts @@ -1,7 +1,6 @@ import {CONST_EXPR, isPresent, isBlank, BaseException, Type} from 'angular2/src/facade/lang'; import {List, ListWrapper, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection'; import {ProtoRecord} from './proto_record'; -import {DehydratedException, ExpressionChangedAfterItHasBeenCheckedException} from './exceptions'; import {WrappedValue} from './pipes/pipe'; import {CHECK_ALWAYS, CHECK_ONCE, CHECKED, DETACHED, ON_PUSH} from './constants'; @@ -126,12 +125,6 @@ export class ChangeDetectionUtil { } } - static throwOnChange(proto: ProtoRecord, change) { - throw new ExpressionChangedAfterItHasBeenCheckedException(proto, change, null); - } - - static throwDehydrated() { throw new DehydratedException(); } - static changeDetectionMode(strategy: string): string { return strategy == ON_PUSH ? CHECK_ONCE : CHECK_ALWAYS; } @@ -140,15 +133,13 @@ export class ChangeDetectionUtil { return _simpleChange(previousValue, currentValue); } - static addChange(changes, propertyName: string, change): Map { - if (isBlank(changes)) { - changes = {}; - } - changes[propertyName] = change; - return changes; - } - static isValueBlank(value: any): boolean { return isBlank(value); } static s(value: any): string { return isPresent(value) ? `${value}` : ''; } + + static protoByIndex(protos: ProtoRecord[], selfIndex: number): ProtoRecord { + return selfIndex < 1 ? + null : + protos[selfIndex - 1]; // self index is shifted by one because of context + } } diff --git a/modules/angular2/src/change_detection/codegen_name_util.ts b/modules/angular2/src/change_detection/codegen_name_util.ts index 0efd100aba..6e797e6423 100644 --- a/modules/angular2/src/change_detection/codegen_name_util.ts +++ b/modules/angular2/src/change_detection/codegen_name_util.ts @@ -9,7 +9,7 @@ import {ProtoRecord} from './proto_record'; // detection will fail. const _ALREADY_CHECKED_ACCESSOR = "alreadyChecked"; const _CONTEXT_ACCESSOR = "context"; -const _CURRENT_PROTO = "currentProto"; +const _FIRST_PROTO_IN_CURRENT_BINDING = "firstProtoInCurrentBinding"; const _DIRECTIVES_ACCESSOR = "directiveRecords"; const _DISPATCHER_ACCESSOR = "dispatcher"; const _LOCALS_ACCESSOR = "locals"; @@ -67,7 +67,9 @@ export class CodegenNameUtil { getModeName(): string { return this._addFieldPrefix(_MODE_ACCESSOR); } - getCurrentProtoName(): string { return this._addFieldPrefix(_CURRENT_PROTO); } + getFirstProtoInCurrentBinding(): string { + return this._addFieldPrefix(_FIRST_PROTO_IN_CURRENT_BINDING); + } getLocalName(idx: int): string { return `l_${this._sanitizedNames[idx]}`; } diff --git a/modules/angular2/src/change_detection/dynamic_change_detector.ts b/modules/angular2/src/change_detection/dynamic_change_detector.ts index eb20e58c56..dfbd2a7b0f 100644 --- a/modules/angular2/src/change_detection/dynamic_change_detector.ts +++ b/modules/angular2/src/change_detection/dynamic_change_detector.ts @@ -55,12 +55,8 @@ export class DynamicChangeDetector extends AbstractChangeDetector { checkNoChanges(): void { this.runDetectChanges(true); } - // TODO(vsavkin): #3366. Update to work with [AbstractChangeDetector#detectChangesInRecords] - detectChangesInRecords(throwOnChange: boolean) { - if (!this.hydrated()) { - ChangeDetectionUtil.throwDehydrated(); - } - var protos: List = this.protos; + detectChangesInRecordsInternal(throwOnChange: boolean) { + var protos = this.protos; var changes = null; var isChanged = false; @@ -69,6 +65,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector { var bindingRecord = proto.bindingRecord; var directiveRecord = bindingRecord.directiveRecord; + if (this._firstInBinding(proto)) { + this.firstProtoInCurrentBinding = proto.selfIndex; + } + if (proto.isLifeCycleRecord()) { if (proto.name === "onCheck" && !throwOnChange) { this._getDirectiveFor(directiveRecord.directiveIndex).onCheck(); @@ -100,6 +100,11 @@ export class DynamicChangeDetector extends AbstractChangeDetector { this.alreadyChecked = true; } + _firstInBinding(r: ProtoRecord): boolean { + var prev = ChangeDetectionUtil.protoByIndex(this.protos, r.selfIndex - 1); + return isBlank(prev) || prev.bindingRecord !== r.bindingRecord; + } + callOnAllChangesDone() { this.dispatcher.notifyOnAllChangesDone(); var dirs = this.directiveRecords; @@ -122,7 +127,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector { _addChange(bindingRecord: BindingRecord, change, changes) { if (bindingRecord.callOnChange()) { - return ChangeDetectionUtil.addChange(changes, bindingRecord.propertyName, change); + return super.addChange(changes, change.previousValue, change.currentValue); } else { return changes; } @@ -133,14 +138,10 @@ export class DynamicChangeDetector extends AbstractChangeDetector { _getDetectorFor(directiveIndex) { return this.directives.getDetectorFor(directiveIndex); } _check(proto: ProtoRecord, throwOnChange: boolean): SimpleChange { - try { - if (proto.isPipeRecord()) { - return this._pipeCheck(proto, throwOnChange); - } else { - return this._referenceCheck(proto, throwOnChange); - } - } catch (e) { - this.throwError(proto, e, e.stack); + if (proto.isPipeRecord()) { + return this._pipeCheck(proto, throwOnChange); + } else { + return this._referenceCheck(proto, throwOnChange); } } @@ -156,7 +157,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector { if (!isSame(prevValue, currValue)) { if (proto.lastInBinding) { var change = ChangeDetectionUtil.simpleChange(prevValue, currValue); - if (throwOnChange) ChangeDetectionUtil.throwOnChange(proto, change); + if (throwOnChange) this.throwOnChangeError(prevValue, currValue); this._writeSelf(proto, currValue); this._setChanged(proto, true); @@ -241,7 +242,7 @@ export class DynamicChangeDetector extends AbstractChangeDetector { if (proto.lastInBinding) { var change = ChangeDetectionUtil.simpleChange(prevValue, currValue); - if (throwOnChange) ChangeDetectionUtil.throwOnChange(proto, change); + if (throwOnChange) this.throwOnChangeError(prevValue, currValue); this._writeSelf(proto, currValue); this._setChanged(proto, true); diff --git a/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart b/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart index 38e28dc2d9..eee47ad8f3 100644 --- a/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart +++ b/modules/angular2/src/transform/template_compiler/change_detector_codegen.dart @@ -109,8 +109,6 @@ class _CodegenState { } void detectChangesInRecordsInternal(throwOnChange) { - ${_names.getCurrentProtoName()} = null; - ${_names.genInitLocals()} var $_IS_CHANGED_LOCAL = false; var $_CHANGES_LOCAL = null; @@ -225,7 +223,11 @@ class _CodegenState { } else { rec = _genReferenceCheck(r); } - return '$rec${_maybeGenLastInDirective(r)}'; + return ''' + ${this._maybeFirstInBinding(r)} + ${rec} + ${this._maybeGenLastInDirective(r)} + '''; } String _genDirectiveLifecycle(ProtoRecord r) { @@ -249,12 +251,9 @@ class _CodegenState { var pipe = _names.getPipeName(r.selfIndex); var cdRef = 'this.ref'; - - var protoIndex = r.selfIndex - 1; var pipeType = r.name; var read = ''' - ${_names.getCurrentProtoName()} = ${_names.getProtosName()}[$protoIndex]; if ($_IDENTICAL_CHECK_FN($pipe, $_UTIL.uninitialized)) { $pipe = ${_names.getPipesAccessorName()}.get('$pipeType', $context, $cdRef); } else if (!$pipe.supports($context)) { @@ -280,11 +279,7 @@ class _CodegenState { String _genReferenceCheck(ProtoRecord r) { var oldValue = _names.getFieldName(r.selfIndex); var newValue = _names.getLocalName(r.selfIndex); - - var protoIndex = r.selfIndex - 1; - var read = ''' - ${_names.getCurrentProtoName()} = ${_names.getProtosName()}[$protoIndex]; ${_genUpdateCurrentValue(r)} '''; @@ -411,8 +406,7 @@ class _CodegenState { } else { return ''' ${_genThrowOnChangeCheck(oldValue, newValue)} - ${_names.getDispatcherName()}.notifyOnBinding( - ${_names.getCurrentProtoName()}.bindingRecord, ${newValue}); + this.notifyDispatcher(${newValue}); '''; } } @@ -421,8 +415,7 @@ class _CodegenState { if (this._generateCheckNoChanges) { return ''' if(throwOnChange) { - $_UTIL.throwOnChange( - ${_names.getCurrentProtoName()}, $_UTIL.simpleChange(${oldValue}, ${newValue})); + this.throwOnChangeError(${oldValue}, ${newValue}); } '''; } else { @@ -438,16 +431,17 @@ class _CodegenState { } } + String _maybeFirstInBinding(ProtoRecord r) { + var prev = ChangeDetectionUtil.protoByIndex(_records, r.selfIndex - 1); + var firstInBindng = prev == null || prev.bindingRecord != r.bindingRecord; + return firstInBindng ? "${_names.getFirstProtoInCurrentBinding()} = ${r.selfIndex};" : ''; + } + String _genAddToChanges(ProtoRecord r) { var newValue = _names.getLocalName(r.selfIndex); var oldValue = _names.getFieldName(r.selfIndex); if (!r.bindingRecord.callOnChange()) return ''; - return ''' - $_CHANGES_LOCAL = $_UTIL.addChange( - $_CHANGES_LOCAL, - ${_names.getCurrentProtoName()}.bindingRecord.propertyName, - $_UTIL.simpleChange($oldValue, $newValue)); - '''; + return "$_CHANGES_LOCAL = addChange($_CHANGES_LOCAL, $oldValue, $newValue);"; } String _maybeGenLastInDirective(ProtoRecord r) { diff --git a/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart b/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart index cd9510846a..a6419c4535 100644 --- a/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart +++ b/modules/angular2/test/transform/integration/two_annotations_files/expected/bar.ng_deps.dart @@ -34,7 +34,6 @@ class _MyComponent_ChangeDetector0 } void detectChangesInRecordsInternal(throwOnChange) { - this.currentProto = null; var l_context = this.context, l_myNum0, c_myNum0, @@ -43,7 +42,7 @@ class _MyComponent_ChangeDetector0 var isChanged = false; var changes = null; - this.currentProto = this.protos[0]; + this.firstProtoInCurrentBinding = 1; l_myNum0 = l_context.myNum; if (_gen.looseNotIdentical(l_myNum0, this.myNum0)) { c_myNum0 = true; @@ -51,18 +50,14 @@ class _MyComponent_ChangeDetector0 this.myNum0 = l_myNum0; } if (c_myNum0) { - this.currentProto = this.protos[1]; l_interpolate1 = "Salad: " "${l_myNum0 == null ? "" : l_myNum0}" " is awesome"; if (_gen.looseNotIdentical(l_interpolate1, this.interpolate1)) { if (throwOnChange) { - _gen.ChangeDetectionUtil.throwOnChange(this.currentProto, - _gen.ChangeDetectionUtil.simpleChange( - this.interpolate1, l_interpolate1)); + this.throwOnChangeError(this.interpolate1, l_interpolate1); } - this.dispatcher.notifyOnBinding( - this.currentProto.bindingRecord, l_interpolate1); + this.notifyDispatcher(l_interpolate1); this.interpolate1 = l_interpolate1; } diff --git a/modules/angular2/test/transform/template_compiler/all_tests.dart b/modules/angular2/test/transform/template_compiler/all_tests.dart index d327ceeaab..cbf74cb54a 100644 --- a/modules/angular2/test/transform/template_compiler/all_tests.dart +++ b/modules/angular2/test/transform/template_compiler/all_tests.dart @@ -31,10 +31,10 @@ void changeDetectorTests() { // TODO(tbosch): This is just a temporary test that makes sure that the dart server and // dart browser is in sync. Change this to "not contains notifyBinding" // when https://github.com/angular/angular/issues/3019 is solved. - it('shouldn always notifyBinding for template variables', () async { + it('shouldn always notifyDispatcher for template variables', () async { var inputPath = 'template_compiler/ng_for_files/hello.ng_deps.dart'; var output = await (process(new AssetId('a', inputPath))); - expect(output).toContain('notifyOnBinding'); + expect(output).toContain('notifyDispatcher'); }); it('should include directives mentioned in directive aliases.', () async {