Async diffing and model building #425

elihart · 2018-04-23T05:19:52Z

First pass at #286

ngsilverman

LGTM!

ngsilverman · 2018-04-24T00:32:47Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/AsyncListDifferWithPayload.java

+ *
+ * Also adds support for canceling an in progress diff.
+ */
+class AsyncListDifferWithPayload<T> {


Add a @UiThread annotation to the whole class?

I actually want to figure out if we should support setting a new list from any thread. That could be nice when the models are built on a background thread. It would complicate this to synchronize things though. cc @gpeal

I think the most important thing is that it happens off of the main thread rather than on a specific executor

ngsilverman · 2018-04-24T00:35:44Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/SimpleEpoxyController.java

@@ -10,6 +10,10 @@
  private List<? extends EpoxyModel<?>> currentModels;
  private boolean insideSetModels;

+  public SimpleEpoxyController() {
+    setDebugLoggingEnabled(true);


Just curious, why make this change?

this was just for debugging purposes, I should have removed it.

thanks for the catch!

gpeal

Exciting!

gpeal · 2018-04-24T23:42:51Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/AsyncListDifferWithPayload.java

+ *
+ * Also adds support for canceling an in progress diff.
+ */
+class AsyncListDifferWithPayload<T> {


I think the most important thing is that it happens off of the main thread rather than on a specific executor

gpeal · 2018-04-24T23:43:19Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/AsyncListDifferWithPayload.java

+
+    final DiffCallback<T> wrappedCallback = new DiffCallback<>(list, newList, diffCallback);
+
+    DIFF_EXECUTOR.execute(new Runnable() {


This would be easier with coroutines :)

yeah :( not ready to add that dependency yet though

elihart · 2018-06-16T12:54:30Z

There are two things left I need to address:

Update documentation to reflect that duplicate unique ids won't crash anymore. I could build debug handling to check this case however.

Right now some people rely on posting a runnable after requesting a model build to make sure something runs after item changes are notified (such as scrolling to the top of the list after an item is inserted). This will no longer work, so will be a breaking change. I'd like to make sure I have a good alternative to present, and possibly enable this conditionally to give people time to update

appsailor · 2018-07-05T22:35:04Z

@elihart , any idea when this is getting released? Looking forward to trying it out on my upcoming android app which uses Firebase real time db. thx Aldrin

elihart · 2018-08-06T01:01:09Z

@ngsilverman @gpeal @BenSchwab This is updated to make the differ completely thread safe (it can now handle read/write from any thread), and also adds support for async model building.

This is my approach:

EpoxyController has a second constructor that takes two `Handler` instances, one for running model building and one for handling diffing. By default these use the main thread, but they can be changed to allow async work.

The default handlers can be changed by setting the static fields `EpoxyController#defaultDiffingHandler` and `EpoxyController#defaultModelBuildingHandler` which lets you force async behavior for all controllers in an app.

For example, you can set the global default to async like this
    HandlerThread handlerThread = new HandlerThread("epoxy");
    handlerThread.start();
    Handler handler = new Handler(handlerThread.getLooper());
    EpoxyController.defaultDiffingHandler = handler;
   EpoxyController.defaultModelBuildingHandler = handler;

A Handler is used because epoxy needs to be able to easily work with the main thread as the default. Also, Epoxy doesn't support parallel model building, and a Handler enforces this with it's single threaded looping behavior.

Note that if you use async model building you must guarantee that all of your data access is thread safe - Epoxy does not help you with that. This means that your `EpoxyController#buildModels` implementation is thread safe in all of it's data access.

If the same Handler is provided for model building and diffing then all of that work will proceed synchronously.

I feel pretty good about these changes, but it definitely adds a fair amount of complexity that is hard to test. I played around with it in the sample app and it seems good, but we'll want to test this carefully in airbnb too.

I took great care to make sure we could release this without changing user's model building behavior at all. Default behavior is still posted, but then runs synchronously until adapter changes are notified. This isn't too trivial, because by default the differ posts it's work to whatever thread it is given. I set up a wrapper class to execute Runnables asynchronously if its looper is the same. This is another reason I need to use Handlers instead of an Executor.

I also added a new listener that can be registered for when model build finishes.

Please review the concurrency logic carefully and lmk if you see room for improvement.

elihart · 2018-08-06T01:01:47Z

@appsailor sorry, I just saw your comment. The work is now done, and I'll be releasing this week.

gpeal

@elihart looks great overall. It's tough to fully grok the whole thing but overall, it looks good :) Just a couple of questions.

gpeal · 2018-08-06T15:49:20Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/AsyncEpoxyDiffer.java

+   * Marks the generation as done, and updates the list if the generation is the most recent.
+   * Calls to this MUST be synchronized on "this" so list object and generation always stay in sync.
+   * This method isn't synchronized directly so callers have flexibility to includes other
+   * actions in their synchronized block


Java uses reentrant locks for synchronized blocks. If you synchronize the method and call it from within another synchronized block, that should be fine.

That makes a lot of sense, I should have looked that up :P

gpeal · 2018-08-06T15:50:31Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/AsyncEpoxyDiffer.java

+   * @return True if the given generation is the most recent, in which case the given list was
+   * set. False if the generation is old and the list was ignored.
+   */
+  private boolean tryLatchList(@Nullable List<? extends EpoxyModel<?>> newList, int runGeneration) {


Is this @WorkerThread?

nope, anythread. annotated

gpeal · 2018-08-06T16:48:12Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/AsyncEpoxyDiffer.java

+
+    if (newList == previousList) {
+      // nothing to do
+      onRunCompleted(runGeneration, newList, new DiffResult(previousList, newList, null));


Maybe you can create static functions for these like:
DiffResult.noop(), DiffResult.clear(), and DiffResult.insert()

gpeal · 2018-08-06T16:48:42Z

epoxy-adapter/src/main/java/com/airbnb/epoxy/DiffResult.java

+  @Nullable final DiffUtil.DiffResult differResult;
+
+  @SuppressWarnings("unchecked")
+  public DiffResult(


Can you add a javadoc for what null means?

ngsilverman approved these changes Apr 24, 2018

View reviewed changes

gpeal approved these changes Apr 24, 2018

View reviewed changes

elihart mentioned this pull request May 29, 2018

Reduce memory #433

Merged

elihart added 4 commits July 29, 2018 21:31

Async Diffing

55aae10

start work on model building async

9eb7c88

async build models mostly working

24eea6d

synchronized differ

c915cc7

elihart mentioned this pull request Aug 6, 2018

Diff on background thread for EpoxyController #286

Closed

elihart force-pushed the eli-async_diffing branch from ad2493c to 196bd54 Compare August 6, 2018 00:37

fix tests

05ede30

elihart force-pushed the eli-async_diffing branch from 196bd54 to 05ede30 Compare August 6, 2018 00:44

add support for listening to model build events

29167c5

elihart mentioned this pull request Aug 6, 2018

Wish: The EpoxyController has the power to scroll to particular position #471

Closed

gpeal reviewed Aug 6, 2018

View reviewed changes

clean up synchronization and address gabes comments

f598443

elihart changed the title ~~Async diffing~~ Async diffing and model building Aug 7, 2018

elihart merged commit 5c51480 into master Aug 7, 2018

elihart deleted the eli-async_diffing branch August 7, 2018 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async diffing and model building #425

Async diffing and model building #425

elihart commented Apr 23, 2018 •

edited

Loading

ngsilverman left a comment

ngsilverman Apr 24, 2018

elihart Apr 24, 2018

gpeal Apr 24, 2018

ngsilverman Apr 24, 2018

elihart Apr 24, 2018

gpeal left a comment

gpeal Apr 24, 2018

gpeal Apr 24, 2018

elihart Apr 29, 2018

elihart commented Jun 16, 2018

appsailor commented Jul 5, 2018

elihart commented Aug 6, 2018 •

edited

Loading

elihart commented Aug 6, 2018

gpeal left a comment

gpeal Aug 6, 2018

elihart Aug 6, 2018

gpeal Aug 6, 2018

elihart Aug 6, 2018

gpeal Aug 6, 2018

elihart Aug 6, 2018

gpeal Aug 6, 2018


		final DiffCallback<T> wrappedCallback = new DiffCallback<>(list, newList, diffCallback);

		DIFF_EXECUTOR.execute(new Runnable() {

Async diffing and model building #425

Async diffing and model building #425

Conversation

elihart commented Apr 23, 2018 • edited Loading

ngsilverman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elihart commented Jun 16, 2018

appsailor commented Jul 5, 2018

elihart commented Aug 6, 2018 • edited Loading

elihart commented Aug 6, 2018

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elihart commented Apr 23, 2018 •

edited

Loading

elihart commented Aug 6, 2018 •

edited

Loading