Page MenuHomePhabricator

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

Authored by atul on Sep 14 2023, 2:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, May 2, 2:29 PM
Unknown Object (File)
Thu, May 2, 6:34 AM
Unknown Object (File)
Tue, Apr 30, 12:09 AM
Unknown Object (File)
Sun, Apr 28, 1:31 PM
Unknown Object (File)
Sun, Apr 28, 1:31 PM
Unknown Object (File)
Sun, Apr 28, 1:30 PM
Unknown Object (File)
Sun, Apr 28, 1:10 PM
Unknown Object (File)
Sun, Apr 28, 12:16 AM
Subscribers

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 published this revision for review.Sep 14 2023, 2:22 PM
tomek requested changes to this revision.Sep 18 2023, 6:08 AM

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
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.