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

Remove dependency of Schedulers from ObservableRefCount #6452

Merged
merged 1 commit into from
Apr 4, 2019

Conversation

mycallmax
Copy link

@mycallmax mycallmax commented Apr 3, 2019

Resolves #6451.

In the constructor of ObservableRefCount that takes ConnectableObservable<T> source as the argument, we set timeout to 0L. In that specific use case of ObservableRefCount, scheduler is never needed. It's only referenced in cancel() method but if timeout is 0, it won't be triggered at all because there is early return. This commit removes the need to depend on Schedulers.trampoline() and instead passes null to be scheduler when the ref count is not time-based. Similarly, applies the same change to FlowableRefCount.

The reasons for this change are the following:

  1. In projects that don't depend on Schedulers class, if there is no reference to Schedulers, the whole Schedulers can be stripped out of the library after optimizations (e.g., proguard). With constructor that references Schedulers, the optimizer can't properly strip it out. In our quick test of our Android app, we were able to reduce the RxJava library size dependency from 51KB to 37KB (after optimization but before compression) by simply avoiding access to Schedulers in ObservableRefCount.
  2. In terms of modularity, ObservableRefCount is just an operator so it by itself should probably not have dependency on what available pool of schedulers (Schedulers) there are. It should just know that there is some Scheduler that could be passed to ObservableRefCount when ObservableRefCount needs it.

In the constructor of `ObservableRefCount` that takes `ConnectableObservable<T> source` as the argument, we set `timeout` to `0L`. In that specific use case of `ObservableRefCount`, `scheduler` is never needed. It's only referenced in `cancel()` method but if timeout is 0, it won't be triggered at all because there is early return. This commit removes the need to depend on `Schedulers.trampoline()` and instead passes null to be scheduler when the ref count is not time-based.

The reasons for this change are the following:

1. In projects that don't depend on `Schedulers` class, if there is no reference to `Schedulers`, the whole `Schedulers` can be stripped out of the library after optimizations (e.g., proguard). With constructor that references `Schedulers`, the optimizer can't properly strip it out. In our quick test of our Android app, we were able to reduce the RxJava library size dependency from 51KB to 37KB (after optimization but before compression) by simply avoiding access to `Schedulers` in `ObservableRefCount`.
2. In terms of modularity, `ObservableRefCount` is just an operator so it by itself should probably not have dependency on what available pool of schedulers (`Schedulers`) there are. It should just know that there is some `Scheduler` that could be passed to `ObservableRefCount` when `ObservableRefCount` needs it.
@mycallmax mycallmax marked this pull request as ready for review April 3, 2019 19:01
@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #6452 into 2.x will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #6452      +/-   ##
============================================
+ Coverage     98.27%   98.31%   +0.03%     
- Complexity     6296     6298       +2     
============================================
  Files           675      675              
  Lines         45173    45173              
  Branches       6246     6246              
============================================
+ Hits          44393    44410      +17     
+ Misses          241      238       -3     
+ Partials        539      525      -14
Impacted Files Coverage Δ Complexity Δ
...ernal/operators/observable/ObservableRefCount.java 100% <100%> (ø) 28 <1> (ø) ⬇️
.../internal/operators/flowable/FlowableRefCount.java 100% <100%> (ø) 28 <1> (ø) ⬇️
.../operators/flowable/FlowableBlockingSubscribe.java 93.02% <0%> (-4.66%) 10% <0%> (-1%)
...ernal/operators/flowable/FlowableFlatMapMaybe.java 90.82% <0%> (-3.87%) 2% <0%> (ø)
...activex/internal/observers/QueueDrainObserver.java 97.43% <0%> (-2.57%) 21% <0%> (-1%)
...ava/io/reactivex/processors/BehaviorProcessor.java 96.86% <0%> (-2.25%) 60% <0%> (ø)
...l/operators/observable/ObservableFlatMapMaybe.java 92.15% <0%> (-1.97%) 2% <0%> (ø)
...operators/observable/ObservableMergeWithMaybe.java 99.1% <0%> (-0.9%) 2% <0%> (ø)
.../operators/maybe/MaybeFlatMapIterableFlowable.java 97.54% <0%> (-0.82%) 2% <0%> (ø)
...ex/internal/operators/flowable/FlowableCreate.java 97.09% <0%> (ø) 6% <0%> (ø) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a78cfc...183f2e9. Read the comment docs.

@akarnokd akarnokd added this to the 2.2 backlog milestone Apr 3, 2019
@mycallmax
Copy link
Author

Thanks for the review and the approval!

@mycallmax mycallmax closed this Apr 3, 2019
@mycallmax mycallmax reopened this Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants