-
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
Subjects Refactor - Non-Blocking, Common Abstraction, Performance #651
Merged
benjchristensen
merged 8 commits into
ReactiveX:master
from
benjchristensen:subjects-fixes
Dec 23, 2013
Merged
Subjects Refactor - Non-Blocking, Common Abstraction, Performance #651
benjchristensen
merged 8 commits into
ReactiveX:master
from
benjchristensen:subjects-fixes
Dec 23, 2013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Common logic composed inside SubjectSubscriptionManager - ReplaySubject does not block while replaying to new subscribers - Added unit tests and fixed behavior while reviewing with @headinthebox compared to Rx.Net - Uses mostly non-blocking approach (I believe it’s all correct, unit and long running tests have been used to prove it. The tests found concurrency problems during development and became stable once I got the design correct. As with all concurrent code I may be missing something.)
- previous commit got non-blocking working but perf tests showed it slow - this commit retains non-blocking but surpasses master branch performance Master branch: 11,947,459 ops/sec This commit: 16,151,174 ops/sec
Closed
RxJava-pull-requests #584 SUCCESS |
import java.util.HashMap; | ||
import java.util.Map; | ||
import java.util.concurrent.CountDownLatch; | ||
import java.util.concurrent.atomic.AtomicBoolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import
This was referenced Dec 22, 2013
- Performance difference between changes are trivial at best so preferring to keep code that is easier to understand. Test numbers: ``` Old SafeObserver Run: 0 - 3,751,704 ops/sec Run: 1 - 34,619,086 ops/sec Run: 2 - 30,483,715 ops/sec Run: 3 - 27,746,023 ops/sec Run: 4 - 54,078,608 ops/sec Run: 5 - 55,107,045 ops/sec Run: 6 - 53,935,396 ops/sec Run: 7 - 54,947,168 ops/sec Run: 8 - 57,024,246 ops/sec Run: 9 - 55,059,712 ops/sec Run: 10 - 56,904,832 ops/sec Run: 11 - 55,919,967 ops/sec Run: 12 - 55,076,087 ops/sec Run: 13 - 55,066,685 ops/sec Run: 14 - 55,025,476 ops/sec Run: 0 - 3,839,266 ops/sec Run: 1 - 34,115,371 ops/sec Run: 2 - 29,675,175 ops/sec Run: 3 - 28,677,042 ops/sec Run: 4 - 55,405,652 ops/sec Run: 5 - 55,260,220 ops/sec Run: 6 - 55,147,464 ops/sec Run: 7 - 54,261,126 ops/sec Run: 8 - 53,941,505 ops/sec Run: 9 - 54,324,501 ops/sec Run: 10 - 55,125,576 ops/sec Run: 11 - 56,102,870 ops/sec Run: 12 - 55,061,834 ops/sec Run: 13 - 55,476,039 ops/sec Run: 14 - 55,073,054 ops/sec Run: 0 - 3,704,536 ops/sec Run: 1 - 34,694,514 ops/sec Run: 2 - 30,778,227 ops/sec Run: 3 - 28,441,329 ops/sec Run: 4 - 54,116,946 ops/sec Run: 5 - 55,204,699 ops/sec Run: 6 - 54,859,450 ops/sec Run: 7 - 55,214,757 ops/sec Run: 8 - 55,005,500 ops/sec Run: 9 - 55,339,118 ops/sec Run: 10 - 55,501,903 ops/sec Run: 11 - 55,074,570 ops/sec Run: 12 - 55,102,187 ops/sec Run: 13 - 55,756,278 ops/sec Run: 14 - 54,768,411 ops/sec New SafeObserver Run: 0 - 3,983,308 ops/sec Run: 1 - 34,767,250 ops/sec Run: 2 - 30,806,957 ops/sec Run: 3 - 29,855,113 ops/sec Run: 4 - 57,451,453 ops/sec Run: 5 - 55,515,152 ops/sec Run: 6 - 56,086,822 ops/sec Run: 7 - 56,295,529 ops/sec Run: 8 - 55,371,905 ops/sec Run: 9 - 55,816,653 ops/sec Run: 10 - 55,793,296 ops/sec Run: 11 - 56,011,426 ops/sec Run: 12 - 55,568,521 ops/sec Run: 13 - 55,396,137 ops/sec Run: 14 - 56,353,267 ops/sec Run: 0 - 3,933,367 ops/sec Run: 1 - 34,498,342 ops/sec Run: 2 - 30,233,584 ops/sec Run: 3 - 29,179,785 ops/sec Run: 4 - 55,761,874 ops/sec Run: 5 - 55,948,124 ops/sec Run: 6 - 55,264,801 ops/sec Run: 7 - 56,267,020 ops/sec Run: 8 - 57,474,567 ops/sec Run: 9 - 55,879,657 ops/sec Run: 10 - 55,998,880 ops/sec Run: 11 - 56,044,073 ops/sec Run: 12 - 55,498,515 ops/sec Run: 13 - 56,204,720 ops/sec Run: 14 - 55,845,954 ops/sec Run: 0 - 3,981,914 ops/sec Run: 1 - 34,160,822 ops/sec Run: 2 - 30,873,631 ops/sec Run: 3 - 29,135,067 ops/sec Run: 4 - 55,845,330 ops/sec Run: 5 - 55,101,883 ops/sec Run: 6 - 55,724,276 ops/sec Run: 7 - 56,085,564 ops/sec Run: 8 - 55,639,942 ops/sec Run: 9 - 56,464,955 ops/sec Run: 10 - 55,453,275 ops/sec Run: 11 - 56,115,463 ops/sec Run: 12 - 56,509,945 ops/sec Run: 13 - 53,863,348 ops/sec Run: 14 - 55,866,858 ops/sec ```
benjchristensen
added a commit
that referenced
this pull request
Dec 23, 2013
Subjects Refactor - Non-Blocking, Common Abstraction, Performance
RxJava-pull-requests #591 SUCCESS |
rickbw
pushed a commit
to rickbw/RxJava
that referenced
this pull request
Jan 9, 2014
Subjects Refactor - Non-Blocking, Common Abstraction, Performance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes evolved out of reviewing pull request #605 and code reviewing master branch with @headinthebox and validating functionality against .Net. I also spent some time to figure out a non-blocking design that I believe is working.
The results for
ReplaySubject
is that it performs faster than what is currently in master and does not block in almost all cases. I have not yet perf-tested the other 3 Subjects.onComplete
/onError
are in process. It will wait for those to complete before applying the terminal state on itself.As for performance, for relative comparison my machine gets these numbers for the master branch:
This commit gets these numbers:
I'm curious about adding bounded support and using a circular-array rather than ArrayList and seeing if it performs better.
The performance tests are inside
SubjectPerformanceTests
and inspired by tests done in the non-blocking code workshop with Martin Thompson.Some of the code is not as elegant as I'd like but was done for performance reasons. For example, I had a much more elegant version using a simple linked list but it had horrible performance. Another example is that using
Notification
to wrap everyonNext
so we have a single data structure is very inefficient so it maintainsT
without wrapping and then conditional logic to check for terminal state. These and other things have been determined while doing perf tests during development and I have consciously moved towards performance rather than the most elegant code.I'd appreciate feedback on concurrency problems if any exist or ways of getting more performance out while achieving the same functional requirements.