[native] Memoize `flatList` in `ChatThreadList`
AbandonedPublic
Actions

Authored by atul on Sep 14 2023, 2:22 PM.

Details

Reviewers

ginsu
tomek
rohan

Summary

We want to ensure that the FlatList renders only when absolutely necessary. This helps "isolate" the FlatList from state that may trigger re-renders of eg fixedSearch. For example, we want to isolate flatList from changes to searchText which would always trigger re-render of fixedSearch which would cause the entirety of chatThreadList to re-render. (In practicce a change to searchText would probably change eg threadSearchResults` which would in turn change partialData which would trigger flatList to expectedly re-render.. but just for demonstration).

Depends on D9210

Test Plan

Search experience continues to look and work as expected. Set logs in all expected callbacks to ensure that they fired as expected. Made sure that x-icon and "Cancel" button continue to look and behave as before.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Sep 14 2023, 2:22 PM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 14 2023, 2:22 PM

atul published this revision for review.Sep 14 2023, 2:22 PM

Harbormaster completed remote builds in B22646: Diff 31157.Sep 14 2023, 2:39 PM

atul added a child revision: D9212: [native] Consume `useThreadListSearch` in `ChatThreadList`.Sep 14 2023, 3:51 PM

Have you tested if memoizing the list is beneficial? FlatList is already memoized internally (PureComponent) so wrapping it with memo shouldn't improve anything, but maybe I'm missing something.

This revision now requires changes to proceed.Sep 18 2023, 6:08 AM

In D9211#271261, @tomek wrote:

Have you tested if memoizing the list is beneficial? FlatList is already memoized internally (PureComponent) so wrapping it with memo shouldn't improve anything, but maybe I'm missing something.

Yeah, previously flatList would re-render when fixedSearch changed because it was in the chatThreadList dep list. Separating flatList out into its own useMemo seems to prevent that.

atul requested review of this revision.Sep 18 2023, 9:15 AM

rebase

Harbormaster completed remote builds in B22688: Diff 31216.Sep 18 2023, 10:49 AM

atul mentioned this in D9215: [native] Remove `boundChatListData` from `useThreadListSearch` dep list.Sep 18 2023, 1:49 PM

In D9211#271306, @atul wrote:

In D9211#271261, @tomek wrote:

Have you tested if memoizing the list is beneficial? FlatList is already memoized internally (PureComponent) so wrapping it with memo shouldn't improve anything, but maybe I'm missing something.

Yeah, previously flatList would re-render when fixedSearch changed because it was in the chatThreadList dep list. Separating flatList out into its own useMemo seems to prevent that.

It's really strange that the list rerenders because it is a PureComponent.

This revision is now accepted and ready to land.Sep 19 2023, 4:56 AM

@tomek Sorry, you’re totally right that this is useless. I tested performance with/without this diff in isolation and there was no difference.

Revision Contents
Changeset List

Path

Size

native/

chat/

chat-thread-list.react.js

52 lines

Diff 31216

View Options

native/chat/chat-thread-list.react.js

[native] Memoize `flatList` in `ChatThreadList`AbandonedPublicActions