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

ScrollView: investigate expensive layout measurements #473

Closed
brunolemos opened this issue May 9, 2017 · 21 comments
Closed

ScrollView: investigate expensive layout measurements #473

brunolemos opened this issue May 9, 2017 · 21 comments

Comments

@brunolemos
Copy link
Contributor

brunolemos commented May 9, 2017

What is the current behavior?

When scrolling, there is a huge lag.
Chrome shows some warnings on console that could help find the issue.

Most violations point to UIManager measureLayout), the last one to a debounce function:

[Violation] 'requestAnimationFrame' handler took 93ms
[Violation] Forced reflow while executing JavaScript took 184ms
[Violation] 'requestIdleCallback' handler took 242ms
[Violation] 'setTimeout' handler took 74ms

image

Steps to reproduce

Tried to create a minimal test case on codepen but could not. Maybe it's because my component tree is relatively deep and heavy styled (ps: using styled-components). Added you to the repository so you can run it. PS: On mobile, works fine. Bad performance only on web.

  1. Accept my invitation to the repo
  2. yarn
  3. cd web/
  4. yarn
  5. npm start (on web folder)
  6. Open notifications tab
  7. Scroll vertically

Environment (include versions)

OS: macOS 10.12.4
Device: Macbook Pro 2016
Browser: Chrome 58.0
React (version): 16.0.0-alpha.6
React DOM (version): 15.5.4 (react-native-web does not support >= 16)
React Native (version): 0.43.2
React Native for Web (version): 0.0.94

@brunolemos
Copy link
Contributor Author

brunolemos commented May 9, 2017

I'm running some profiling to try to narrow it down...
Everything seems to come back to UIManager measureLayout/getRect functions

UIManager/index.js

@brunolemos
Copy link
Contributor Author

Maybe some tips so my component measure layouts faster?

@RangerMauve
Copy link

Maybe there's a way to improve performance by using Element.getBoundingClientRect()

@necolas
Copy link
Owner

necolas commented May 9, 2017

Are you using the onContentSizeChange callback?

If you're using styled-components with RN/RNW that is also likely to be very slow and wasteful.

@brunolemos
Copy link
Contributor Author

@necolas thanks for the answer.

I'm not using onContentSizeChange or something similar on my code, but there is indead a onLayout=bound_handleContentOnLayout on my react tree that internally calls onContentSizeChange and apparently it's coming from the react-native-web ListView Component

image

@RangerMauve
Copy link

@brunolemos Is your ListView deeply nested within your app?

@brunolemos
Copy link
Contributor Author

@RangerMauve yes, it's trello-like

@RangerMauve
Copy link

RangerMauve commented May 9, 2017

The getRect function you pointed up above will be slower depending on how deeply nested it is within the DOM. I'm not 100% certain, but I think that Element.getBoundingClientRect() might be faster alternative.

Regardless, this means that ListViews are a huge performance issue if they're not near the root of your DOM, and more generally onLayout is a huge performance issue if it's not called near the root.

@brunolemos
Copy link
Contributor Author

brunolemos commented May 9, 2017

Found out something (kinda obvious but might help).

The ListView is initially rendering 10 item.
When I scroll, it adds more items.

If I set initialListSize={100}, app is slower to start but the scroll gets much better.
I guess this multiple row addition + measureLayout on a deep listview is making things slow.

@necolas
Copy link
Owner

necolas commented May 9, 2017

Yeah the ListView is not really worked on, not performant, and will be removed once React Native deprecates it. getBoundingClientRect doesn't return the correct values (for RN) when scale transforms are applied to the element, but I'll look at alternative ways to correct the value without iterating up the tree.

@tazsingh
Copy link
Contributor

I've done a benchmark of getBoundingClientRect yesterday to see if it would be any faster for #474, but the performance of it was generally 20% slower than the current method.

@necolas
Copy link
Owner

necolas commented May 24, 2017

o_O

@tazsingh
Copy link
Contributor

Haha I was also shocked as I was expecting it to be faster for sure. Thought it would be quick win but nope... And I'm kind of confused why as everything that I know suggests that it should be faster... But hey if it doesn't work then it doesn't work. Thought I'd loop back here to say that it's also not more performant.

@tazsingh
Copy link
Contributor

Can you let me know if #490 solves some of this for you?

@necolas
Copy link
Owner

necolas commented May 25, 2017

Might still be worth using getBoundingClientRect if there are performance issues with the current approach in deeply nested trees of nodes

@brunolemos
Copy link
Contributor Author

brunolemos commented Jun 4, 2017

I just noticed my component tree go down to ~100 items deep. That's insane. Also lot's of Connect(XXX), withXXX and other wrappers.

So although there are improvements to be made on react-native-web layout measuring code, I really need to do some work on my part too.

image

@RangerMauve
Copy link

@brunolemos Yeah, I had a similar problem. I also hacked React.createElement so that it would execute any stateless functions as functions if they didn't need to be components for stuff like context

@necolas necolas changed the title Performance issues on scroll + Chrome violations (e.g. [Violation] 'requestAnimationFrame' handler took 93ms) ScrollView: investigate expensive layout measurements Jun 6, 2017
@aFarkas
Copy link

aFarkas commented Mar 16, 2018

@brunolemos
Can I have access to your repo. I'm interested to look into this. (Although, I'm only interested in perf issues that are caused by forced sync reflows.)

@brunolemos
Copy link
Contributor Author

brunolemos commented Mar 18, 2018

@aFarkas you can see it running here: http://brunolemos.github.io/devhub/

edit: this link is for the old slow version

I didn't make the code public yet neither shared the project with anyone.

@brunolemos
Copy link
Contributor Author

brunolemos commented May 12, 2018

I rewrote my app from scratch and I don't have this issue anymore. Performance is great.

As I didn't isolate the issue, I don't know the real cause of it, but one of the changes was that I removed styled-components and took extra care to avoid unnecessary re-renders.

@necolas
Copy link
Owner

necolas commented May 12, 2018

Thanks for following up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants