Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize VirtualizedList #31327

Closed
wants to merge 1 commit into from

Conversation

halaei
Copy link
Contributor

@halaei halaei commented Apr 8, 2021

Summary

Using FlatList components with 50 items, and the default values for initialNumToRender and maxToRenderPerBatch, the following happens:

  • Render first 10 items (Items[0 .. 9]).
  • Sleep.
  • Render first 20 items (Items[0 .. 19]). It should be rendering items [10 .. 19] instead.
  • Sleep.
  • Render first 30 items (Items[0 .. 29]).
  • Sleep.
  • Render first 40 items (Items[0 .. 39]).
  • Sleep.
  • Render all items (Items[0..49]).

So the flat list is rendering the first 10 items 5 times. It is a performance issue when the items are complicated, or the FlatList has many items. Having N items the FlatList will render O(N^2) items over time.

This PR reduces time complexity by:

  1. Adding a cache of cells to VirtualizedList and reusing the components in consecetive calls to list.render().
  2. Making CellRenderer a React.PureComponent and re-shaping state to prevent re-renders.

Possibly related issues

Changelog

[JavaScript] [Fixed] - Don't render list items if not changed.

Test Plan

Some tests are needed to make sure that all the cached components are rendered just enough, no more/less.
Footer, header, and cell components are cached. Other child components don't need to be cached because they are either simple or rendered once.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 8, 2021
@halaei halaei mentioned this pull request Apr 8, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: a15a46c

@halaei halaei force-pushed the optimize-virtualized-list branch from 84da4e5 to cdf3dd6 Compare April 8, 2021 12:06
@efstathiosntonas
Copy link

efstathiosntonas commented Apr 13, 2021

@halaei I've applied the patch and performance seems improved, I've faced an issue with SectionList though Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.. Nothing has been touched in that SectionList it was working perfectly before the patch.

@satya164
Copy link
Contributor

It is a performance issue when the items are complicated, or the FlatList has many items.

Isn't that optimized by using React.memo/React.PureComponent for the individual items as recommended?

...
};

class CellRenderer extends React.Component<
class CellRenderer extends React.PureComponent<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component accepts props which changes every render, so I think PureComponent checks will be false every time:

  • onLayout - can be fixed
  • parentProps - can't be fixed
  • Maybe more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onLayout doesn't seem to cause extra renders in my manual tests. It's input value is given by the creator passed to _pushCell() function, which is only called when needed. So caching already is applied to onLayout too.

I think maybe for some use-cases there can be more optimization, for example not always everyone needs all the fields in parentProps, and sometimes the order and number of items in data doesn't change the internal of an item component, so maybe ii can be removed from deps too. I am interested to see other improvements done too.

@halaei
Copy link
Contributor Author

halaei commented Apr 14, 2021

Isn't that optimized by using React.memo/React.PureComponent for the individual items as recommended?

Simply wrapping the return value of renderItem() inside a React.memo or returning a React.PureComponent doesn't help:

const ItemComponent = (props) => {
    x =...
    y = ...
    z = ...
     return <ComponentDependingOn_x_y_z ...>;
}
const MemoItemComponent = React.memo(ItemComponent); // It doesn't prevent extra re-renders because props always change by each call to VirtualizedList.render().

However using memoiztion techniques inside the component prevents extra renders of the nested component ComponentDependingOn_x_y_z:

const UseMemoItemCompont = (props) =>{
    x =...
    y = ...
    z = ...
    return React.useMemo(() => {
        return <ComponentDependingOn_x_y_z ...>;
    }, [x, y, z]);
}

So I think this PR has the following values:
1.It lets developers write simpler code, because they won't need anymore to think about memoization techniques in their list components.
2. It is more efficient than using memoization in renderItem(), because it removes extra renders of parent/wrapper components in VirtualizedList and renderItem() itself.

@satya164
Copy link
Contributor

It is more efficient than using memoization in renderItem(), because it removes extra renders of parent/wrapper components in VirtualizedList and renderItem() itself.

It's not safe to memoize a React Element. It could be using some state in the component which VirtualizedList doesn't know about, which will skip re-rendering even when it should re-render.

However, if it was something like ItemComponent which accepted a React component instead of a render callback, it would be safe to memoize that.

@halaei
Copy link
Contributor Author

halaei commented Apr 14, 2021

@efstathiosntonas

I've faced an issue with SectionList though Error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.

I tested with a simple SectionList and it worked for me. Also current tests are passed. Can you give an example SectionList that fails?

@bohdan145
Copy link

Any info about the status of this request ?
Is this gonna be merged any time soon?

@halaei
Copy link
Contributor Author

halaei commented Jul 7, 2021

I am also interested to have feedback. I've just checked this PR with master branch and PR #31401 for more tests. All tests passed.

@dev-arson
Copy link

Could You check this optimization on RTL mode? In RTL We don't have 'sleep' phase, and as You write should be work the same as in LTR mode, render initialNumRender first and sleep, render second , sleep. etc.
Maybe this optimization could be used for start fixing RTL issue with flickering @halaei @NickGerleman ?

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 27, 2023
Copy link

github-actions bot commented Jan 3, 2024

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants