Remove sync layout optimization in RecyclerBinder

Summary: This optimization will be handled by mihaelao's experiment with posting calculating the range until after onBindViewHolder (which will compute layout sync for exactly the rows that need it). That experiment controls `ComponentsConfiguration.insertPostAsyncLayout` Reviewed By: mihaelao Differential Revision: D7041994 fbshipit-source-id: 1fbea9fe9db60b4671a02a37c6036864e0eac910
chowjee · Feb 21, 2018 · 53fed0b · 53fed0b
1 parent 73f040d
commit 53fed0b
Show file tree

Hide file tree

Showing 2 changed files with 16 additions and 43 deletions.
diff --git a/litho-it/src/test/java/com/facebook/litho/widget/RecyclerBinderTest.java b/litho-it/src/test/java/com/facebook/litho/widget/RecyclerBinderTest.java
@@ -468,11 +468,18 @@ public void testAddItemsAfterMeasuring() {
 
     int rangeTotal = RANGE_SIZE + (int) (RANGE_SIZE * RANGE_RATIO);
 
-    for (int i = 0; i < RANGE_SIZE; i++) {
+    // The first component is used to calculate the range
+    TestComponentTreeHolder firstHolder =
+        mHoldersForComponents.get(components.get(0).getComponent());
+    assertThat(firstHolder.isTreeValid()).isTrue();
+    assertThat(firstHolder.mLayoutSyncCalled).isTrue();
+
+    for (int i = 1; i < RANGE_SIZE; i++) {
       componentTreeHolder = mHoldersForComponents.get(components.get(i).getComponent());
 
       assertThat(componentTreeHolder.isTreeValid()).isTrue();
-      assertThat(componentTreeHolder.mLayoutSyncCalled).isTrue();
+      assertThat(componentTreeHolder.mLayoutAsyncCalled).isTrue();
+      assertThat(componentTreeHolder.mLayoutSyncCalled).isFalse();
     }
 
     for (int i = RANGE_SIZE; i <= rangeTotal; i++) {
@@ -773,8 +780,8 @@ public void testMoveItemInsideVisibleRange() {
         mHoldersForComponents.get(components.get(99).getComponent());
 
     assertThat(movedHolder.isTreeValid()).isTrue();
-    assertThat(movedHolder.mLayoutAsyncCalled).isFalse();
-    assertThat(movedHolder.mLayoutSyncCalled).isTrue();
+    assertThat(movedHolder.mLayoutAsyncCalled).isTrue();
+    assertThat(movedHolder.mLayoutSyncCalled).isFalse();
     assertThat(movedHolder.mDidAcquireStateHandler).isFalse();
     final int rangeTotal = (int) (RANGE_SIZE + (RANGE_RATIO * RANGE_SIZE));
 
@@ -850,8 +857,8 @@ public void testInsertInVisibleRange() {
         mHoldersForComponents.get(newRenderInfo.getComponent());
 
     assertThat(holder.isTreeValid()).isTrue();
-    assertThat(holder.mLayoutSyncCalled).isTrue();
-    assertThat(holder.mLayoutAsyncCalled).isFalse();
+    assertThat(holder.mLayoutSyncCalled).isFalse();
+    assertThat(holder.mLayoutAsyncCalled).isTrue();
     assertThat(holder.mDidAcquireStateHandler).isFalse();
 
     final int rangeTotal = (int) (RANGE_SIZE + (RANGE_RATIO * RANGE_SIZE));

diff --git a/litho-widget/src/main/java/com/facebook/litho/widget/RecyclerBinder.java b/litho-widget/src/main/java/com/facebook/litho/widget/RecyclerBinder.java
@@ -492,7 +492,6 @@ public final void insertItemAt(int position, RenderInfo renderInfo) {
     assertNoInsertOperationIfCircular();
 
     final ComponentTreeHolder holder = createComponentTreeHolder(renderInfo);
-    final boolean computeLayout;
     final int childrenWidthSpec, childrenHeightSpec;
     synchronized (this) {
       mComponentTreeHolders.add(position, holder);
@@ -511,24 +510,12 @@ public final void insertItemAt(int position, RenderInfo renderInfo) {
               childrenWidthSpec,
               childrenHeightSpec,
               mLayoutInfo.getScrollDirection());
-
-          computeLayout = false;
         } else if (mRequiresRemeasure.get()) {
           requestRemeasure();
-          computeLayout = false;
-        } else {
-          final int firstVisiblePosition = Math.max(mCurrentFirstVisiblePosition, 0);
-          computeLayout = position >= firstVisiblePosition &&
-              position < firstVisiblePosition + mRange.estimatedViewportCount;
         }
-      } else {
-        computeLayout = false;
       }
     }
 
-    if (computeLayout) {
-      holder.computeLayoutSync(mComponentContext, childrenWidthSpec, childrenHeightSpec, null);
-    }
     mInternalAdapter.notifyItemInserted(position);
 
     maybePostComputeRange();
@@ -614,12 +601,8 @@ public final void updateItemAt(int position, RenderInfo renderInfo) {
 
     final ComponentTreeHolder holder;
     final boolean renderInfoWasView;
-    final boolean shouldComputeLayout;
-    final int childrenWidthSpec, childrenHeightSpec;
     synchronized (this) {
       holder = mComponentTreeHolders.get(position);
-      shouldComputeLayout = mRange != null && position >= mCurrentFirstVisiblePosition &&
-          position < mCurrentFirstVisiblePosition + mRange.estimatedViewportCount;
       renderInfoWasView = holder.getRenderInfo().rendersView();
 
       mRenderInfoViewCreatorController.maybeTrackViewCreator(renderInfo);
@@ -636,21 +619,14 @@ public final void updateItemAt(int position, RenderInfo renderInfo) {
             getActualChildrenHeightSpec(holder),
             mLayoutInfo.getScrollDirection());
       }
-
-      childrenWidthSpec = getActualChildrenWidthSpec(holder);
-      childrenHeightSpec = getActualChildrenHeightSpec(holder);
     }
 
     // If this item is rendered with a view (or was rendered with a view before now) we need to
     // notify the RecyclerView's adapter that something changed.
-    final boolean doNotifyItemChanged = renderInfoWasView || renderInfo.rendersView();
-    if (doNotifyItemChanged) {
-      if (shouldComputeLayout) {
-        holder.computeLayoutSync(mComponentContext, childrenWidthSpec, childrenHeightSpec, null);
-      }
-
+    if (renderInfoWasView || renderInfo.rendersView()) {
       mInternalAdapter.notifyItemChanged(position);
     }
+
     computeRange(mCurrentFirstVisiblePosition, mCurrentLastVisiblePosition);
     mViewportManager.setDataChangedIsVisible(mViewportManager.isUpdateInVisibleRange(position, 1));
   }
@@ -708,8 +684,7 @@ public final void moveItem(int fromPosition, int toPosition) {
     ThreadUtils.assertMainThread();
 
     final ComponentTreeHolder holder;
-    final boolean isNewPositionInRange, isNewPositionInVisibleRange;
-    final int childrenWidthSpec, childrenHeightSpec;
+    final boolean isNewPositionInRange;
     final int mRangeSize = mRange != null ? mRange.estimatedViewportCount : -1;
     synchronized (this) {
       holder = mComponentTreeHolders.remove(fromPosition);
@@ -718,20 +693,11 @@ public final void moveItem(int fromPosition, int toPosition) {
       isNewPositionInRange = mRangeSize > 0 &&
           toPosition >= mCurrentFirstVisiblePosition - (mRangeSize * mRangeRatio) &&
           toPosition <= mCurrentFirstVisiblePosition + mRangeSize + (mRangeSize * mRangeRatio);
-
-      isNewPositionInVisibleRange = mRangeSize > 0 &&
-          toPosition >= mCurrentFirstVisiblePosition &&
-          toPosition <= mCurrentFirstVisiblePosition + mRangeSize;
-
-      childrenWidthSpec = getActualChildrenWidthSpec(holder);
-      childrenHeightSpec = getActualChildrenHeightSpec(holder);
     }
     final boolean isTreeValid = holder.isTreeValid();
 
     if (isTreeValid && !isNewPositionInRange) {
       holder.acquireStateHandlerAndReleaseTree();
-    } else if (isNewPositionInVisibleRange && !isTreeValid) {
-      holder.computeLayoutSync(mComponentContext, childrenWidthSpec, childrenHeightSpec, null);
     }
     mInternalAdapter.notifyItemMoved(fromPosition, toPosition);