fix to remove drift from schedulePeriodic #1162
Conversation
|
RxJava-pull-requests #1077 FAILURE |
|
Very nice, thanks @kirkshoop. While considering this I though of something but I haven't played with this enough to know exactly how it will behave, but if someone chooses an interval of 1ms, and the work being done each time takes longer than 1ms, what is the expected behavior? We should never have concurrent execution. If I understand correctly it will result in a negative delay and result in scheduling it immediately, but obviously have drift. |
|
From what I can tell it does the right thing and just delays rather than allow concurrent execution. public class periodic {
public static void main(String[] args) {
final Scheduler scheduler = Schedulers.immediate();
final Scheduler.Worker w = scheduler.createWorker();
final long initial = 100;
final long period = 100;
final long start = scheduler.now() + initial;
w.schedulePeriodically(new Action0() {
long count = 0;
@Override
public void call() {
try {
Thread.sleep(1500);
} catch (InterruptedException e) {
}
long tick = scheduler.now();
System.out.println(String.format("expected -> %dms, actual -> %dms, drift -> %dms", count*period, tick - start, tick - (start + (count*period))));
++count;
}
}, initial, period, TimeUnit.MILLISECONDS);
}
}Based on this I'm merging this as it is safe in the degraded case while being better in the normal case (work being less time than the interval). |
fix to remove drift from schedulePeriodic
|
Yes, that was the behavior before and I kept it. There were only two options and skipping a period seemed like the wrong policy to me. The user supplies both the function and the period and so they own the behavior (they can return immediately to skip and get back on track, or just ensure that the period was sufficient to begin with. On May 6, 2014, at 9:11 AM, "Ben Christensen" <notifications@github.commailto:notifications@github.com> wrote: Very nice, thanks @kirkshoophttps://github.com/kirkshoop. While considering this I though of something but I haven't played with this enough to know exactly how it will behave, but if someone chooses an interval of 1ms, and the work being done each time takes longer than 1ms, what is the expected behavior? We should never have concurrent execution. If I understand correctly it will result in a negative delay and result in scheduling it immediately, but obviously have drift. Reply to this email directly or view it on GitHubhttps://github.com//pull/1162#issuecomment-42323326. |
|
This is an excellent improvement. I'm working on something that does microsecond level scheduling, and this change makes it work very accurately: Without this change, it isn't even close: |
When implementing the 'worker' scheduler pattern in rxcpp I implemented schedulePeriodic differently because I predicted that the RxJava version would drift. Once the changes were working, I verified my prediction in rxcpp.
I decided to test the same in RxJava and contribute my approach.
This was my test:
The existing impl causes this output:
With this commit the test outputs: