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(Subscription): improve parent management #4526

Merged
merged 1 commit into from
Feb 1, 2019

Conversation

bmeurer
Copy link
Contributor

@bmeurer bmeurer commented Feb 1, 2019

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 Subscriptions (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.

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.
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8037

  • 28 of 29 (96.55%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 96.805%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/internal/Subscription.ts 25 26 96.15%
Totals Coverage Status
Change from base Build 8025: 0.05%
Covered Lines: 5242
Relevant Lines: 5415

💛 - Coveralls

@benlesh benlesh merged commit 06f1a25 into ReactiveX:master Feb 1, 2019
@bmeurer bmeurer deleted the Subscription_parents branch February 1, 2019 20:20
@benlesh
Copy link
Member

benlesh commented Feb 1, 2019

Thanks again, @bmeurer! I really appreciate the work you're doing.

@bmeurer
Copy link
Contributor Author

bmeurer commented Feb 1, 2019

Thanks 👍

@lock lock bot locked as resolved and limited conversation to collaborators Mar 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants