From 46d4f3f0903178169470255eaa0a45c64da8addd Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Thu, 31 Jan 2019 22:12:51 +0100 Subject: [PATCH] perf(Subscription): improve parent management Merge the `_parent` and `_parents` fields on the `Subscription` class into a single `_parentOrParents` field, which can hold either `null` (in case of no parents), a `Subscription` instance (in case of a single parent), or an array of `Subscription`s (in case of multiple parents). This not only shrinks the size of `Subscription` (and subclass) instances, but more importantly reduces the number of megamorphic property access sites in the hot `Subscription` methods, especially in `unsubscribe()` and `add()`. Also inline the `Subscription#_addParent()` method into `Subscription#add()`, as it's the only call site anyways and this removes another hot megamorphic `LoadIC` that was only needed to lookup `subscription._addParent` in the call site. Finally remove the `hasErrors` variable from `unsubscribe()` method and instead just check `errors` variable directly. --- src/internal/Subscriber.ts | 8 ++-- src/internal/Subscription.ts | 93 ++++++++++++++++-------------------- 2 files changed, 43 insertions(+), 58 deletions(-) diff --git a/src/internal/Subscriber.ts b/src/internal/Subscriber.ts index 68d1622632..4e6558416c 100644 --- a/src/internal/Subscriber.ts +++ b/src/internal/Subscriber.ts @@ -151,14 +151,12 @@ export class Subscriber extends Subscription implements Observer { /** @deprecated This is an internal implementation detail, do not use. */ _unsubscribeAndRecycle(): Subscriber { - const { _parent, _parents } = this; - this._parent = null; - this._parents = null; + const { _parentOrParents } = this; + this._parentOrParents = null; this.unsubscribe(); this.closed = false; this.isStopped = false; - this._parent = _parent; - this._parents = _parents; + this._parentOrParents = _parentOrParents; return this; } } diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 9ec33b5a17..0f1906a27e 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -30,9 +30,7 @@ export class Subscription implements SubscriptionLike { public closed: boolean = false; /** @internal */ - protected _parent: Subscription = null; - /** @internal */ - protected _parents: Subscription[] = null; + protected _parentOrParents: Subscription | Subscription[] = null; /** @internal */ private _subscriptions: SubscriptionLike[] = null; @@ -53,47 +51,40 @@ export class Subscription implements SubscriptionLike { * @return {void} */ unsubscribe(): void { - let hasErrors = false; let errors: any[]; if (this.closed) { return; } - let { _parent, _parents, _unsubscribe, _subscriptions } = ( this); + let { _parentOrParents, _unsubscribe, _subscriptions } = ( this); this.closed = true; - this._parent = null; - this._parents = null; + this._parentOrParents = null; // null out _subscriptions first so any child subscriptions that attempt // to remove themselves from this subscription will noop this._subscriptions = null; - let index = -1; - let len = _parents ? _parents.length : 0; - - // if this._parent is null, then so is this._parents, and we - // don't have to remove ourselves from any parent subscriptions. - while (_parent) { - _parent.remove(this); - // if this._parents is null or index >= len, - // then _parent is set to null, and the loop exits - _parent = ++index < len && _parents[index] || null; + if (_parentOrParents instanceof Subscription) { + _parentOrParents.remove(this); + } else if (_parentOrParents !== null) { + for (let index = 0; index < _parentOrParents.length; ++index) { + const parent = _parentOrParents[index]; + parent.remove(this); + } } if (isFunction(_unsubscribe)) { try { _unsubscribe.call(this); } catch (e) { - hasErrors = true; errors = e instanceof UnsubscriptionError ? flattenUnsubscriptionErrors(e.errors) : [e]; } } if (isArray(_subscriptions)) { - - index = -1; - len = _subscriptions.length; + let index = -1; + let len = _subscriptions.length; while (++index < len) { const sub = _subscriptions[index]; @@ -101,7 +92,6 @@ export class Subscription implements SubscriptionLike { try { sub.unsubscribe(); } catch (e) { - hasErrors = true; errors = errors || []; if (e instanceof UnsubscriptionError) { errors = errors.concat(flattenUnsubscriptionErrors(e.errors)); @@ -113,7 +103,7 @@ export class Subscription implements SubscriptionLike { } } - if (hasErrors) { + if (errors) { throw new UnsubscriptionError(errors); } } @@ -164,14 +154,34 @@ export class Subscription implements SubscriptionLike { } } - if (subscription._addParent(this)) { - // Optimize for the common case when adding the first subscription. - const subscriptions = this._subscriptions; - if (subscriptions) { - subscriptions.push(subscription); - } else { - this._subscriptions = [subscription]; + // Add `this` as parent of `subscription` if that's not already the case. + let { _parentOrParents } = subscription; + if (_parentOrParents === null) { + // If we don't have a parent, then set `subscription._parents` to + // the `this`, which is the common case that we optimize for. + subscription._parentOrParents = this; + } else if (_parentOrParents instanceof Subscription) { + if (_parentOrParents === this) { + // The `subscription` already has `this` as a parent. + return subscription; } + // If there's already one parent, but not multiple, allocate an + // Array to store the rest of the parent Subscriptions. + subscription._parentOrParents = [_parentOrParents, this]; + } else if (_parentOrParents.indexOf(this) === -1) { + // Only add `this` to the _parentOrParents list if it's not already there. + _parentOrParents.push(this); + } else { + // The `subscription` already has `this` as a parent. + return subscription; + } + + // Optimize for the common case when adding the first subscription. + const subscriptions = this._subscriptions; + if (subscriptions === null) { + this._subscriptions = [subscription]; + } else { + subscriptions.push(subscription); } return subscription; @@ -192,29 +202,6 @@ export class Subscription implements SubscriptionLike { } } } - - /** @internal */ - private _addParent(parent: Subscription): boolean { - let { _parent, _parents } = this; - if (_parent === parent) { - // If the new parent is the same as the current parent, then do nothing. - return false; - } else if (!_parent) { - // If we don't have a parent, then set this._parent to the new parent. - this._parent = parent; - return true; - } else if (!_parents) { - // If there's already one parent, but not multiple, allocate an Array to - // store the rest of the parent Subscriptions. - this._parents = [parent]; - return true; - } else if (_parents.indexOf(parent) === -1) { - // Only add the new parent to the _parents list if it's not already there. - _parents.push(parent); - return true; - } - return false; - } } function flattenUnsubscriptionErrors(errors: any[]) {