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

Subjects Refactor - Non-Blocking, Common Abstraction, Performance #651

Merged
merged 8 commits into from
Dec 23, 2013

Conversation

benjchristensen
Copy link
Member

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.

  • Common logic composed inside SubjectSubscriptionManager used by all 4 Subjects
  • 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.)
    • The only place it blocks is if a new Observer subscribes, completes replay and 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:

     * ArrayList with raw values & synchronized access
     * 
     * Run: 10 - 11,993,341 ops/sec
     * Run: 11 - 11,719,523 ops/sec
     * Run: 12 - 11,965,214 ops/sec
     * Run: 13 - 11,814,730 ops/sec
     * Run: 14 - 11,947,459 ops/sec

This commit gets these numbers:

     * ArrayList with raw values & non-blocking (no synchronization)
     * 
     * Run: 10 - 16,069,678 ops/sec
     * Run: 11 - 15,954,688 ops/sec
     * Run: 12 - 16,158,874 ops/sec
     * Run: 13 - 16,209,504 ops/sec
     * Run: 14 - 16,151,174 ops/sec

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 every onNext so we have a single data structure is very inefficient so it maintains T 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.

- 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
@cloudbees-pull-request-builder

RxJava-pull-requests #584 SUCCESS
This pull request looks good

import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicBoolean;
Copy link
Member

Choose a reason for hiding this comment

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

Unused import

akarnokd and others added 4 commits December 22, 2013 22:24
- 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
@benjchristensen benjchristensen merged commit 44c3739 into ReactiveX:master Dec 23, 2013
@benjchristensen benjchristensen deleted the subjects-fixes branch December 23, 2013 18:36
@cloudbees-pull-request-builder

RxJava-pull-requests #591 SUCCESS
This pull request looks good

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants