rxjava-android Poposal: Call action immediately in HandlerThreadScheduler if thread is the same #1102
Conversation
if action is scheduled in current looper thread than just invoke Action instead of posting it as Runnable
|
RxJava-pull-requests #1013 SUCCESS |
|
Looks good to me. Great idea, I was thinking to add this as well. I wonder if it should be optional however, sometimes you need to be sure to post a runnable (if modifying certain view properties during a layout pass for example). |
|
We have already a discussion here: #949
|
|
Maybe adding an option to the But it would require the user learning the implementations of operators. They needs to be aware of which operator will use recursive schedule. I'm not convinced by this idea. |
|
Yes, the main goal of this PR is to make So may be it will be better to add option (as @zsxwing suggest) to constructor, but leave existed constructor (with single Handler argument) as default recursive implementation (so it will have the same behavior as now). |
|
I remember there was a PR before which was pretty much the same. @zsxwing can you elaborate on why it doesn't work with recursive scheduling? In which cases do you use recursive scheduling? Unless I'm simply not aware of it, I don't think I ever had the need for it on Android. Maybe we can find a compromise, because I generally agree that the extra roundtrip through the main looper is unnecessary overhead. |
|
My comments are here: #949 (comment) and #949 (comment) |
|
@mattkay, and your comment: #949 (comment) |
it will allow to avoid rescheduling actions if scheduled from the same thread mostly usefull for mainThreadScheduler
|
How about this approach? |
|
RxJava-pull-requests #1052 FAILURE |
|
@zsxwing Could you elaborate again on a case where you need recursive scheduling? I'm not sure I understand when it's useful. I'm afraid we use a very simple, formulaic approach on Android, which is putting something on a background thread and observing it on the main thread. Is this used when rescheduling something as part of an observable transformation? In any event, I think we should get some numbers to back up that it actually improves performance. We should measure what the exact performance gains are on at least a few devices and a varying numbers of events. |
Now looks only @Yarikx, do you have some number to prove the performance improvement. Maybe this is a premature optimization. |
|
Sorry for delay. |
|
Ok, I check some metrics. I don't sure that the measurement is done right, but it's enough to make some assumptions. So, I created two instances of Handler handler = new Handler();
Scheduler immediate = AndroidSchedulers.handlerThread(handler, true);
Scheduler recursive = AndroidSchedulers.handlerThread(handler, false);The good partin this part i tested minimal delay for scheduling one item. This action was made in button handler, (after public void test(View view){
Handler handler = new Handler();
Scheduler immediate = AndroidSchedulers.handlerThread(handler, true);
Scheduler recursive = AndroidSchedulers.handlerThread(handler, false);
Observable.from(1)
.timestamp()
.observeOn(immediate)
.subscribe(new Action1<Timestamped<Integer>>() {
@Override
public void call(Timestamped<Integer> timestamped) {
long time = System.currentTimeMillis() - timestamped.getTimestampMillis();
Log.d("Immediate", "time = "+time);
}
});
Observable.from(1)
.timestamp()
.observeOn(recursive)
.subscribe(new Action1<Timestamped<Integer>>() {
@Override
public void call(Timestamped<Integer> timestamped) {
long time = System.currentTimeMillis() - timestamped.getTimestampMillis();
Log.d("Recursive", "time = "+time);
}
});
}and the output after few button clicks: So, actual gap between sending The bad partNow let's try to process some sequence public void test(View view){
Handler handler = new Handler();
Scheduler immediate = AndroidSchedulers.handlerThread(handler, true);
Scheduler recursive = AndroidSchedulers.handlerThread(handler, false);
Observable.range(0, 20)
.timestamp()
.observeOn(immediate)
.subscribe(new Action1<Timestamped<Integer>>() {
@Override
public void call(Timestamped<Integer> timestamped) {
long time = System.currentTimeMillis() - timestamped.getTimestampMillis();
Log.d("Immediate", "time = "+time);
}
});
Observable.range(0, 20)
.timestamp()
.observeOn(recursive)
.subscribe(new Action1<Timestamped<Integer>>() {
@Override
public void call(Timestamped<Integer> timestamped) {
long time = System.currentTimeMillis() - timestamped.getTimestampMillis();
Log.d("Recursive", "time = "+time);
}
});
}And the test result: Now we see that first item processing (on recursive scheduler) start only after finishing current task (emitting items to scheduler). Actually in current example there is not so much work to do, but in real apps there can be more UI related work, so sequence observing will delay even more. Another thing: As we process each item in it's own The ugly partLet's do last example, but add few more layers of scheduling, and we will run it on Observable.range(0, 20)
.timestamp()
.observeOn(immediate)
.observeOn(immediate)
.observeOn(immediate)
.observeOn(immediate)
.observeOn(immediate)
.observeOn(immediate)
.observeOn(immediate)
.subscribe(new Action1<Timestamped<Integer>>() {
@Override
public void call(Timestamped<Integer> timestamped) {
long time = System.currentTimeMillis() - timestamped.getTimestampMillis();
Log.d("Immediate", "time = "+time);
}
});
Observable.range(0, 20)
.timestamp()
.observeOn(recursive)
.observeOn(recursive)
.observeOn(recursive)
.observeOn(recursive)
.observeOn(recursive)
.observeOn(recursive)
.observeOn(recursive)
.subscribe(new Action1<Timestamped<Integer>>() {
@Override
public void call(Timestamped<Integer> timestamped) {
long time = System.currentTimeMillis() - timestamped.getTimestampMillis();
Log.d("Recursive", "time = "+time);
}
});and the output: So we see here huge gap, enough to be noticed by the user in UI. ConclusionOf course But for cases when we want to receive some expensive data, that can be cached (api call or bitmap), |
|
Sorry for the late reply. Thank you for your work, @Yarikx
Could you redo your test by Observable.range(0, 20)
.observeOn(immediate)
.timeInterval(immediate)
.subscribe(new Action1<TimeInterval<Integer>>() {
@Override
public void call(TimeInterval<Integer> interval) {
Log.d("Immediate", "time = " + interval.getIntervalInMilliseconds());
}
}); |
|
In addition, make sure public void testImmediate() {
Handler handler = new Handler();
final Scheduler immediate = AndroidSchedulers.handlerThread(handler, true);
immediate.createWorker().schedule(new Action0() {
@Override
public void call() {
Observable.range(0, 20)
.observeOn(immediate)
.timeInterval(immediate)
.subscribe(new Action1<TimeInterval<Integer>>() {
@Override
public void call(TimeInterval<Integer> interval) {
Log.d("Immediate", "time = " + interval.getIntervalInMilliseconds());
}
});
}
});
}
public void testRecursive() {
Handler handler = new Handler();
final Scheduler recursive = AndroidSchedulers.handlerThread(handler, false);
recursive.createWorker().schedule(new Action0() {
@Override
public void call() {
Observable.range(0, 20)
.observeOn(recursive)
.timeInterval(recursive)
.subscribe(new Action1<TimeInterval<Integer>>() {
@Override
public void call(TimeInterval<Integer> interval) {
Log.d("Recursive", "time = " + interval.getIntervalInMilliseconds());
}
});
}
});
} |
|
In general I'd be very careful with such manual measurements. Have a look at this document outlining how deceiving micro benchmarks can be: http://www.ibm.com/developerworks/java/library/j-jtp02225/index.html The lack of authoritative documentation for Dalvik and the optimizations it applies makes that even worse. Another thing I'm worried about is that introducing this change will in fact change the behavior of client code in some cases, so we might see application code failing. Here's why: Say in one of your fragments, you create an observable and connect it in Given, connecting a synchronous observable in fragment onCreate is not very likely. I'm just saying, these are things that have to be considered. |
|
In fact, after some consideration, I'm against applying this patch. I think of In other words, we're still fulfilling Android's message loop contract, since we're merely adapting its API to fulfill RxJava's Scheduler contract in terms of an Android message loop. By applying optimizations like these, we're bypassing the message loop and create non-deterministic behavior where sometimes the message loop is involved, sometimes it isn't. This will make it difficult to reason about failure on the call site unless this behavior is clearly documented. Perhaps there is a way to make this explicit in the type system or in the Scheduler APIs? Another option would be to introduce a variant of |
|
@mttkay I think you are right. After some thoughts I also don't think this is the best way to do operations on handler. I still think that there is better way to deal with main thread, but it's not here right now =) So I will close this PR. |
By avoiding unnecessary Runnable creation, and posting it to handler we can decrease overhead when scheduling is made from the same thread.
Current Activity.runOnUIThread is implemented in the same way: