Proposal/rehash: Use async Messaging #416
Conversation
This is a main thread scheduler that's optimized to directly execute given runnables if it's already on the main thread, rather than always scheduling.
|
Note that this is upstreaming from our internal implementation at Uber. We've been using this in our production apps for a little over a year now. From some discussion with @JakeWharton offline: does it make sense for a scheduler that doesn't always necessarily "schedule"? Some potential precedent of this in RxJava include |
|
I think I expressed my views in the prior art. Doing it plainly may result in a stack overflow, adding a main-thread trampoline may add overhead negating the optimization and supporting task disposal may become quite complicated. |
|
Agreed the main thread trampoline adds a lot of overhead. Is it really necessary though? It seems like that risks duplicating what the main thread looper does under the hood. For stack overflow, is that due to stacktraces no longer being chopped? I didn't quite follow that bit |
Worker w = new FastPathScheduler().createWorker();
Schedulers.single().scheduleDirect(() -> w.dispose(), 10, TimeUnit.SECONDS);
w.schedule(new Runnable() {
@Override public void run() {
w.schedule(this);
}
});
Thread.sleep(11000);Trampoline would limit the stack depth. |
|
Doesn't this potentially violate the serial requirement for schedulers? If you have an upstream observable that emits a value which must be scheduled on the handler, followed closely by an emission which can be immediate (or trampolined), the second emission could easily pass through to the subscriber before the first. My first thought for solving this problem would be to only take the fast path when there are no queued emissions. |
ZacSweers
changed the title
|
Hey all - after a lot of discussion offline and with the Android framework team, I've updated the PR with a new solution that relies on asynchronous |
I thought the if check was enough but I can never remember the rules of this
|
I hate Travis CI sometimes. Will debug more after allowing time for discussion |
| @@ -47,6 +48,28 @@ public static Scheduler from(Looper looper) { | |||
| return new HandlerScheduler(new Handler(looper)); | |||
| } | |||
|
|
|||
| @TargetApi(Build.VERSION_CODES.P) | |||
There was a problem hiding this comment.
I agree! But lint fails without it despite the if-check :|
There was a problem hiding this comment.
Did you try just using 28 as a literal? I prefer avoiding the codenames (which are thankfully almost dead) and letters (which we should be trying to kill next).
There was a problem hiding this comment.
Huh, yeah using just 28 worked. c9e2bbf
RIP dessert club
| .getConstructor(Looper.class, Handler.Callback.class, boolean.class) | ||
| .newInstance(looper, null, true); | ||
| } catch (IllegalAccessException e) { | ||
| return new Handler(looper); |
There was a problem hiding this comment.
Just make these all empty and hoist a single return outside.
There was a problem hiding this comment.
Good catch 551b7b4
| private static Handler createAsyncHandler(Looper looper) { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) { | ||
| return Handler.createAsync(looper); | ||
| } else { |
There was a problem hiding this comment.
else not required after return
|
|
||
| static final Scheduler DEFAULT = new HandlerScheduler(new Handler(Looper.getMainLooper())); | ||
| static final Scheduler DEFAULT | ||
| = new HandlerScheduler(createAsyncHandler(Looper.getMainLooper())); |
There was a problem hiding this comment.
This seems like it's going to require a major version update. The implications to the entirety of the world which uses RxAndroid are scary and a silent change in a minor release doesn't seem appropriate.
There was a problem hiding this comment.
I agree, but a major change would require breaking with keeping the same major version as rxjava :|.
Either that or maybe do an extended preview release and ask for feedback?
There was a problem hiding this comment.
I think we overload from(Looper) with a version that takes a boolean for async just like the Handler constructor no matter what.
As to getting it into mainThread(), you can use the RxAndroidPlugins to install an init callback for its creation which you can then delegate to from(Looper.getMainLooper(), true).
I think that's the minimum-viable API to at least get this into a release now for people to start testing it. And from there we can determine whether or not it's even possible to make it the default behind mainThread() in the next major version–or sometime in the future–and whether or not we need to expose a vsyncMainThread() or similar accessor to a singleton instance for code which truly needs the old behavior.
There was a problem hiding this comment.
That works too! For some reason I had it in my head that you were against having support for both. Can update. Should I add a brief mention/example in the readme with it or save that for changelog?
|
Can we get rid of the use of |
|
We could but that API was only added (made public?) on API 22. If it was just private before we'd still need reflection. |
|
The functionality didn't exist before 22, just like the constructor on Handler you're currently reflecting into: |
|
Oh wtf. I could have sworn I went back and saw the handler constructor on older APIs (my understanding was it existed with the introduction of project butter circa API 17). In that case, yeah we can avoid reflection. Probably doesn't make sense to use the new |
|
We already use |
|
So keep the handler then? I was thrown off by
|
|
We'd also have to promote |
|
The boolean comes in through `from` which creates the `HandlerScheduler`.
That's the only public API change.
…On Fri, Jun 29, 2018, 5:56 PM Zac Sweers ***@***.***> wrote:
We'd also have to promote HandlerScheduler to be a public API, which it
isn't right now
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#416 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEZ4buyT18dNwAUfVw4mUEAO-n1w4ks5uBqKmgaJpZM4SCP9r>
.
|
|
Hi, chiming in from the Android team to give a general thumbs up to skipping the vsync barriers here. Ordering between "async" messages is still preserved; we made waiting until after the barriers the default behavior back in Jellybean because there's a bunch of UI code out there that expects a message or runnable to be processed after any pending view tree traversals (measure/layout/draw) which wait for vsync after Jellybean. The tradeoff for the compatibility is a lot of latency that most use cases shouldn't have to pay for, which this proposal addresses. The Message.setAsynchronous method has been in the codebase since API 16 (jellybean - https://android.googlesource.com/platform/frameworks/base/+/jb-release/core/java/android/os/Message.java#397) and Handler's constructor with the boolean async param has been there since API 17 (jellybean-mr1 - https://android.googlesource.com/platform/frameworks/base/+/jb-mr1-release/core/java/android/os/Handler.java). It would be safe to opportunistically call either one of these using reflection (or for Message.setAsynchronous just by building against a newer sdk and using a different API level check/catch if something goes wrong) as of these API levels, provided that when you're running on a platform version where it's officially available you call the real public API. This is the policy we use for reflection in the Android support libraries - as long as the implementation is public API clean as of some published API level the use of reflection isn't defining de facto public API that we have to support into the future. |
| useAsync = false; | ||
| } | ||
| } | ||
| return new HandlerScheduler(new Handler(looper), useAsync); |
There was a problem hiding this comment.
You already created the Handler above. Can pass it here.
| message.setAsynchronous(true); | ||
| } catch (NoSuchMethodError e) { | ||
| useAsync = false; | ||
| } |
There was a problem hiding this comment.
We should recycle the Message to be a good citizen!
|
Alright, reverted the sdk 28 changes to punt the CI issues for now and also tweaked it to only do the |
ZacSweers
changed the title
| * locking. On API < 16, this will no-op. | ||
| * @see android.os.Message#setAsynchronous(boolean) | ||
| */ | ||
| @SuppressLint("NewApi") |
There was a problem hiding this comment.
I guess because the conditional is reversed it is
| * | ||
| * @param async if true, the scheduler will use async messaging on API >= 16 to avoid VSYNC | ||
| * locking. On API < 16, this will no-op. | ||
| * @see android.os.Message#setAsynchronous(boolean) |
There was a problem hiding this comment.
shouldn't need to specify package since this is now imported
|
Merged as 191a758 |
|
to try that feature i need to init it in the Application onCreate() method, right? |
|
@ahulyk here is an article on how to use it, https://medium.com/@sweers/rxandroids-new-async-api-4ab5b3ad3e93 |
This PR changes the default main thread
Handlerto use asynchronous messaging. This allows us to avoid VSYNC locking that otherwise pushes everypostto the next frame.This works by relying on the new
Handler.createAsyncfactory in API 28, and on pre-28 it will reflectively fall back to a private constructor of Handler (that's always existed) to enable this.This should give us the original goal of #228 with the safety of still leaning on the main thread looper and not trying to directly run things ourselves and risking deadlocks/trampolining issues.
To use SDK 28, updated the AGP plugin and gradle along the way.
Prior discussion below
This is a main thread scheduler that's optimized to directly execute given Runnables if it's already on the main thread, rather than always scheduling.Prior art: #228Kept as a standalone implementation right now for review and discussion, but ideally this implementation would replace the existing main thread scheduler implementation as the default.