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

[Challenge] Improve performance #155

Closed
brunolemos opened this issue Jun 26, 2019 · 14 comments · Fixed by #173
Closed

[Challenge] Improve performance #155

brunolemos opened this issue Jun 26, 2019 · 14 comments · Fixed by #173

Comments

@brunolemos
Copy link
Member

brunolemos commented Jun 26, 2019

Issuehunt badges

About the problem

This app, DevHub, renders too many items at once: It can have dozens of columns, and each column can render hundreds of items.

This affects memory usage, scrolling and animations performance. Doing simple tasks like scrolling or opening modals starts to be laggy on this situation (see gifs below).

Can you make it blazing fast™?

Important note: This is a react-native-web project. This challenge is for the web part only.

gif

gif

Kapture 2019-06-26 at 17 45 29

The solution

Virtualized lists exists to solve this problem. With them, the app would render only the visible items (~5 columns, ~5 items per column), which would be around 25 items instead of hundreds.

Virtualization is a must-have for this kind of project.

Progress so far

Attempt #1: FlatList

FlatList has a built-in virtualization. It actually works super great on native mobile, but at the web this happens:

(same gif as shown above)

gif

So I decided to disable virtualization: https://github.com/devhubapp/devhub/search?q=disableVirtualization&unscoped_q=disableVirtualization

This is the result without virtualization:

gif

Looks great, right? But it's not a sustainable solution, only works well for <100 items. Disabling virtualization has other consequences as well, like animations, memory usage, first-render time, etc. So we shouldn't disable virtualization, we kinda need it.

Attempt #2: react-window

I thought the performance issue was with FlatList, so I decided to try react-window. You can check the code on this branch: #152

Same problem happened: the app actually felt slower instead of faster when enabling virtualization.

I'm not sure why, but I suspect the components rendering are too expensive (ps: may not be it! needs more investigation; I couldn’t find the root cause of the performance issues yet, maybe you can?).

There's definitely lot's of script running while scrolling (see this comment for an updated screenshot):

image

Challenge

Can you fix the performance? Can you find what’s the root cause is? Is the only solution re-writing the components to make them less expensive? Would React Concurrent mode help? Is it because the react tree is rendering too many components? Is it because of the position of the planets? 🤪

Do you have any other guesses or insights? Let us know in the comments!
Please try your guesses locally in your machine to see the results.

How to run the web project

It's super quick to get it up and running:

  • git clone git@github.com:devhubapp/devhub.git
  • cd devhub
  • Enable virtualization for web
  • yarn
  • Run
    • Debug mode: yarn dev:web
    • Release mode: yarn workspace @devhub/web build && yarn workspace @devhub/web serve

After running the [web] project, make it slow: Create lot's of columns, and tap "Load more" on each one to have lot's of items loaded. See how some animations and scrolling starts to become laggy.

Prize

To win the bounty, you need to submit your code changes in a pull request with a considerable performance fix.


IssueHunt Summary

brunolemos brunolemos has been rewarded.

Backers (Total: $200.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@gaearon
Copy link

gaearon commented Jun 26, 2019

yarn dev:web

Are you sure you're not running into fixed overhead of DEV mode?

@brunolemos
Copy link
Member Author

@gaearon hi dan, yes I tried on both debug and release.

@alizahid
Copy link

I’m building an app with a very similar layout and running into the same problems. Will share findings/solutions!

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jul 4, 2019
@issuehunt-oss
Copy link

issuehunt-oss bot commented Jul 4, 2019

@issuehunt has funded $200.00 to this issue.


@OriginLive
Copy link

OriginLive commented Jul 4, 2019

https://blog.discordapp.com/using-rust-to-scale-elixir-for-11-million-concurrent-users-c6f19fc029d3
https://blog.discordapp.com/why-discord-is-sticking-with-react-native-ccc34be0d427
Read these two. Their engineering blog shows answers on some of those questions you guys have. Hope it helps

also: If your FlatList is rendering slow, be sure that you've implemented getItemLayout to optimize rendering speed by skipping measurement of the rendered items.

@brunolemos
Copy link
Member Author

OriginLive, thanks for the links! But this issue is related to the web; react-native performance is actually ok. They said in the article they made their list in objective-c, which doesn’t apply here for web. Need some specific investigation.

@brunolemos
Copy link
Member Author

brunolemos commented Jul 7, 2019

Here’s some more information after lots of investigations:

It has different behaviors per browser: On Chrome it’s FPS loss (laggy); on Firefox and mobile safari it’s smooth but render big empty spaces while the components are not rendered.

When I scroll and react-window adds a new list item to the DOM, you can see the FPS dropped to 6 (other frames have ~72fps) (click to zoom):

Screen Shot 2019-07-06 at 20 17 33 copy

Here’s a gif of the lag that happens for each item when scrolling (notice how the scroll "stops" when the next item starts being rendered):

Kapture 2019-07-06 at 21 36 33

Here Chrome says the Scroll Update interaction is laggy because it’s waiting for the main thread for a long time:

Screen Shot 2019-07-06 at 22 27 52 copy

To get the result above, I used the branch react-window with FixedSizeList and scrolled super slowly. It’s using dev version of react so I can see the component names, but the lag happens on production too (less critical on production, of course, but still very perceivable by the user, hurting smoothness).

It’s possible my components are expensive to render, but if that’s the case why are they expensive? How to improve?

Anyone has some more insights?

@brunolemos
Copy link
Member Author

brunolemos commented Jul 7, 2019

Okay I've just re-watched Dan's Beyond React 16 and Andrew's Concurrent React awesome talks and I'm convinced Time Slicing could help us here.

In fact Dan even mentioned in the video that it can improve scrolling responsiveness by not blocking the main thread, which may be the main things we need here based on my findings above.

Needs to make react-native[-web] support Concurrent Mode first though (Caleb sent me his fork).

I'll try these next, I'm excited!

48B6E57C-83DC-41D0-8318-80ECF3D8E092

9DBDAD25-DDB7-4BF4-8A44-B27969D47ACE

@brunolemos
Copy link
Member Author

brunolemos commented Jul 12, 2019

I used react-window, enabled Concurrent Mode, optimized react-native-web styles and unfortunately the result still isn’t satisfactory: https://alpha.devhubapp.com (code)

Note: also it’s quite buggy since react-window’s DynamicSizeList is not production ready yet.
Note2: similar performance results when using VariableSizeList tho.

@brunolemos
Copy link
Member Author

brunolemos commented Jul 12, 2019

I have found that some components are calling some ""expensive"" methods on render, like the function stripMarkdown and some color manipulations. When removing these calls I can notice an improvement.

Need to search for more work that I can move out from render.

Note: In the alpha link above, these methods have not been removed yet

@brunolemos brunolemos changed the title [Challenge] Improve performance of big lists [Challenge] Improve performance Jul 15, 2019
@polarathene
Copy link

If certain methods are quite expensive but their results don't take up too much memory, you could look at using memoization? Moize is a good library for this where you can limit the cache size with various settings(LRU limit, expiry time, etc). You could perhaps also warm that cache layer up with your list data prior to rendering the first time via virtualized list if you're getting a notable hit with first appearance of a list item.

Rendering wise, a virtual list helps, but you can also take into account how the browser groups/layers content for paint/rasterization. Prior to that, ensure that React isn't needlessly re-rendering components because of changes such as a new item coming into view on the virtual list.

I came across this from the issuehunt site btw, I'm quite busy but if I end up with some free time, I might give this a shot as this is my kind of thing :)

@brunolemos
Copy link
Member Author

@polarathene thanks for taking the time!

I'm currently rewriting parts of the app, mainly the card components, to be simpler and less expensive. Result is looking promising so far, I'll know for sure in a couple days!

@brunolemos brunolemos added this to the v0.94.0 milestone Aug 24, 2019
@brunolemos
Copy link
Member Author

brunolemos commented Aug 27, 2019

Hi everyone, please try the beta and let me know if it feels faster for you: #173

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Aug 28, 2019
@devhubapp devhubapp deleted a comment from issuehunt-oss bot Aug 28, 2019
@RichardLindhout
Copy link

React-native-web components are slower than normal components. It also helps to rewrite list items in a web version with normal css.

@brunolemos brunolemos unpinned this issue Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants