Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: ensure same hidden class for OperatorSubscriber #5878

Merged
merged 2 commits into from Jan 11, 2021

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Nov 6, 2020

Description:

See the comments within the change. This PR should help realise one of the advantages of replacing operator-specific subscribers with a general-purpose subscriber - reducing/eliminating megamorphic property accesses within Subscriber.

I've not done any micro optimization/analysis, yet. I'll look at doing that after the version 7 release. However, I think that what's in this PR makes sense and shouldn't be controversial. 🤞

Related issue (if exists): None

@cartant cartant requested a review from benlesh November 6, 2020 08:18
@cartant cartant force-pushed the operator-subscriber-hidden-class branch from a2f7528 to caa37d5 Compare November 6, 2020 08:27
@cartant
Copy link
Collaborator Author

cartant commented Nov 6, 2020

I don't understand this CI failure. It seems to have something to do with side effects? I ran test:side-effects:update, but it seemed to generate a surprising number of changes, so ... 🤷‍♂️

@cartant cartant force-pushed the operator-subscriber-hidden-class branch 2 times, most recently from 363978e to f3f5eb6 Compare November 6, 2020 09:37
@benlesh
Copy link
Member

benlesh commented Nov 17, 2020

So the CI errors are from the side-effects checker. However, I think if you implement the change I'm suggesting (which is to use super._next, super._error, etc), then you won't have those declarations at the top level, and the side-effects checker will be happy again.

@cartant
Copy link
Collaborator Author

cartant commented Nov 18, 2020

So the CI errors are from the side-effects checker.

Yeah, I can see that it's related to that: the checker has no way of knowing (with absolute certainty) that the property accesses are not calls to getters. (It might not know that these aren't side effects, but we do.) I was mostly concerned about the diff after running the update script. It seemed to be unnecessarily large. I'll have another look at it after #5898 is merged/addressed, as that problem is now breaking the build.

@cartant cartant force-pushed the operator-subscriber-hidden-class branch from f3f5eb6 to 807bde1 Compare November 22, 2020 00:07
@benlesh
Copy link
Member

benlesh commented Jan 11, 2021

Sorry, this one almost fell off my radar, @cartant. Thanks for the work!

@benlesh benlesh merged commit 246b449 into ReactiveX:master Jan 11, 2021
@cartant cartant deleted the operator-subscriber-hidden-class branch March 19, 2021 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants