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

Reduce the scope of locking to increase parallelism. #1043

Closed
wants to merge 1 commit into from

Conversation

vigdorchik
Copy link
Contributor

In my rxmon project https://github.com/vigdorchik/rxmon I make heavy use of combineLatest. This change that reduces the scope of locking should help increase combineLatest throughput. I wish I could report the speedup numbers, but I couldn't find how to publish RxJava to maven locally to try it out.

@cloudbees-pull-request-builder

RxJava-pull-requests #961 FAILURE
Looks like there's a problem with this pull request

@vigdorchik
Copy link
Contributor Author

I saw this failure without my change, and I believe it's unrelated.

On Wed, Apr 16, 2014 at 5:14 PM, CloudBees pull request builder plugin <
[email protected]> wrote:

RxJava-pull-requests #961https://netflixoss.ci.cloudbees.com/job/RxJava-pull-requests/961/FAILURE
Looks like there's a problem with this pull request


Reply to this email directly or view it on GitHubhttps://github.com//pull/1043#issuecomment-40596720
.

@vigdorchik
Copy link
Contributor Author

Review by @benjchristensen and @akarnokd.

@benjchristensen
Copy link
Member

I'll take a look ... sounds like a good change.

We have started doing perf testing using JMH. You can take a look at examples here: https://github.com/Netflix/RxJava/tree/master/rxjava-core/src/perf/java/rx/operators

It would be useful to add one for this operator to measure before/after performance.

@akarnokd
Copy link
Member

The change looks okay but not certain about the performance gains. Two other things:

  • It would be great if you could rewrite it to the new Operator/Subscriber idiom.
  • The original had a race condition between next and error, you'd need to wrap the incoming observer/subscriber into a SerializedSubscriber.

@vigdorchik
Copy link
Contributor Author

This change doesn't work. It allows onCompleted before all onNext have been triggered.

@vigdorchik vigdorchik closed this Apr 17, 2014
@akarnokd akarnokd mentioned this pull request Apr 23, 2014
57 tasks
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.

4 participants