Skip to content

Commit

Permalink
feat(core): drop ChangeDetectionStrategy.OnPushObserve
Browse files Browse the repository at this point in the history 
BREAKING CHANGE:

`OnPushObserve` was an experimental
feature for Dart and had
conceptual performance problems,
as setting up observables is slow.
Use `OnPush` instead.
  • Loading branch information
@tbosch @vsavkin
tbosch authored and vsavkin committed Mar 1, 2016
1 parent d900f5c commit f60fa14
Show file tree
Hide file tree
Showing 23 changed files with 14 additions and 1,019 deletions.
74 changes: 0 additions & 74 deletions modules/angular2/src/core/change_detection/abstract_change_detector.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {BindingTarget} from './binding_record';
import {Locals} from './parser/locals';
import {ChangeDetectionStrategy, ChangeDetectorState} from './constants';
import {wtfCreateScope, wtfLeave, WtfScopeFn} from '../profile/profile';
import {isObservable} from './observable_facade';
import {ObservableWrapper} from 'angular2/src/facade/async';

var _scope_check: WtfScopeFn = wtfCreateScope(`ChangeDetector#check(ascii id, bool throwOnChange)`);
Expand All @@ -42,10 +41,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
propertyBindingIndex: number;
outputSubscriptions: any[];

// This is an experimental feature. Works only in Dart.
subscriptions: any[];
streams: any[];

dispatcher: ChangeDispatcher;


Expand Down Expand Up @@ -158,10 +153,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
this.mode = ChangeDetectionUtil.changeDetectionMode(this.strategy);
this.context = context;

if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
this.observeComponent(context);
}

this.locals = locals;
this.pipes = pipes;
this.hydrateDirectives(dispatcher);
Expand All @@ -176,11 +167,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
dehydrate(): void {
this.dehydrateDirectives(true);

// This is an experimental feature. Works only in Dart.
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
this._unsubsribeFromObservables();
}

this._unsubscribeFromOutputs();

this.dispatcher = null;
Expand Down Expand Up @@ -248,19 +234,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
}
}

// This is an experimental feature. Works only in Dart.
private _unsubsribeFromObservables(): void {
if (isPresent(this.subscriptions)) {
for (var i = 0; i < this.subscriptions.length; ++i) {
var s = this.subscriptions[i];
if (isPresent(this.subscriptions[i])) {
s.cancel();
this.subscriptions[i] = null;
}
}
}
}

private _unsubscribeFromOutputs(): void {
if (isPresent(this.outputSubscriptions)) {
for (var i = 0; i < this.outputSubscriptions.length; ++i) {
Expand All @@ -270,53 +243,6 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
}
}

// This is an experimental feature. Works only in Dart.
observeValue(value: any, index: number): any {
if (isObservable(value)) {
this._createArrayToStoreObservables();
if (isBlank(this.subscriptions[index])) {
this.streams[index] = value.changes;
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
} else if (this.streams[index] !== value.changes) {
this.subscriptions[index].cancel();
this.streams[index] = value.changes;
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
}
}
return value;
}

// This is an experimental feature. Works only in Dart.
observeDirective(value: any, index: number): any {
if (isObservable(value)) {
this._createArrayToStoreObservables();
var arrayIndex = this.numberOfPropertyProtoRecords + index + 2; // +1 is component
this.streams[arrayIndex] = value.changes;
this.subscriptions[arrayIndex] = value.changes.listen((_) => this.ref.markForCheck());
}
return value;
}

// This is an experimental feature. Works only in Dart.
observeComponent(value: any): any {
if (isObservable(value)) {
this._createArrayToStoreObservables();
var index = this.numberOfPropertyProtoRecords + 1;
this.streams[index] = value.changes;
this.subscriptions[index] = value.changes.listen((_) => this.ref.markForCheck());
}
return value;
}

private _createArrayToStoreObservables(): void {
if (isBlank(this.subscriptions)) {
this.subscriptions = ListWrapper.createFixedSize(this.numberOfPropertyProtoRecords +
this.directiveIndices.length + 2);
this.streams = ListWrapper.createFixedSize(this.numberOfPropertyProtoRecords +
this.directiveIndices.length + 2);
}
}

getDirectiveFor(directives: any, index: number): any {
return directives.getDirectiveFor(this.directiveIndices[index]);
}
Expand Down
5 changes: 2 additions & 3 deletions modules/angular2/src/core/change_detection/change_detection_jit_generator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,8 @@ export class ChangeDetectorJITGenerator {
this.directiveRecords = definition.directiveRecords;
this._names = new CodegenNameUtil(this.records, this.eventBindings, this.directiveRecords,
this.changeDetectionUtilVarName);
this._logic =
new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
this.changeDetectorStateVarName, this.changeDetectionStrategy);
this._logic = new CodegenLogicUtil(this._names, this.changeDetectionUtilVarName,
this.changeDetectorStateVarName);
this.typeName = sanitizeName(`ChangeDetector_${this.id}`);
}

Expand Down
40 changes: 9 additions & 31 deletions modules/angular2/src/core/change_detection/codegen_logic_util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@ import {codify, combineGeneratedStrings, rawString} from './codegen_facade';
import {ProtoRecord, RecordType} from './proto_record';
import {BindingTarget} from './binding_record';
import {DirectiveRecord} from './directive_record';
import {ChangeDetectionStrategy} from './constants';
import {BaseException} from 'angular2/src/facade/exceptions';

/**
* Class responsible for providing change detection logic for change detector classes.
*/
export class CodegenLogicUtil {
constructor(private _names: CodegenNameUtil, private _utilName: string,
private _changeDetectorStateName: string,
private _changeDetection: ChangeDetectionStrategy) {}
private _changeDetectorStateName: string) {}

/**
* Generates a statement which updates the local variable representing `protoRec` with the current
Expand Down Expand Up @@ -51,31 +49,29 @@ export class CodegenLogicUtil {
break;

case RecordType.PropertyRead:
rhs = this._observe(`${context}.${protoRec.name}`, protoRec);
rhs = `${context}.${protoRec.name}`;
break;

case RecordType.SafeProperty:
var read = this._observe(`${context}.${protoRec.name}`, protoRec);
rhs =
`${this._utilName}.isValueBlank(${context}) ? null : ${this._observe(read, protoRec)}`;
var read = `${context}.${protoRec.name}`;
rhs = `${this._utilName}.isValueBlank(${context}) ? null : ${read}`;
break;

case RecordType.PropertyWrite:
rhs = `${context}.${protoRec.name} = ${getLocalName(protoRec.args[0])}`;
break;

case RecordType.Local:
rhs = this._observe(`${localsAccessor}.get(${rawString(protoRec.name)})`, protoRec);
rhs = `${localsAccessor}.get(${rawString(protoRec.name)})`;
break;

case RecordType.InvokeMethod:
rhs = this._observe(`${context}.${protoRec.name}(${argString})`, protoRec);
rhs = `${context}.${protoRec.name}(${argString})`;
break;

case RecordType.SafeMethodInvoke:
var invoke = `${context}.${protoRec.name}(${argString})`;
rhs =
`${this._utilName}.isValueBlank(${context}) ? null : ${this._observe(invoke, protoRec)}`;
rhs = `${this._utilName}.isValueBlank(${context}) ? null : ${invoke}`;
break;

case RecordType.InvokeClosure:
Expand All @@ -95,7 +91,7 @@ export class CodegenLogicUtil {
break;

case RecordType.KeyedRead:
rhs = this._observe(`${context}[${getLocalName(protoRec.args[0])}]`, protoRec);
rhs = `${context}[${getLocalName(protoRec.args[0])}]`;
break;

case RecordType.KeyedWrite:
Expand All @@ -112,16 +108,6 @@ export class CodegenLogicUtil {
return `${getLocalName(protoRec.selfIndex)} = ${rhs};`;
}

/** @internal */
_observe(exp: string, rec: ProtoRecord): string {
// This is an experimental feature. Works only in Dart.
if (this._changeDetection === ChangeDetectionStrategy.OnPushObserve) {
return `this.observeValue(${exp}, ${rec.selfIndex})`;
} else {
return exp;
}
}

genPropertyBindingTargets(propertyBindingTargets: BindingTarget[],
genDebugInfo: boolean): string {
var bs = propertyBindingTargets.map(b => {
Expand Down Expand Up @@ -202,15 +188,7 @@ export class CodegenLogicUtil {
}
}

private _genReadDirective(index: number) {
var directiveExpr = `this.getDirectiveFor(directives, ${index})`;
// This is an experimental feature. Works only in Dart.
if (this._changeDetection === ChangeDetectionStrategy.OnPushObserve) {
return `this.observeDirective(${directiveExpr}, ${index})`;
} else {
return directiveExpr;
}
}
private _genReadDirective(index: number) { return `this.getDirectiveFor(directives, ${index})`; }

genHydrateDetectors(directiveRecords: DirectiveRecord[]): string {
var res = [];
Expand Down
8 changes: 1 addition & 7 deletions modules/angular2/src/core/change_detection/constants.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,6 @@ export enum ChangeDetectionStrategy {
* `Default` means that the change detector's mode will be set to `CheckAlways` during hydration.
*/
Default,

/**
* This is an experimental feature. Works only in Dart.
*/
OnPushObserve
}

/**
Expand All @@ -78,8 +73,7 @@ export var CHANGE_DETECTION_STRATEGY_VALUES = [
ChangeDetectionStrategy.CheckAlways,
ChangeDetectionStrategy.Detached,
ChangeDetectionStrategy.OnPush,
ChangeDetectionStrategy.Default,
ChangeDetectionStrategy.OnPushObserve
ChangeDetectionStrategy.Default
];

/**
Expand Down
9 changes: 0 additions & 9 deletions modules/angular2/src/core/change_detection/dynamic_change_detector.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,6 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
this.values[0] = this.context;
this.dispatcher = dispatcher;

if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
for (var i = 0; i < this.directiveIndices.length; ++i) {
var index = this.directiveIndices[i];
super.observeDirective(this._getDirectiveFor(index), i);
}
}
this.outputSubscriptions = [];
for (var i = 0; i < this._directiveRecords.length; ++i) {
var r = this._directiveRecords[i];
Expand Down Expand Up @@ -300,9 +294,6 @@ export class DynamicChangeDetector extends AbstractChangeDetector<any> {
}

var currValue = this._calculateCurrValue(proto, values, locals);
if (this.strategy === ChangeDetectionStrategy.OnPushObserve) {
super.observeValue(currValue, proto.selfIndex);
}

if (proto.shouldBeChecked()) {
var prevValue = this._readSelf(proto, values);
Expand Down
5 changes: 0 additions & 5 deletions modules/angular2/src/core/change_detection/observable_facade.dart
View file

This file was deleted.

3 changes: 0 additions & 3 deletions modules/angular2/src/core/change_detection/observable_facade.ts
View file

This file was deleted.

22 changes: 1 addition & 21 deletions modules/angular2/test/core/change_detection/change_detector_config.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,21 +106,6 @@ export function getDefinition(id: string): TestDefinition {
[_DirectiveUpdating.basicRecords[0], _DirectiveUpdating.basicRecords[1]], genConfig);
testDef = new TestDefinition(id, cdDef, null);

} else if (id == "onPushObserveBinding") {
var records = _createBindingRecords("a");
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], records,
[], [], genConfig);
testDef = new TestDefinition(id, cdDef, null);

} else if (id == "onPushObserveComponent") {
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], [], [],
[], genConfig);
testDef = new TestDefinition(id, cdDef, null);

} else if (id == "onPushObserveDirective") {
let cdDef = new ChangeDetectorDefinition(id, ChangeDetectionStrategy.OnPushObserve, [], [], [],
[_DirectiveUpdating.recordNoCallbacks], genConfig);
testDef = new TestDefinition(id, cdDef, null);
} else if (id == "updateElementProduction") {
var genConfig = new ChangeDetectorGenConfig(false, false, true);
var records = _createBindingRecords("name");
Expand Down Expand Up @@ -151,12 +136,7 @@ export function getAllDefinitions(): TestDefinition[] {
allDefs = allDefs.concat(StringMapWrapper.keys(_DirectiveUpdating.availableDefinitions));
allDefs = allDefs.concat(_availableEventDefinitions);
allDefs = allDefs.concat(_availableHostEventDefinitions);
allDefs = allDefs.concat([
"onPushObserveBinding",
"onPushObserveComponent",
"onPushObserveDirective",
"updateElementProduction"
]);
allDefs = allDefs.concat(["updateElementProduction"]);
return allDefs.map(getDefinition);
}

Expand Down
Loading

0 comments on commit f60fa14

Please sign in to comment.