perf(SafeSubscriber): avoid using `Object.create` #5646

JoostK · 2020-08-15T18:35:34Z

In profiling sessions, the SafeSubscriber constructor was showing as hot
function for code that creates and tear downs Observables very frequently.
The usage of Object.create attributes to the slowness; avoiding using it
completely removes any overhead of SafeSubscriber.

The Object.create result was used to call anonymous subscribers in their
own context, i.e. the this pointer in next/error/complete is the
anonymous object itself. This commit implements an alternative to achieve
the same effect; binding the subscriber functions to the observer object.

Timings

The below timings are with NodeJS 14.8.0 in a full Angular CLI build of https://github.com/JoostK/ng9-aot-build-times/tree/extra.

Before:

After:

JoostK · 2020-08-15T18:44:21Z

src/internal/Subscriber.ts

        if (isSubscription(observerOrNext)) {
          observerOrNext.add(this.unsubscribe.bind(this));
        }
-        context.unsubscribe = this.unsubscribe.bind(this);


I was able to remove this statement without affecting any tests; this makes me a bit unsure on what this was trying to achieve and if it's safe to remove it, although I can't think of what this was supposed to do.

And I realized after opening this that without this assignment, it becomes possible to just use context = observerOrNext as an alternative to binding the subscriber callbacks. If that is indeed possible (again, no tests start to fail) then I think switching to use that approach makes sense.

Edit: this assignment does allow anonymous subscribers to call this.unsubscribe(), which this PR would break. Since there's no tests for this, how intentional is that? ~~Furthermore, I'd consider this to cause inconsistencies for anonymous objects that do have unsubscribe themselves.~~ <- Actually, that appears to work, resulting in unsubscription and the anonymous' object's unsubscribe to be called. Here's a small StackBlitz sample.

cartant · 2020-08-16T00:17:29Z

I was able to remove this statement without affecting any tests; this makes me a bit unsure on what this was trying to achieve and if it's safe to remove it, although I can't think of what this was supposed to do.

It adds unsubscribe to the context so that it's possible to effect unsubscription from a synchronous source within a handler. See this comment and the links therein. Notwithstanding the lack of tests/docs, this would be a breaking change.

When the implementation moves to an AbortSignal-based approach, this context/prototype shenanigans will no longer be necessary.

JoostK · 2020-08-16T11:31:09Z

Thanks for the additional context @cartant, that gives me exactly the information I was missing before. The AbortSignal does indeed look like a viable alternative that should allow for avoiding this performance penalty sometime in the future.

I opened a PR in the Angular CLI to switch the anonymous observer to use separate callbacks instead, also avoiding this penalty: angular/angular-cli#18549. I wouldn't normally recommend using that syntax, but this occurs in a hot code path where the performance penalty starts to make a real difference.

benlesh · 2020-08-23T21:29:05Z

😩 I really want to get rid of this. But... one thing at a time.

benlesh · 2020-08-23T21:31:00Z

@cartant Do you know if we have any tests or documentation that covers the edge case outlined in the comment you linked to? I suspect we do not. Also, the promise of all of this sort of fails, as outlined by those firehose tests that I had you add last week.

benlesh · 2020-08-23T21:34:52Z

What I'm looking for is a way to make this change non-breaking.. OR a way to introduce the change as breaking with some sort of backwards compatibility package. It seems like the latter would have to be it, honestly. I'm going to guess that this change would break fewer than 0.001% of projects using RxJS, as I'm all but sure that this is undocumented. (And it's also relatively impossible to type with TypeScript).

benlesh · 2020-08-23T21:38:33Z

At this point, I'm sort of in favor of just making this breaking change. If people gripe during the beta, or even after, we can reenable it behind a flag in config.ts or something. I suspect it will not cause many, if any, issues.

cartant · 2020-08-23T21:57:54Z

Do you know if we have any tests or documentation that covers the edge case outlined in the comment you linked to?

Nope. Only the comment of Paul's. I recall this coming up a couple of times in issues. Paul's comment is the simplest to find in a search and I've referred to it a few times. This was all well before my time, so I'm unaware of the history re: no tes.ts/docs.

At this point, I'm sort of in favor of just making this breaking change.

Adding a useDeprecatedNextContext to the config - and defaulting it to false - would be reasonable, IMO. It would give the few people who might happen to depend on this an out and would let's us move on more quickly.

In profiling sessions, the `SafeSubscriber` constructor was showing as hot function for code that creates and tear downs Observables very frequently. The usage of `Object.create` attributes to the slowness; avoiding using it completely removes any overhead of `SafeSubscriber`. The `Object.create` result was used to call anonymous subscribers in their own context, i.e. the `this` pointer in `next`/`error`/`complete` is the anonymous object itself. This commit implements an alternative to achieve the same effect; binding the subscriber functions to the observer object.

… behind a flag - Adds a flag to `import { config } from 'rxjs';` that allows users to use the undocumented feature that gives access to `unsubscribe` via the `this` context of a `next` handler passed as part of an observer object to `subscribe`. This behavior is deprecated because it has very bad performance implications on all subscriptions and is relatively unused, definitely undocumented, and certainly mostly unknown. - Adds a flag to silence console warn messages that are emitted when "bad" configuration settings are used. - Adds some documentation. - Adds tests. BREAKING CHANGE: `unsubscribe` no longer available via the `this` context of observer functions. To reenable, set `config.useDeprecatedNextContext = true` on the rxjs `config` found at `import { config } from 'rxjs';`. Note that enabling this will result in a performance penalty for all consumer subscriptions.

benlesh · 2020-08-24T20:57:51Z

@cartant I've added the flag, as well as tests, and a little documentation about the breaking change. I've also rebased as there was a conflict in the spec file (since we just recently added more tests, and I also added more tests, no actual tests changed).

cartant

@benlesh LGTM, but I think we shouldn't provide the subscriber to plain callbacks either.

cartant · 2020-08-25T00:55:29Z

src/internal/Subscriber.ts

@@ -230,7 +234,7 @@ export class SafeSubscriber<T> extends Subscriber<T> {
    if (!this.isStopped) {
      const { _parentSubscriber } = this;
      if (this._complete) {
-        const wrappedComplete = () => this._complete.call(this._context);
+        const wrappedComplete = () => this._complete.call(this);


issue: I think this wrappedComplete business should also be conditional on config.useDeprecatedNextContext. I don't think we should provide the subscriber to the caller - via the context - for plain callbacks either.

const wrappedComplete = config.useDeprecatedNextContext ? () => this._complete.call(this) : this._complete;

And I think we need tests for plain callbacks, too.

I am about to make a pass through all of this. I'm going to merge this as-is, and address the rest in another PR.

- Simplifies the call patterns in SafeSubscriber to not be quite so insane. - Note: for some reason one mocha test is passing, but still printing out the error it is catching in `expect(fn).to.throw()` in the console. Annoying, but I stepped through the RxJS code and verified the behavior was correct. Related ReactiveX#5646

- Simplifies the call patterns in SafeSubscriber to not be quite so insane. - Note: for some reason one mocha test is passing, but still printing out the error it is catching in `expect(fn).to.throw()` in the console. Annoying, but I stepped through the RxJS code and verified the behavior was correct. Related #5646

JoostK commented Aug 15, 2020

View reviewed changes

JoostK marked this pull request as ready for review August 15, 2020 18:44

cartant added the blocked label Aug 16, 2020

JoostK mentioned this pull request Aug 16, 2020

perf(@angular-devkit/core): avoid RxJS performance penalty in sync fs calls angular/angular-cli#18549

Merged

benlesh added AGENDA ITEM Flagged for discussion at core team meetings and removed blocked labels Aug 23, 2020

JoostK and others added 2 commits August 24, 2020 15:53

benlesh force-pushed the perf_safe_subscriber branch from a1222ba to 2acf96b Compare August 24, 2020 20:56

benlesh requested a review from cartant August 24, 2020 20:57

benlesh approved these changes Aug 24, 2020

View reviewed changes

cartant approved these changes Aug 25, 2020

View reviewed changes

benlesh merged commit dfdef5d into ReactiveX:master Aug 25, 2020

benlesh mentioned this pull request Aug 25, 2020

refactor(SafeSubscriber): simplify methods #5676

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(SafeSubscriber): avoid using `Object.create` #5646

perf(SafeSubscriber): avoid using `Object.create` #5646

JoostK commented Aug 15, 2020 •

edited

Loading

JoostK Aug 15, 2020

JoostK Aug 15, 2020 •

edited

Loading

cartant commented Aug 16, 2020 •

edited

Loading

JoostK commented Aug 16, 2020

benlesh commented Aug 23, 2020

benlesh commented Aug 23, 2020

benlesh commented Aug 23, 2020 •

edited

Loading

benlesh commented Aug 23, 2020

cartant commented Aug 23, 2020 •

edited

Loading

benlesh commented Aug 24, 2020

cartant left a comment •

edited

Loading

cartant Aug 25, 2020 •

edited

Loading

benlesh Aug 25, 2020

perf(SafeSubscriber): avoid using Object.create #5646

perf(SafeSubscriber): avoid using Object.create #5646

Conversation

JoostK commented Aug 15, 2020 • edited Loading

Timings

JoostK Aug 15, 2020

Choose a reason for hiding this comment

JoostK Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

cartant commented Aug 16, 2020 • edited Loading

JoostK commented Aug 16, 2020

benlesh commented Aug 23, 2020

benlesh commented Aug 23, 2020

benlesh commented Aug 23, 2020 • edited Loading

benlesh commented Aug 23, 2020

cartant commented Aug 23, 2020 • edited Loading

benlesh commented Aug 24, 2020

cartant left a comment • edited Loading

Choose a reason for hiding this comment

cartant Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

benlesh Aug 25, 2020

Choose a reason for hiding this comment

perf(SafeSubscriber): avoid using `Object.create` #5646

perf(SafeSubscriber): avoid using `Object.create` #5646

JoostK commented Aug 15, 2020 •

edited

Loading

JoostK Aug 15, 2020 •

edited

Loading

cartant commented Aug 16, 2020 •

edited

Loading

benlesh commented Aug 23, 2020 •

edited

Loading

cartant commented Aug 23, 2020 •

edited

Loading

cartant left a comment •

edited

Loading

cartant Aug 25, 2020 •

edited

Loading