Diff on background thread for EpoxyController

[elihart]

Diffing is done on the main thread right now. I don't think there are any limitations to moving it to the background thread for Epoxy Controller, which would be a nice optimization. We just have to make sure that we handle multiple diff calls at the same time correctly.

[niccorder]

I'll go ahead and hop on this issue as well.

Should we just have a private final Executor diffExecutor = Executors.newSingleThreadExecutor() live inside the EpoxyController and schedule diffs on this thread? Also, I haven't had the opportunity to handle testing in multi-threaded environments. I will most likely need your feedback for testing this.

[elihart]

Awesome that you can work on this!

I think all EpoxyControllers could share the same thread so we don't have to keep creating new threads.

I'm not sure if it would be better to use AsyncTask.THREAD_POOL_EXECUTOR or create our own thread like you suggested.

I also don't have much experience with building multi-threaded systems so I can't give a lot of direction, but happy to discuss and code review. I should be able to help more with the Epoxy DiffHelper code changes.

[niccorder]

Thanks for the info @elihart. Looking forward to the feedback on the PR.

I wanted to ask why all controllers should share the same thread? I thought it was a 1:1 relationship between controller to adapter. Thanks for your insight! I want to get as much info before beginning on this.

[elihart]

@niccorder you're right that its a 1:1 relationship between controller and adapter, and generally only one controller should be diffing at one time. However, if most screens are using Epoxy it seems wasteful to create a new thread for each screen. Also, diffing usually only takes a few milliseconds and doesn't happen too often, so having a dedicated thread seems like overkill.

[mradzinski]

@elihart I beg to differ (no pun intended 😅) regarding your statement that diffing ... doesn't happen too often.

Actually, I just stumbled an app using Epoxy where diffing happened at least 40 times per minute. It's an app backed up by a real-time DB (let's call it Firebase for simplicity), and since the only way of letting Epoxy know there is a change in the models (this app had listeners for likes, votes and things like that, so models changed a lot) without throwing the existent models away is to pass an ever-growing list of objects to the setData method of the controllers then full diffing was happening every time someone liked or voted on a post for example.

Diffing can happen very often. This is just an example, but if you put into consideration this abovementioned app can end up having millions of users then diffing will end up happening more than once a second.

[elihart]

@mradzinski thanks for sharing - in cases like that it is best practice to use requestDelayedModelBuild to debounce events. a delay of a second or so would prevent model rebuilding and diffing from happening too much much (to the point of causing bad performance).

In that regard I don't think that background diffing would help - it is more useful in the case that a single diffing event takes a long time (long enough to drop frames). Diffing can take a long time if there are many models ( roughly more than ~100, depending on model complexity). With the new paging controller though the hope is that we can optimize and not build all models at once, so that diffing stays fast.

Between delayed model builds and paging I have been thinking that diffing might not be necessary. It is great to hear real world example cases though to see what is and isn't working so we know what to build. Otherwise I have been hesitant to implement background diffing if nobody is requesting it.

[niccorder]

Hey @mradzinski, It's funny you brought this up! Eli and I had a chat and I brought up similar examples at that time. With the paging extension Eli is referring to, I agree that his logic is correct. However, you still get alot of diffs if you don't use this. Perhaps it should be documented better @elihart?

…

On Wed, Nov 8, 2017 at 3:40 PM Eli Hart ***@***.***> wrote: @mradzinski <https://github.com/mradzinski> thanks for sharing - in cases like that it is best practice to use requestDelayedModelBuild to debounce events. a delay of a second or so would prevent model rebuilding and diffing from happening too much much (to the point of causing bad performance). In that regard I don't think that background diffing would help - it is more useful in the case that a single diffing event takes a long time (long enough to drop frames). Diffing can take a long time if there are many models ( roughly more than ~100, depending on model complexity). With the new paging controller though the hope is that we can optimize and not build all models at once, so that diffing stays fast. Between delayed model builds and paging I have been thinking that diffing might not be necessary. It is great to hear real world example cases though to see what is and isn't working so we know what to build. Otherwise I have been hesitant to implement background diffing if nobody is requesting it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#286 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGBw_Z7Na_enJ0uOtiWiqQ_1NUmXRGG-ks5s0jvfgaJpZM4PZR9b> .

[mradzinski]

@elihart Yep, I thought of requestDelayedModelBuild then, however, how's that achievable using a TypedEpoxyController? AFAIK that type of controller requests the build when you call setData and if you attempt to do it by hand you hit an exception, right? That build is requested using requestModelBuild() if I'm correct (haven't checked that code in a few, going by heart here)

So for me there's a huge tradeoff there: You either stick to an EpoxyController and give up on the goodness of the TypedEpoxyController (extremely predictable, readable, and testable) or you go with it and hope not to deal with diffing performance issues. The solution would be to overload the setData() method to allow the user to pass in some delay in ms and use requestDelayedModelBuild instead of requestModelBuild. I think that would pretty much solve it up and allow to use either types of controllers to achieve delayed builds. One can do this by hand (lets say by copy-pasting the code of a typed controller and just replace the calls in setData) but I guess Epoxy could also provide this out of the box.

@niccorder Yeah, I agree that the paging extension would help, but there's another tradeoff to be aware of: No way of programmatically scrolling through it (at least not yet).

[elihart]

You're right about TypedEpoxyController not supporting delayed model builds, and also right that it wouldn't be too hard to add support to the typed controllers for delaying builds.

Adding a second parameter to setData may be confusing, since we also have typed controller that take data as multiple parameters. Instead, we could set a debounce time with setDebounceTime and then have setDataDebounced.

If you want to put up a PR for this I'd be happy to review, otherwise it would take me some time to get to as I'm currently a bit swamped with other projects.

The programmatic scrolling of paged lists is something that I believe can be supported, but it is not something we currently need internally so I haven't prioritized it, so same as above applies here. If you are in need of it it would be great to start a new issue dedicated to it. The basic idea I have to solve it is to add a method to the paging controller to scrollTo to have the controller rebuild models around that index and then have the recyclerview scroll to that position - not sure yet how that coordination would work or if it needs to happen synchronously

[gajicm93]

Support libraries from recently include ListAdapter which is a light RecyclerView.Adapter wrapper with a proper DiffUtil integration on the background thread. It is currently part of the PagingLibrary, but should soon be separated into a "RecyclerView extensions" library, and can be used normally without Paging..

I think it would be the best and easiest to change BaseEpoxyAdapter to extend this new ListAdapter instead of RecyclerView.Adapter, and accommodate Epoxy's diff calculation to work via DiffUtil, and you're done, the rest will work out of the box.

EDIT: Even if ListAdapter API shows to be too restrictive for Epoxy use case, they have a ListAdapterHelper class for more advanced use cases which can be used to easily integrate background DiffUtil into existing RecyclerView.Adapter directly. Just read the docs on their page.

With these helpers you don't have to literally do anything about threading, race conditions, etc.. You just have to make Epoxy use DiffUtil for diff calculations instead of it's own algorithm, and you get everything else out of the box.

[elihart]

@gajicm93 Yeah, that is a very reasonable idea and would simplify things nicely.

The reason I haven't done that yet is because Epoxy's custom diffing was significantly faster last time I tested it (when DiffUtil first came out). DiffUtil computes a diff from scratch every time, but Epoxy saves results of the previous diff and leverages hashmaps to diff more quickly (at the expense of using more memory).

I would be curious to run benchmarks again, but even if DiffUtil is slower, it may not matter much if it is all happening in the background.

[gajicm93]

@elihart Exactly. I think it's much better and safer and less-bug-prone in every way to rely on code Google is writing and most importantly maintaining, instead to introduce unnecessary complexity into Epoxy that you will have to maintain yourself forever.

And as you say, even if DiffUtil is a bit slower, it doesn't matter as long as it's on a background thread, and once again, Google is maintaining it and making it faster with every release (hopefully).

[mradzinski]

@elihart @gajicm93 Last time I benchmarked Google's DiffUtil against Epoxy's one (last month), the one made by Google was significantly slower (sometimes even in the order of 10x) on sets of 100 elements and more (on sets smaller than that is barely noticeable, but the bigger the set grows the slower it became... and it wasn't exactly a straight relationship between set size and speed).

On the other hand, I don't actually think they are improving their Diffing algorithm at all. Since it was announced and through the last couple of iterations no code has been added nor deleted (you can check this yourselves by simply going through Google's library code), so I don't know if they plan on making it faster or that's good enough for them.

Just my 2C.

[elihart]

@mradzinski thanks for the info, good to have some validation on what I observed as well. I have had the same thoughts that I doubt they plan to change their implementation - they would need to take a completely different approach I believe.

@gajicm93 I agree, all very good points.

I think there are two broad cases:

Small lists: Epoxy vs DiffUtil speed is roughly the same. Shouldn't matter too much what we use. DiffUtil is more memory efficient and a better choice for maintainability

Large lists: Epoxy is faster, but runs on Main thread so we risk skipping frames, so this should probably run on the background. If it is run on background we may as well use DiffUtil anyway even though it is slower

I also have concerns that Epoxy's diffing uses too much memory on large lists, which may impact lower speed devices more.

So I'm leaning towards switching to DiffUtil and supporting background diffing. I labored over Epoxy's diffing, so it's a bit painful for me to do, but it seems like the right thing.

Would appreciate any other concerns or perspectives.

[elihart]

This is done with #425

I also added support for async model building. This will be merged and released soon!