VirtualizedList CellRenderer optimization

[christianbach]

Fixes

Fixes #20174 based of @jasekiw pull request #20208

Changes

• Remove parentProps from CellRenderer and pass as ordinary props
• CellRenderer is now a PureComponent

Test Plan:

Tested on a real device with why-did-you-update running to view when the CellRendere component re-rendered. Applied updates to the list items to make sure they still get updated.

Added a test that makes sure the initial onLayout is still called properly.

Release Notes:

[GENERAL] [ENHANCEMENT][VirtualList] - Prevent unnecessary re-renders of the CellRenderer Component.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[analysis-bot]

eslint found some issues. You may run yarn prettier or npm run prettier to fix these.

[analysis-bot]

dot-notation: ["i4"] is better written in dot notation.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[christianbach]

@hramos I have tried to ping @sahrens on this PR and the PR #20208 he's probably just super busy like all of us. But perhaps you know someone else thats suited for reviewing PR related to VirtualizedList?

[hramos]

Thanks for adding tests and @matthargett for approving this. I'll import this and put it on the team's review queue, as an additional precaution.

[facebook-github-bot]

hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[sahrens]

Sorry for the delay - thanks for making this change.

Did you test to make sure that sticky headers in RNTester still work?

[christianbach]

No unfortunately I didn’t. I’ll check and report the results. But I checked all the list files for additional props and didn’t find any other references. Does the sticky headers implemention need CellRenderer to re-render?

[hramos]

When this patch is applied, I get a RedScreen on RNTester, specifically on VirtualizedList, VirtualizedSectionList.js:201: Invariant Violation: Tried to get frame for out of range index NaN

[christianbach]

Thanks @hramos I just booted up RNTester my self and got the same error. My bad for only testning this on FlatList and SectionLists without Sticky Headers.

[christianbach]

@sahrens I'm I correct that the problems lays in the fact that onLayout is extracted by ScrollViewStickyHeader and since it doesn't get the props from cellRenderer we receive a no cellKey and no index?

[christianbach]

@sahrens @hramos Updated the PR and now RNTester/SectionList works as expected. Thanks for testing!

[christianbach]

@sahrens @hramos Any updates? I know your super busy, but a update would be appreciated :)

[christianbach]

@hramos can I do anything to help speed up the review?

[facebook-github-bot]

hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hramos]

@christianbach thanks for adding the tests, I'm merging this.

[matthargett]

LGTM, thanks for adding a unit test!

[christianbach]

@hramos any updates? 😸

[sebryu]

You are calling component function _onLayout which is not available from child.props.
I think this line should look like this:

Suggested change

	child.props._onLayout(event);
	child.props.onLayout(event, child.props.cellKey, child.props.index);

[christianbach]

Hi @sebryu and thanks for reviewing this!

_onLayout is available and is part of the rewrite of CellRenderer to stop functions being created on every render

_onLayout = (e): void =>
     this.props.onLayout &&
     this.props.onLayout(e, this.props.cellKey, this.props.index);

[sebryu]

Hello @christianbach !
I read your code, and I noticed _onLayout function in CellRenderer class. However - you are trying to call it as child.props._onLayout, and simply - _onLayout function is not accessible from child.props. You would need to have ref to CellRenderer to call its functions.

So in this case, my suggested changes should work.

[christianbach]

My bad and thanks! I will change it straight away!

[christianbach]

Finally had the time to do a commit. Tested the change in our app without issues.

[christianbach]

@sebryu Do I need to do anything else, perhaps rebase from master?

[sebryu]

Hi @christianbach !

You have to ping RN guys, I just wanted to create similar issue, so I reviewed your code.
It looks ok for me though 👍

[christianbach]

@sahrens @hramos I think this is now ready to go

[pull-bot]

	Messages
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖	📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 14d41f2

[derekstavis]

@christianbach There's a conflict once again. Would you mind fixing it? Also, as soon as the conflict is resolved, can we merge this one or are we missing anything here?

[christianbach]

@derekstavis sure, will try to find some time this week.

[cpojer]

The reason we haven't landed this change yet is mainly because nobody at FB has volunteered to ensure that this won't break various surfaces. We have more than 1000 screens written using React Native and this change can potentially break any number of them by not updating things properly and we have no way of automatically detecting it :(

[sunnylqm]

How about make this optimization an option but not enabled by default.

[cpojer]

If we don't change the default behavior this will be much easier to land, yes!

[azizhk]

I tried my hand at not making this the default behavior and accepting an additional prop flattenParentProps, but I couldn't figure out how to retain the current onLayout behavior.

Changes: https://github.com/christianbach/react-native/compare/virtuallist-optimize-cellrenderer...azizhk:virtuallist-optimize-cellrenderer?expand=1

[brunolemos]

@cpojer: We have more than 1000 screens written using React Native and this change can potentially break any number of them by not updating things properly

Can't anyone add extraData={{}} on all FlatLists like Spencer suggested? Should garantee the lists will update and allow merging this pr.

[cihati]

@christianbach just wanted to say that I admire your tenacity to get this PR merged (after close to a year!), and I am really looking forward to it! Many thanks for putting in the effort.

[azizhk]

I was able to contain most of the changes to be opt-in and not be the default behavior.
The diff with @christianbach 's branch is here and with facebook's master branch is here.

Though this has a bit of hacky JS context trickery which might make it brittle. If acceptable, would like some guidance on how to write some good tests so that this does not break easily.

let virtualizedList = this;
this._onCellLayout = function (e) {
  virtualizedList._onCellLayoutGen(e, this.cellKey, this.index)
}

[chaitanyadeorukhkar]

This PR is beautiful. I hope this gets merged.

CellRenderer re-renders is one of the massive bottlenecks for our performance where we have a VirtualizedList with over 4000 rows.

Thank you everyone who is involved in this PR. Hope I can help here too.

[sunnylqm]

@azizhk If that is too diffcult, how about something like this:

export default (props) => {
   return props.experimentalOptimazation ?  
      <ExperimentalFlatList {...props} /> : <FlatList {...props} />
}

[azizhk]

@sunnylqm Thanks. We also have changes in ScrollViewStickyHeader.js so for that we would need to make changes in a lot of Components (like ScrollView). That gave me a different idea, wrapping inside an isExperimentalOptimization Context Provider. Something like:

<VirtualListExperimentalOptimization.Provider>
  <VirtualList {...props} />
</VirtualListExperimentalOptimization.Provider>

This can be passed down to ScrollViewStickyHeader. Let me whip up something over the weekend which agrees to the "not changing the default behavior" requirement.

[brunolemos]

@cpojer: If we don't change the default behavior this will be much easier to land, yes!

I have an idea to get this merged:

-const { extraData } = this.props
+const { extraData = {} } = this.props

By changing the default value of extraData to a new object reference, we keep the current FlatList behavior: update on every render.

If the user wants to enable the optimization they just need to pass extraData={null} or some memoized value.

Ideally this would not be necessary, but since internal facebook code is blocking the merge of this pull request this may be a good middle ground solution. No facebook code change necessary while still allowing people to opt in.

[azizhk]

I was able to retain the default behavior for onLayout as well: master...azizhk:exp_context_virtuallist

@cpojer Let me know if looks good and I'll create a PR and add tests.

[christianbach]

Perhaps it's time to close this PR?

[cpojer]

Alright, I'll close it but I still think it is valueable to make this change in a backwards-compatible way – I'm fine with introducing new props with new names that we can migrate people to.

[sunnylqm]

@cpojer There is a change in a backwards-compatible way, please review master...azizhk:exp_context_virtuallist
And I dont think @christianbach really means to close the pr but remind you to push this forward.

[azizhk]

@cpojer @christianbach I've created my PR: #27115. Hoping we can continue the discussion there. Please close it, if it does not seem viable.