Reduce viewport updates in `LazyList` for improved perf with Zipline #1471

veyndan · 2023-09-11T15:02:53Z

Closes #1256.

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt

JakeWharton · 2023-09-12T12:53:05Z

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt

+    // the serialization cost between the host-guest bridge can be expensive.
+    val viewportItemCount = lastVisibleItemIndex - firstVisibleItemIndex + 1
+    val firstPagedItemIndex = (((firstVisibleItemIndex / viewportItemCount) * viewportItemCount) - viewportItemCount).coerceAtLeast(0)
+    val lastPagedItemIndex = ((lastVisibleItemIndex / viewportItemCount) * viewportItemCount) + viewportItemCount * 2


Does this need a coerceAtMost of the list size? Or do we not know the upper bound?

We know the upper bound that the LazyList is aware of, though that upper bound can dynamically increase (e.g., in the case of Paging). I don't think we necessarily need coerceAtMost (or even coerceAtLeast), but it'll marginally reduce the number of unnecessary onViewportChanged lambda invocations for small lists.

swankjesse · 2023-09-12T15:26:34Z

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt

+    // particularly important when using `LazyList` in Treehouse (compared to plain Redwood), as
+    // the serialization cost between the host-guest bridge can be expensive.
+    val viewportItemCount = lastVisibleItemIndex - firstVisibleItemIndex + 1
+    val firstPagedItemIndex = (((firstVisibleItemIndex / viewportItemCount) * viewportItemCount) - viewportItemCount).coerceAtLeast(0)


Why subtract a page?

Suggested change

val firstPagedItemIndex = (((firstVisibleItemIndex / viewportItemCount) * viewportItemCount) - viewportItemCount).coerceAtLeast(0)

val firstPagedItemIndex = (firstVisibleItemIndex / viewportItemCount) * viewportItemCount

swankjesse · 2023-09-12T15:27:17Z

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt

+    // the serialization cost between the host-guest bridge can be expensive.
+    val viewportItemCount = lastVisibleItemIndex - firstVisibleItemIndex + 1
+    val firstPagedItemIndex = (((firstVisibleItemIndex / viewportItemCount) * viewportItemCount) - viewportItemCount).coerceAtLeast(0)
+    val lastPagedItemIndex = (((lastVisibleItemIndex / viewportItemCount) * viewportItemCount) + viewportItemCount * 2).coerceAtMost(items.items.size)


To round up, do this?

Suggested change

val lastPagedItemIndex = (((lastVisibleItemIndex / viewportItemCount) * viewportItemCount) + viewportItemCount * 2).coerceAtMost(items.items.size)

val lastPagedItemIndex = (lastVisibleItemIndex + viewportItemCount - 1) / viewportItemCount * viewportItemCount

...yout-widget/src/commonTest/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyListTest.kt

swankjesse · 2023-09-12T15:28:36Z

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt

+    // Paginate the results so `onViewportChanged` isn't blasted with a bunch of updates. This is
+    // particularly important when using `LazyList` in Treehouse (compared to plain Redwood), as
+    // the serialization cost between the host-guest bridge can be expensive.
+    val viewportItemCount = lastVisibleItemIndex - firstVisibleItemIndex + 1


Would you consider coerceAtLeast() instead of + 1 to make sure it's nonzero?

Replacing + 1 with coerceAtLeast() would mean that we are no longer calculating the number of items in the viewport. The + 1 is to ensure that that is what we're calculating, and the fact that it produces a nonzero is happy happenstance.

What does the + 1 do? Inclusive to exclusive?

I'm kinda confused by what you're asking, as we're not dealing with a range here. As Jake pointed (#1471 (comment)), we need to do a + 1 to ensure we're calculating the number of items in the viewport.

...yout-widget/src/commonTest/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyListTest.kt

swankjesse

I wanna nail down exactly what the calls to onViewportChanged() should be, so the guest code has the maximum amount of control over preloading...

I assume that the bounds on the viewport are inclusive, and the two arguments to onViewportChanged() are both inclusive. To make the math easier I’m making the visible end index exclusive below.

Suppose the page size is 10. I think my preference is:

scrolls to [40..50), we call onViewportChanged(40, 49) to show 10 elements.
scrolls to [41..51), we call onViewportChanged(40, 59) to show 20 elements
scrolls to [42, 52), we do nothing
scrolls to [49, 59), we do nothing
scrolls to [50, 60), we call onViewportChanged(50, 59) to show 10 elements

Alternately we could subtract 1 off at the end to adjust back to inclusive/inclusive?

veyndan · 2023-09-12T16:16:14Z

I just updated the tests to showcase what the current solution does. In the current case, the "page size" is identical to the "viewport item count". Therefore, we request a page above and below what where we're currently at. With your proposed solution, hard coding a page size of 10 would be okay in most cases, but if you have very large items (think Instagram posts, where you can only fit one full post on a mobile screen), a page size of 10 is much too large.

swankjesse · 2023-09-12T17:15:18Z

Sorry, I didn’t mean to imply a hardcoded page size. I just used examples where the page size as always 10.

JakeWharton · 2023-09-12T17:17:30Z

I would not let the viewport get within 1 row of needing placeholders. I would think at minimum we should probably keep at least half a viewport's amount of row's offscreen in either direction.

swankjesse · 2023-09-12T17:17:51Z

...yout-widget/src/commonTest/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyListTest.kt

+    windowedLazyList.updateViewport(49, 58)
+    windowedLazyList.updateViewport(59, 68)
+    windowedLazyList.viewportChanges.test {
+      assertThat(awaitItem()).isEqualTo(30..70)


Expanding it by one page on either side feels like you’re doing the guest code’s work on the host side. Would you consider changing these to 40..60 and 50..70 ?

Are these inclusive or exclusive? 40..60 seems like it’s 10 elements too large for 41..50 as input

veyndan · 2023-09-13T09:37:16Z

...ood-lazylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyList.kt

      val proposedPlaceholderPoolSize = visibleItemCount + visibleItemCount / 2
      // We only ever want to increase the pool size.
      if (placeholderPoolSize < proposedPlaceholderPoolSize) {
        placeholderPoolSize = proposedPlaceholderPoolSize
      }
-      state.firstVisibleItemIndex = localFirstVisibleItemIndex
-      lastVisibleItemIndex = localLastVisibleItemIndex
+      state.firstVisibleItemIndex = localFirstPagedItemIndex


@swankjesse @JakeWharton I didn't notice it before, but such a paginated viewport introduces a bug here. This PR means we can no longer reliably expose to the caller of LazyList what the first visible item index is.

Can we add a third parameter to the lambda for the actual first index?

If we did that and didn't update the frequency of the lambda invocation on every scroll, then the first visible item index would still be incorrect due to the new deduplication logic. If the new deduplication logic took into account the actual first index, then we're serializing and deserializing the viewport updates at the same frequency as before.

veyndan · 2023-12-07T12:46:18Z

Closing because of #1471 (comment).

veyndan force-pushed the veyndan/2023-09-11/paginate-viewport-changes branch 3 times, most recently from 95f3864 to 5b29665 Compare September 11, 2023 15:09

JakeWharton reviewed Sep 11, 2023

View reviewed changes

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt Outdated Show resolved Hide resolved

JakeWharton approved these changes Sep 11, 2023

View reviewed changes

...zylayout-widget/src/commonMain/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyList.kt Outdated Show resolved Hide resolved

veyndan force-pushed the veyndan/2023-09-11/paginate-viewport-changes branch from 5b29665 to f5fe930 Compare September 12, 2023 12:12

JakeWharton reviewed Sep 12, 2023

View reviewed changes

veyndan force-pushed the veyndan/2023-09-11/paginate-viewport-changes branch from f5fe930 to c5eafbf Compare September 12, 2023 14:53

veyndan enabled auto-merge (squash) September 12, 2023 14:56

veyndan force-pushed the veyndan/2023-09-11/paginate-viewport-changes branch from c5eafbf to 7f230a4 Compare September 12, 2023 15:24

swankjesse reviewed Sep 12, 2023

View reviewed changes

...yout-widget/src/commonTest/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyListTest.kt Outdated Show resolved Hide resolved

swankjesse reviewed Sep 12, 2023

View reviewed changes

...yout-widget/src/commonTest/kotlin/app/cash/redwood/lazylayout/widget/WindowedLazyListTest.kt Outdated Show resolved Hide resolved

veyndan disabled auto-merge September 12, 2023 15:29

swankjesse reviewed Sep 12, 2023

View reviewed changes

veyndan force-pushed the veyndan/2023-09-11/paginate-viewport-changes branch from 7f230a4 to fb06b95 Compare September 12, 2023 16:11

Reduce viewport updates in LazyList for improved perf with Zipline

028306a

veyndan force-pushed the veyndan/2023-09-11/paginate-viewport-changes branch from fb06b95 to 028306a Compare September 12, 2023 16:29

veyndan enabled auto-merge (squash) September 12, 2023 16:32

swankjesse reviewed Sep 12, 2023

View reviewed changes

veyndan disabled auto-merge September 12, 2023 18:19

veyndan commented Sep 13, 2023

View reviewed changes

veyndan closed this Dec 7, 2023

veyndan deleted the veyndan/2023-09-11/paginate-viewport-changes branch December 7, 2023 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce viewport updates in `LazyList` for improved perf with Zipline #1471

Reduce viewport updates in `LazyList` for improved perf with Zipline #1471

veyndan commented Sep 11, 2023 •

edited

Loading

JakeWharton Sep 12, 2023

veyndan Sep 12, 2023 •

edited

Loading

swankjesse Sep 12, 2023

swankjesse Sep 12, 2023

swankjesse Sep 12, 2023

veyndan Sep 12, 2023

swankjesse Sep 12, 2023

veyndan Sep 13, 2023

swankjesse left a comment

veyndan commented Sep 12, 2023

swankjesse commented Sep 12, 2023

JakeWharton commented Sep 12, 2023

swankjesse Sep 12, 2023

veyndan Sep 13, 2023

JakeWharton Sep 13, 2023

veyndan Sep 13, 2023 •

edited

Loading

veyndan commented Dec 7, 2023

	val firstPagedItemIndex = (((firstVisibleItemIndex / viewportItemCount) * viewportItemCount) - viewportItemCount).coerceAtLeast(0)
	val firstPagedItemIndex = (firstVisibleItemIndex / viewportItemCount) * viewportItemCount

	val lastPagedItemIndex = (((lastVisibleItemIndex / viewportItemCount) * viewportItemCount) + viewportItemCount * 2).coerceAtMost(items.items.size)
	val lastPagedItemIndex = (lastVisibleItemIndex + viewportItemCount - 1) / viewportItemCount * viewportItemCount

Reduce viewport updates in LazyList for improved perf with Zipline #1471

Reduce viewport updates in LazyList for improved perf with Zipline #1471

Conversation

veyndan commented Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

veyndan Sep 12, 2023 • edited Loading

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

swankjesse left a comment

Choose a reason for hiding this comment

veyndan commented Sep 12, 2023

swankjesse commented Sep 12, 2023

JakeWharton commented Sep 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veyndan Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

veyndan commented Dec 7, 2023

Reduce viewport updates in `LazyList` for improved perf with Zipline #1471

Reduce viewport updates in `LazyList` for improved perf with Zipline #1471

veyndan commented Sep 11, 2023 •

edited

Loading

veyndan Sep 12, 2023 •

edited

Loading

veyndan Sep 13, 2023 •

edited

Loading