Calculate average item size #296

naqvitalha · 2022-04-06T19:18:06Z

Description

resolves #297

FlashList will compute average sizes from onLayout callbacks. I've introduced an average window so that the list reacts quickly to size changes and we can avoid worrying about cleaning up average based on prop changes. This is needed because average for a large number of items can become very rigid. Swapping out total count for a smaller number was another way to achieve the same thing but deciding where to do it was a problem.

I'm not changing estimatedItemSize requirement as part of this. This is a behind the scene optimization for v0.5 milestone. I'd want to discuss and make the prop optional as part of OSS milestone.

Reviewers’ hat-rack 🎩

Open GridLayoutProviderWithProps.ts file
Log averageItemSize at the end of reportItemLayout method and see it updating in realtime

Check if the values look correct and constantly update with changes.

Checklist

I have added a changelog entry following the Keep a Changelog guidelines.

src/__tests__/FlashList.test.tsx

src/__tests__/GridLayoutProviderWithProps.test.ts

naqvitalha · 2022-04-06T20:05:07Z

I'll walk you folks through the changes in our next sync up.

fortmarek

Thanks for making all those tests 👏 I have a couple of smaller things but mostly looks good 💯

documentation/docs/fundamentals/usage.md

src/GridLayoutProviderWithProps.ts

fortmarek · 2022-04-07T13:17:50Z

src/utils/AverageWindow.ts

+    this.currentCount = newCount;
+  }
+
+  protected getStoredValues() {


could be a getter method if we rename the underlying variable to have underscore.

fortmarek · 2022-04-07T13:18:50Z

src/__tests__/AverageWindow.test.ts

+import { AverageWindow } from "../utils/AverageWindow";
+
+class AverageWindowTest extends AverageWindow {
+  public get storedValues(): (number | undefined)[] {


I don't really see why this is necessary? As commented in AverageWindow, let's make this method part of AverageWindow itself and remove getStoredValues

Point was to make sure storedValues isn't a public API. I don't want people to ever have access unless it's for tests. I just saw this issue in TS: microsoft/TypeScript#19335
It seems like string accessor is an escape hatch for testing :) I'll try and use that

fortmarek · 2022-04-07T13:20:45Z

src/__tests__/AverageWindow.test.ts

+    const reduceObj = inputValues.reduce(
+      (obj, val) => {
+        if (val !== undefined) {
+          obj.sum += val;
+          obj.count++;
+        }
+        return obj;
+      },
+      { sum: 0, count: 0 }
+    );


can't we compute the value outside the test and then use absolute value? I don't really like adding non-trivial logic to a test just for the sake of using it as an assert.

This logic only belongs to this test and totally irrelevant outside. We wouldn't want this code to be in average window. This is verifying if running average is same as what we'd get from explicit calculation.

I understand but I still think it would be better for the assert to rather work with hard values instead of computing them on the go. But it's not a big deal, so we can keep it.

src/__tests__/FlashList.test.tsx

Co-authored-by: Marek Fořt <marek.fort@shopify.com>

into automaticAverage

src/__tests__/helpers/mountFlashList.tsx

naqvitalha · 2022-04-07T19:58:24Z

@fortmarek I've addressed the comments. Please check again when you have time.

fortmarek

There are some discussions but most of them low prio, so nothing holding back to merge this from my side.

ElviraBurchik · 2022-04-08T09:51:29Z

src/utils/AverageWindow.ts

+  /**
+   * Can be used to get the current average value
+   */
+  public get currentValue(): number {


+1 to Marek, I'd rather have a longer, but more obvious meaning.
averageWindow.current doesn't mean currentAverage imho, it could be an instance of averageWindow for example

ElviraBurchik · 2022-04-08T10:41:22Z

src/utils/AverageWindow.ts

+    const target = this.getNextIndex();
+    const oldValue = this.inputValues[target];
+    const newCount =
+      oldValue === undefined ? this.currentCount + 1 : this.currentCount;
+
+    this.inputValues[target] = value;
+
+    this.currentAverage =
+      this.currentAverage * (this.currentCount / newCount) +
+      (value - (oldValue ?? 0)) / newCount;
+
+    this.currentCount = newCount;


Suggested change

const target = this.getNextIndex();

const oldValue = this.inputValues[target];

const newCount =

oldValue === undefined ? this.currentCount + 1 : this.currentCount;

this.inputValues[target] = value;

this.currentAverage =

this.currentAverage * (this.currentCount / newCount) +

(value - (oldValue ?? 0)) / newCount;

this.currentCount = newCount;

// The index of the next value to be added

const targetIndex = this.getNextIndex();

// Current value at the target index. can be undefined if we are adding the first value

const oldValue = this.inputValues[targetIndex];

// If no value was previously at the target index, we need to increase the number of items

const newCount =

oldValue === undefined ? this.numberOfItems + 1 : this.numberOfItems;

// Set the new value at the target index

this.inputValues[targetIndex] = value;

const targetValueDiff = value - (oldValue ?? 0);

const newAverage =

(this.currentAverage * this.numberOfItems + targetValueDiff) / newCount;

this.currentAverage = newAverage;

this.numberOfItems = newCount;

In running average it's better to divide number of items with new count before multiplying. This way the output of the multiplication can never overshoot number's max size.

e.g, currentAverage * numberOfItems can be very big if numbers you're averaging are large and can go over what the variable can handle.

ElviraBurchik · 2022-04-08T10:46:15Z

src/GridLayoutProviderWithProps.ts

  public newLayoutManager(
    renderWindowSize: Dimension,
    isHorizontal?: boolean,
    cachedLayouts?: Layout[]
  ): LayoutManager {
+    // Average window is updated whenever a new layout manager is created. This is because old values are not relevant anymore.
+    const estimatedItemCount = Math.max(


What does 3 mean here?

ElviraBurchik · 2022-04-08T10:47:22Z

src/FlashList.tsx

@@ -698,6 +705,12 @@ class FlashList<T> extends React.PureComponent<
    return currentOffset >= this.distanceFromWindow;
  }

+  private onItemLayout = (index: number) => {
+    // Informing the layout provider about change to an item's layout. It already knows the dimensions so there's not need to pass them.


Suggested change

// Informing the layout provider about change to an item's layout. It already knows the dimensions so there's not need to pass them.

// Informing the layout provider about change to an item's layout. It already knows the dimensions so there's no need to pass them.

ElviraBurchik · 2022-04-08T10:57:43Z

src/utils/AverageWindow.ts

+   *
+   * @param value Add new value to the average window and updated current average
+   */
+  public addValue(value: number): void {


+1, I first thought it's related to UI window. However, reference to Sliding Window is helpful to understand the meaning of what we do here. AverageSlidingWindow? AverageValueTracker is good too.

naqvitalha added 10 commits April 4, 2022 14:34

Added average utility

2449358

updated algorithm and tests

2e64d59

working implementation

cfa5941

lint fixes

05a5990

Added tests

c8510c0

Fixing warnings

b5acda9

updated documentation for overridelayout

f7cd023

Treating 0 as a valid column span

f3a9dbc

Added more comments

2eba2e0

Merge branch 'main' into automaticAverage

c391d66

naqvitalha commented Apr 6, 2022

View reviewed changes

src/__tests__/FlashList.test.tsx Outdated Show resolved Hide resolved

naqvitalha commented Apr 6, 2022

View reviewed changes

src/__tests__/FlashList.test.tsx Show resolved Hide resolved

naqvitalha commented Apr 6, 2022

View reviewed changes

src/__tests__/GridLayoutProviderWithProps.test.ts Outdated Show resolved Hide resolved

naqvitalha added 2 commits April 6, 2022 12:44

Lint fix

886a149

update changelog

6113f93

naqvitalha requested review from fortmarek, ElviraBurchik and davebcn87 April 6, 2022 19:55

naqvitalha commented Apr 6, 2022

View reviewed changes

src/__tests__/GridLayoutProviderWithProps.test.ts Show resolved Hide resolved

Added min value for average window

8c31a3f

fortmarek reviewed Apr 7, 2022

View reviewed changes

naqvitalha and others added 7 commits April 7, 2022 10:35

Update documentation/docs/fundamentals/usage.md

125db3f

Co-authored-by: Marek Fořt <marek.fort@shopify.com>

Update documentation/docs/fundamentals/usage.md

4983e2a

Co-authored-by: Marek Fořt <marek.fort@shopify.com>

Update src/utils/AverageWindow.ts

1aa6cb3

Co-authored-by: Marek Fořt <marek.fort@shopify.com>

review fixes

8837840

Merge branch 'automaticAverage' of https://github.com/Shopify/flash-list

604ca42

into automaticAverage

Fixing lint

caf4fb2

review comments

df0cad1

naqvitalha commented Apr 7, 2022

View reviewed changes

src/__tests__/helpers/mountFlashList.tsx Show resolved Hide resolved

bug fix for cases where size match the estimate exactly

fa12dca

fortmarek approved these changes Apr 8, 2022

View reviewed changes

ElviraBurchik reviewed Apr 8, 2022

View reviewed changes

naqvitalha added 2 commits April 8, 2022 08:37

review suggestion

a84cb5b

fix typo

545cb89

naqvitalha merged commit 7e16f0d into main Apr 8, 2022

naqvitalha deleted the automaticAverage branch April 8, 2022 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate average item size #296

Calculate average item size #296

naqvitalha commented Apr 6, 2022 •

edited

naqvitalha commented Apr 6, 2022

fortmarek left a comment

fortmarek Apr 7, 2022

fortmarek Apr 7, 2022

naqvitalha Apr 7, 2022

fortmarek Apr 7, 2022

naqvitalha Apr 7, 2022 •

edited

fortmarek Apr 8, 2022 •

edited

naqvitalha commented Apr 7, 2022

fortmarek left a comment

ElviraBurchik Apr 8, 2022

ElviraBurchik Apr 8, 2022

naqvitalha Apr 8, 2022

ElviraBurchik Apr 8, 2022

ElviraBurchik Apr 8, 2022

ElviraBurchik Apr 8, 2022

-    const target = this.getNextIndex();
-    const oldValue = this.inputValues[target];
-    const newCount =
-      oldValue === undefined ? this.currentCount + 1 : this.currentCount;
-    this.inputValues[target] = value;
-    this.currentAverage =
-      this.currentAverage * (this.currentCount / newCount) +
-      (value - (oldValue ?? 0)) / newCount;
-    this.currentCount = newCount;
+// The index of the next value to be added
+    const targetIndex = this.getNextIndex();
+    // Current value at the target index. can be undefined if we are adding the first value
+    const oldValue = this.inputValues[targetIndex];
+    // If no value was previously at the target index, we need to increase the number of items
+    const newCount =
+      oldValue === undefined ? this.numberOfItems + 1 : this.numberOfItems;
+    // Set the new value at the target index
+    this.inputValues[targetIndex] = value;
+    const targetValueDiff = value - (oldValue ?? 0);
+    const newAverage =
+      (this.currentAverage * this.numberOfItems + targetValueDiff) / newCount;
+    this.currentAverage = newAverage;
+    this.numberOfItems = newCount;

	// Informing the layout provider about change to an item's layout. It already knows the dimensions so there's not need to pass them.
	// Informing the layout provider about change to an item's layout. It already knows the dimensions so there's no need to pass them.

Calculate average item size #296

Calculate average item size #296

Conversation

naqvitalha commented Apr 6, 2022 • edited

Description

Reviewers’ hat-rack 🎩

Checklist

naqvitalha commented Apr 6, 2022

fortmarek 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

naqvitalha Apr 7, 2022 • edited

Choose a reason for hiding this comment

fortmarek Apr 8, 2022 • edited

Choose a reason for hiding this comment

naqvitalha commented Apr 7, 2022

fortmarek 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

naqvitalha commented Apr 6, 2022 •

edited

naqvitalha Apr 7, 2022 •

edited

fortmarek Apr 8, 2022 •

edited