1.x: Optimizate single just #3642
Conversation
| public void call(final SingleSubscriber<? super R> child) { | ||
|
|
||
| Single<? extends R> o = func.call(value); | ||
| if (o.getClass() == ScalarSynchronousSingle.class) { |
There was a problem hiding this comment.
This can be instanceof as well.
|
Could you also squash the commits? |
|
In the master, there is a new class |
|
Ok, I will try to do it. |
|
@akarnokd I can't find class SingleSourcePerf in master, do you mean SinglePerfBaseline? |
|
I have run the benchmark on 1.x and this PR(Intel Core i5-2400 @3.10GHz, Windows 7 x64, Java 1.8.0_45-b15), the results are as follows: 1.x
this PR
The results show that most of scores in this PR is higher than 1.x, especially that flatmap and flatmapConnst have a great ascension. |
|
👍 |
|
rebased |
| if (o instanceof ScalarSynchronousSingle) { | ||
| child.onSuccess(((ScalarSynchronousSingle<? extends R>) o).value); | ||
| } else { | ||
| o.unsafeSubscribe(new Subscriber<R>() { |
There was a problem hiding this comment.
You need to put this new Subscriber to a new variable, and call child.add(...) to maintain the subscription chain.
There was a problem hiding this comment.
You can use this unit test to verify the behavior:
final Action0 unsubscribe = mock(Action0.class);
Single<Integer> s = Single.create(new OnSubscribe<Integer>() {
@Override
public void call(SingleSubscriber<? super Integer> subscriber) {
subscriber.add(Subscriptions.create(unsubscribe));
}
});
Subscription subscription = Single.merge(Single.just(s)).subscribe();
subscription.unsubscribe();
verify(unsubscribe, times(1)).call();
|
@Chaoba could you rebase against the master and fix the minor subscription issue? Thanks! |
|
@zsxwing Thanks for your comment. Should I squash the commits? |
|
The commits has been squashed |
|
👍 ping @akarnokd to take a final look and pull in :) |
|
👍 @artem-zinnatullin I'd like your opinion on this |
| @@ -1212,4 +1212,18 @@ public void iterableToArrayShouldConvertSet() { | |||
| assertSame(s1, singlesArray[0]); | |||
| assertSame(s2, singlesArray[1]); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testScalarFlatMap() { | |||
There was a problem hiding this comment.
test prefix is not needed :)
|
@Chaoba please add tests for: Otherwise looks great, will be happy to see it in next release! |
|
add some new testcases, now the coverage is as follows: |
|
👍 |
| Single.just(1).flatMap(new Func1<Integer, Single<Integer>>() { | ||
| @Override | ||
| public Single<Integer> call(Integer v) { | ||
| return Single.just(v); |
There was a problem hiding this comment.
Please return another result, at the moment test can pass even if flatMap returns same Single, you can also change type from Integer to let's say String during flatMap :)
|
Great set of tests! Just few comments left, @Chaoba please ping again once you fix them! (you can squash commits right away so we could merge it ASAP) |
|
ping @artem-zinnatullin to review again please. |
|
👍 thanks a lot, @Chaoba! (Looks like we can merge it now) |
|
👍 |
This PR add similar optimization as ScalarSynchronousObservable to Single.just method.