From 59ca19e8a15528a895c5f81afc1ef760108780b3 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Tue, 8 Feb 2022 10:58:32 -0600 Subject: [PATCH 1/3] refactor(SafeSubscriber): optimize perf for ordinary observers No longer require function binding if we aren't using the deprecated next context. This should improve performance in the common path of consumers subscribing with an object or even with a function. Adds a simple class `ConsumerObserver` which is mostly meant to optimize the number of function refrences created. We should never expose this externally. Related #6783 --- src/internal/Subscriber.ts | 101 +++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 37 deletions(-) diff --git a/src/internal/Subscriber.ts b/src/internal/Subscriber.ts index 5862a8d91b..eab188fda3 100644 --- a/src/internal/Subscriber.ts +++ b/src/internal/Subscriber.ts @@ -147,6 +147,46 @@ function bind any>(fn: Fn, thisArg: any): Fn { return _bind.call(fn, thisArg); } +/** + * Internal optimization only, DO NOT EXPOSE. + * @internal + */ +class ConsumerObserver implements Observer { + constructor(private partialObserver: Partial>) {} + + next(value: T): void { + if (this.partialObserver.next) { + try { + this.partialObserver.next(value); + } catch (error) { + handleUnhandledError(error); + } + } + } + + error(err: any): void { + if (this.partialObserver.error) { + try { + this.partialObserver.error(err); + } catch (error) { + handleUnhandledError(error); + } + } else { + handleUnhandledError(err); + } + } + + complete(): void { + if (this.partialObserver.complete) { + try { + this.partialObserver.complete(); + } catch (error) { + handleUnhandledError(error); + } + } + } +} + export class SafeSubscriber extends Subscriber { constructor( observerOrNext?: Partial> | ((value: T) => void) | null, @@ -155,18 +195,21 @@ export class SafeSubscriber extends Subscriber { ) { super(); - let next: ((value: T) => void) | undefined; - if (isFunction(observerOrNext)) { + let partialObserver: Partial>; + if (isFunction(observerOrNext) || !observerOrNext) { // The first argument is a function, not an observer. The next // two arguments *could* be observers, or they could be empty. - next = observerOrNext; - } else if (observerOrNext) { + partialObserver = { + next: observerOrNext ?? undefined, + error: error ?? undefined, + complete: complete ?? undefined, + }; + } else { // The first argument is an observer object, we have to pull the handlers // off and capture the owner object as the context. That is because we're // going to put them all in a new destination with ensured methods // for `next`, `error`, and `complete`. That's part of what makes this // the "Safe" Subscriber. - ({ next, error, complete } = observerOrNext); let context: any; if (this && config.useDeprecatedNextContext) { // This is a deprecated path that made `this.unsubscribe()` available in @@ -174,46 +217,30 @@ export class SafeSubscriber extends Subscriber { // now, as it is *very* slow. context = Object.create(observerOrNext); context.unsubscribe = () => this.unsubscribe(); + partialObserver = { + next: observerOrNext.next && bind(observerOrNext.next, context), + error: observerOrNext.error && bind(observerOrNext.error, context), + complete: observerOrNext.complete && bind(observerOrNext.complete, context), + }; } else { - context = observerOrNext; + partialObserver = observerOrNext; } - next = next && bind(next, context); - error = error && bind(error, context); - complete = complete && bind(complete, context); } - // Once we set the destination, the superclass `Subscriber` will - // do it's magic in the `_next`, `_error`, and `_complete` methods. - this.destination = { - next: next ? wrapForErrorHandling(next, this) : noop, - error: wrapForErrorHandling(error ?? defaultErrorHandler, this), - complete: complete ? wrapForErrorHandling(complete, this) : noop, - }; + this.destination = new ConsumerObserver(partialObserver); } } -/** - * Wraps a user-provided handler (or our {@link defaultErrorHandler} in one case) to - * ensure that any thrown errors are caught and handled appropriately. - * - * @param handler The handler to wrap - * @param instance The SafeSubscriber instance we're going to mark if there's an error. - */ -function wrapForErrorHandling(handler: (arg?: any) => void, instance: SafeSubscriber) { - return (...args: any[]) => { - try { - handler(...args); - } catch (err) { - if (config.useDeprecatedSynchronousErrorHandling) { - captureError(err); - } else { - // Ideal path, we report this as an unhandled error, - // which is thrown on a new call stack. - reportUnhandledError(err); - } - } - }; +function handleUnhandledError(error: any) { + if (config.useDeprecatedSynchronousErrorHandling) { + captureError(error); + } else { + // Ideal path, we report this as an unhandled error, + // which is thrown on a new call stack. + reportUnhandledError(error); + } } + /** * An error handler used when no error handler was supplied * to the SafeSubscriber -- meaning no error handler was supplied From 3d0325f793ea3b5b4e54f3e84866d71338dff799 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Tue, 8 Feb 2022 11:08:25 -0600 Subject: [PATCH 2/3] chore: update comments --- src/internal/Subscriber.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/internal/Subscriber.ts b/src/internal/Subscriber.ts index eab188fda3..31003c32ef 100644 --- a/src/internal/Subscriber.ts +++ b/src/internal/Subscriber.ts @@ -205,11 +205,7 @@ export class SafeSubscriber extends Subscriber { complete: complete ?? undefined, }; } else { - // The first argument is an observer object, we have to pull the handlers - // off and capture the owner object as the context. That is because we're - // going to put them all in a new destination with ensured methods - // for `next`, `error`, and `complete`. That's part of what makes this - // the "Safe" Subscriber. + // The first argument is a partial observer. let context: any; if (this && config.useDeprecatedNextContext) { // This is a deprecated path that made `this.unsubscribe()` available in @@ -223,10 +219,13 @@ export class SafeSubscriber extends Subscriber { complete: observerOrNext.complete && bind(observerOrNext.complete, context), }; } else { + // The "normal" path. Just use the partial observer directly. partialObserver = observerOrNext; } } + // Wrap the partial observer to ensure it's a full observer, and + // make sure proper error handling is accounted for. this.destination = new ConsumerObserver(partialObserver); } } From d2bbc4c468bc189d51291d5a48ca9422385f8f3e Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Tue, 8 Feb 2022 21:59:25 -0600 Subject: [PATCH 3/3] refactor(Subscriber): reduce property access --- src/internal/Subscriber.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/internal/Subscriber.ts b/src/internal/Subscriber.ts index 31003c32ef..a09986d5c1 100644 --- a/src/internal/Subscriber.ts +++ b/src/internal/Subscriber.ts @@ -155,9 +155,10 @@ class ConsumerObserver implements Observer { constructor(private partialObserver: Partial>) {} next(value: T): void { - if (this.partialObserver.next) { + const { partialObserver } = this; + if (partialObserver.next) { try { - this.partialObserver.next(value); + partialObserver.next(value); } catch (error) { handleUnhandledError(error); } @@ -165,9 +166,10 @@ class ConsumerObserver implements Observer { } error(err: any): void { - if (this.partialObserver.error) { + const { partialObserver } = this; + if (partialObserver.error) { try { - this.partialObserver.error(err); + partialObserver.error(err); } catch (error) { handleUnhandledError(error); } @@ -177,9 +179,10 @@ class ConsumerObserver implements Observer { } complete(): void { - if (this.partialObserver.complete) { + const { partialObserver } = this; + if (partialObserver.complete) { try { - this.partialObserver.complete(); + partialObserver.complete(); } catch (error) { handleUnhandledError(error); }