microsoft / react-native-macos

A framework for building native macOS apps with React.
https://microsoft.github.io/react-native-windows/
MIT License
3.46k stars 132 forks source link

Dubious performance of FlatList with only 500 items and 1 <Text> element in each #1119

Open alexweej opened 2 years ago

alexweej commented 2 years ago

Environment

react-native -v:
% react-native -v 
zsh: command not found: react-native
% npx react-native -v
error: unknown option `-v'

npm ls react-native-macos:
% npm ls react-native-macos
superpower@0.0.1 /Users/alex/dev/superpower
└── react-native-macos@0.64.28

node -v:
% node -v
v16.14.0

npm -v:
% npm -v
8.3.1

yarn --version:
% yarn --version
zsh: command not found: yarn

xcodebuild -version:
% xcodebuild -version 
Xcode 13.2.1
Build version 13C100

Steps to reproduce the bug

const data = Array.from({length: 500}, (_, i) => ({
  id: i,
  text: `This is another item with index ${i}`,
}));

function App() {
  return (
    <FlatList
      ItemSeparatorComponent={() => <View style={{height: 2}}></View>}
      data={data}
      renderItem={({item}) => <Text>{item.text}</Text>}
    />
  );
}

Running this with npm run macos and scrolling through the list frequently stutters and shows blank temporarily while views are rendered. This is on a 2018 MacBook Pro. In my real application the item views are a fair bit more complex so this test case demonstrates the problem to a lesser extent than I am experiencing.

Expected Behavior

Smoother scrolling

Actual Behavior

Stuttered rendering while scrolling.

Reproducible Demo

I can provide this if need be, possibly I'm just doing something obviously wrong.

Additional context

I'm not aware of any way to run a react-native-macos app in production mode. AFAIUI React does a lot of sanity checking when running in dev mode, and maybe this is part of the problem...

Thanks!

ospfranco commented 2 years ago

I'm seeing the same problem in my app, in my case I have a row of emojis displayed in a flatlist, but scrolling is not smooth at all

Screenshot 000402 2

ospfranco commented 2 years ago

After some testing, it seems the flatlist is not correctly calculating its height, even when I set a fixed height in the style, it still renders all the rows at once.

Saadnajmi commented 2 years ago

After some testing, it seems the flatlist is not correctly calculating its height, even when I set a fixed height in the style, it still renders all the rows at once.

Could you give more info about your findings?

ospfranco commented 2 years ago

Here is a simple test:

https://github.com/ospfranco/macosFlatlistTest

Screen Recording 2022-05-12 at 18 54 38

ospfranco commented 2 years ago

Some more findings:

Edit: Maybe it has something to do with the fact that I'm using a heavily modified NSPanel instead of the default ViewController, the flatlist seems to have some sort of integrated bound checking and refuses to extend to the top of the window for example. It's mostly fine in my case, because I usually have a text input at the top of the window... but could be related to that

github-actions[bot] commented 1 year ago

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

Saadnajmi commented 1 year ago

Fwiw, this should be much faster in 0.71 @ospfranco

ospfranco commented 1 year ago

w00p w00p! is 0.71 out?

Edit: I think it is not out, but once it is out... could you provide a migration guide? I have too much custom code on sol and the migration to 0.71 was not easy for iOS/Android

Saadnajmi commented 1 year ago

We cut the stable branch, but are having troubles publishing it to NPM 😅. Working on it...

I'm not sure what a migration guide would have that's different from a standard RN upgrade? Which to be fair.. is already not fun..

The macOS native code should just work though.

ospfranco commented 1 year ago

I'll give it a try! :)

alexweej commented 1 year ago

I just (somewhat laboriously, maybe I did it wrong :/) upgraded my react-native-macos app to 0.71.1. I ended up upgrading all dependencies (using npm-check-updates) because of a zoo of peer dependency issues. I see very little change, quite possibly worse performance on the FlatList browser in my app. It for some reason calls the render function on each item very slowly. I'm setting up a repro project now to check with a bit more a clean slate.

alexweej commented 1 year ago

OK, I can see that even setting getItemLayout faithfully(?) it still has bad performance. There seems to be an issue where renderItem is called over and over again over the range of items in the data array that have already been rendered, even from wake up where there are only 14 items visible.

I've put the whole demo project here, and the log of a run on my system in this gist.

The pattern in the gist is quite predictable. It seems to decide it wants to increase the number of realized rows by 10 each time (even without any interaction), but then ends up calling the renderItem callback for each item, from the start, over and over again. I'm not sure if this is a red herring, but it seems odd that renderItem itself would be called repeatedly, right?

Thanks

alexweej commented 1 year ago

I tried replacing FlatList with a basic ScrollView. With only around 400 fairly basic items to render, my 2018 MacBook Pro locks up the main thread for 16s or so just trying to render this. (I'm not very experienced with macOS tooling.)

Screenshot 2023-04-10 at 16 09 34

I believed the architecture of React Native to try and do as much work off main thread as possible. Does this seem normal?

Thanks

alexweej commented 1 year ago

The React DevTools Profiler doesn't seem to indicate anything too excessive - renders taking milliseconds - though this is my first time using it so I have little intuition. The fact that the main thread is spinning 100% for tens of seconds makes me think that there is a performance problem with the inter-thread communication or something. (The heaviest render is 50ms in this profile where the app was frozen filtering the items in the list.)

Screenshot 2023-04-10 at 17 21 50
Saadnajmi commented 1 year ago

@alexweej The fact that renderItem is called many times implies to me the flatlist items aren't getting properly memoized, and are re-rendering on every update. I'm also curious, if you do the same profiling on iOS, do you see similar hangs on the main thread? If you're using hooks in renderItem, you might want to try the prop listItemComponent instead (that's unhelpfully not well documented I think.. but is renderItem but with better support for hooks I think).

Saadnajmi commented 1 year ago

Also, this might be helpful for RN-macOS upgrades... https://react-native-community.github.io/upgrade-helper/?from=0.68.69&to=0.71.0&package=react-native-macos They should be about the same as upgrading iOS to RN 0.71, which itself can be painful. There isn't much we can do about that unfortunately.

Edit: We also have this to help upgrades: https://microsoft.github.io/rnx-kit/docs/tools/align-deps

alexweej commented 8 months ago

Sorry for the silence. Life got in the way. I'm still on react-native-macos 0.71.1 but I wanted to try and debug things a bit more before attempting an upgrade. I noticed that in Xcode there is a UI tree that clearly includes fully realized views and subviews for every single item in the FlatList.

(lldb) po [[((RCTScrollContentView*)0x7fb7de025000) subviews] count]
409

Not sure if I misunderstand something about how views are retained, but this should never be the case, right?

alexweej commented 8 months ago

OK, I logged out the item index being passed to renderItem and loaded this into Google Sheets to see what was going on. It seems like some algorithm is going a bit wrong...

Screenshot 2023-12-22 at 13 56 24
alexweej commented 8 months ago

It's as if, every time it attempts to render another window of items to fill the view, it for some reason starts again at 0. Bad Big-O...

alexweej commented 8 months ago

I can confirm that I see the exact same behaviour on iOS with upstream react-native 0.71.0. From a wakeup iPhone SE screen with the following code it just calls renderItem in the above pattern, starting from item 0 every time and eventually reaching index 366. When I scroll down a small amount less than the row height, it clearly knows it needs to render one more row, but it starts at 0 again.

const data = Array.from({length: 500}, (_, i) => ({
  id: i,
  text: `This is another item with index ${i}`,
}));

const ITEM_HEIGHT = 20;

function App() {
  return (
    <FlatList
      ItemSeparatorComponent={() => <View style={{height: 2}}></View>}
      data={data}
      getItemLayout={(data, index) => ({
        length: ITEM_HEIGHT,
        offset: ITEM_HEIGHT * index,
        index,
      })}
      renderItem={({ item, index }) => {
        console.log('rendering item', index);
        return <Text style={{height: ITEM_HEIGHT}}>hi {item.text}</Text>
      }}
    />
  );
}

export default App;

Curiously in the log I see:

LOG rendering item 393 LOG rendering item 394 LOG VirtualizedList: You have a large list that is slow to update - make sure your renderItem function renders components that follow React performance best practices like PureComponent, shouldComponentUpdate, etc. {"contentLength": 7372.5, "dt": 654, "prevDt": 608} LOG rendering item 0 LOG rendering item 1

Given that the returned element is a Text element, I don't see how there's anything I can do better or more optimally. Can anyone please confirm that this seems wrong to them?

Thanks

alexweej commented 8 months ago

OK so it took me this long to discover that

So that explains why it's trying to render so many rows... Doesn't explain why it's calling renderItem on rows that are already rendered to Views, though.

bear-ei commented 3 months ago

OK so it took me this long to discover that

  • windowSize is measured in 'number of screens'
  • windowSize default is 21

So that explains why it's trying to render so many rows... Doesn't explain why it's calling renderItem on rows that are already rendered to Views, though.

Hey there, buddy. Are you still looking into that issue? Have there been any new breakthroughs? I've recently been building an app on the 0.73.* version and am experiencing terribly poor performance with FlatList. The optimizations that worked on iOS don't seem to be doing the trick here. If you have any good optimization methods up your sleeve, I'd really appreciate it if you could share them.

bear-ei commented 3 months ago

@alexweej The fact that renderItem is called many times implies to me the flatlist items aren't getting properly memoized, and are re-rendering on every update. I'm also curious, if you do the same profiling on iOS, do you see similar hangs on the main thread? If you're using hooks in renderItem, you might want to try the prop listItemComponent instead (that's unhelpfully not well documented I think.. but is renderItem but with better support for hooks I think).

Can we reopen an investigation into this issue? My component has been minimized to only <Text>{itemKey}</Text>, yet the scrolling performance on MacOS remains extremely poor. The complete component scrolls smoothly on iOS. I've implemented all optimizations outlined in the official React Native documentation, yet the performance remains terribly subpar, leading me to believe the issue lies at the native level.

 <AnimatedContainer testID={`listItem--${id}`}> // use React.memo()
            <Text>{itemKey}</Text> 
 </AnimatedContainer>
Saadnajmi commented 3 months ago

Hi, I want to say I'm aware this is still an issue, seems to be something specific to macOS (like, at the Appkit vs UIKit level) given that the same code on iOS seems to work fine, and I don't really have any leads on what's causing the issue.