-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Large CompositeSubscription performance improvements #1145
Conversation
RxJava-pull-requests #1066 SUCCESS |
I need more time to review this ... looks like impressive work and analysis! Does this still make sense once #1000 is implemented and we can limit the buffering on operators like |
It depends on the usage pattern. If there is a burst of 1000s of new subscriptions, a bigger threshold is better. If there are lots of small additions and removals while having a moderate number of items in the composite, the lower threshold is better. I don't know a case where suddenly 1000s of subscriptions get created and added to the composite. |
This change has been incorporated into #1190 so I don't recommend merging this. I'll keep this open until the approach is approved or discarded. |
I tested these changes while playing with performance. As expected they don't play a role in normal master-performance branch
with CompositeSubscriptionSpeedup merged
reverted changes
|
But mine is a bit slower than the master and the 0.16.x. I think the reason is that by changing the size of objects, the allocator may run out of the cache line at different times. |
I'm closing this because of the new subscription containers have these optimizations in separate classes. |
This is a proposition to improve the addition/removal speed of the
CompositeSubscription
in case of thousands of items in it.In some operators, especially when using Schedulers and/or observeOn, thousands of items may be present in the composite and since adding/removing a new item is O(n), it takes more and more time to add and remove items once the composite gets large.
My proposal is to change the composite state implementation to switch to a HashSet representation above a certain threshold (and switch back below some). The following diagram shows some benchmarking results. (Configuration: i7 920 @ 2.4GHz, 32K L1 Data, 256K L2, 8M L3, 6GB DDR3 1333MHz RAM, Windows 7 x64, Java 8u05 x64.)
This benchmark how the CompositeSubscription behaves when there are some items already in it and a new unique item is added and removed immediately (all single threaded). The blue line indicates the current master implementation; the red, green and purple show the new implementation with thresholds set to 8, 16 and 24 respectively. Once the internal array size reaches the cache-line size, it is generally better to use HashSet instead.
The second benchmark compares how fast can the CompositeSubscription be filled with subscribers to a various capacity, and how the target size affects the fill speed.
When the composite is filled in, the one-by-one array resize performs better until a larger capacity is reached, but then again, using a HashSet to append further items is faster. Unfortunately, what seems to be an optimal threshold for the first case performs worse in this case.
There are some drawbacks of this hybrid approach: