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

Forced reflow - Performance bottleneck #1274

Open
Jeevan-Kishore opened this issue Jun 15, 2018 · 20 comments
Open

Forced reflow - Performance bottleneck #1274

Jeevan-Kishore opened this issue Jun 15, 2018 · 20 comments

Comments

@Jeevan-Kishore
Copy link

Jeevan-Kishore commented Jun 15, 2018

Hello akiran,

The slider has been an amazing addition for many projects of ours. Thank you.

We were trying to optimize all our apps using several metrics.

A case in which the slider seems to trigger expensive forced reflow of our layouts which is impacting the performance on a huge scale.

We wanted to know if it is such that this issue can be fixed or our implementation of it is not the optimal in a way.

  • helpers.js
    getWidth: function getWidth(elem) { return elem && (elem.getBoundingClientRect().width || elem.offsetWidth) || 0; },

Can this be optimized?

screen shot 2018-06-15 at 11 31 54 am 2

screen shot 2018-06-15 at 11 46 37 am 2

Thank you for your time.

@akiran
Copy link
Owner

akiran commented Jun 15, 2018

Can you explain bit more

@sapkra
Copy link

sapkra commented Jun 21, 2018

#1204 describes the same problem.

@aripekkako
Copy link

aripekkako commented Jun 25, 2018

Accessing elem.offsetWidth always causes a forced reflow (see https://gist.github.com/paulirish/5d52fb081b3570c81e3a for a list of other properties that causes this). The library needs this property when producing the initializedState

let listWidth = Math.ceil(getWidth(ReactDOM.findDOMNode(spec.listRef)));

which is used by the inner slider when state updates are made

updateState = (spec, setTrackStyle, callback) => {

updateState is called in a few occasions, but most notably by componentWillReceiveProps. Essentially, any time any of the props of the slider change, a reflow seems to be forced.

I'm not 100% sure, but I think we could optimize this behavior by recycling the listWidth and trackWidth parameters when the state is updated through componentWillReceiveProps in most cases.

@Jeevan-Kishore
Copy link
Author

Hello aripekkako,
Thanks for the details of it, it seems like recycling listWidth & trackWidth could give us a perf boost.
Can we please attempt this at the earliest?

@aripekkako
Copy link

I'm not a maintainer of this repository. I think your best bet is to

  1. make a quick-and-dirty test to see if your performance problems are fixed with the recycling.
  2. If it works, then submit a PR with a proper fix
  3. Hope Akiran will have time to look the PR through and prepare to maintaining your own fork in the meantime.

@sapkra
Copy link

sapkra commented Jun 27, 2018

I think this won't be the solution because we are talking about a performance problem in componentDidMount and not componentWillReceiveProps. You can see it in the screenshot.

I tried to wrap the content of componentDidMount into a setTimeout function to let offsetWidth and offsetHeight not block the first rendering of the react app.

@aripekkako
Copy link

Oh. Missed that from the screenshots. Getting properly rid of the initial layout forcing componentDidMount falls beyond my expertise, as I'm not that familiar with layout trashing in general.

setTimeout or similar (maybe requestAnimationFrame?) might indeed work. updateState in asynchronous by nature because it only calls React's setState in the end so if you find a working solution for componentDidMount it might be wise to just do it inside updateState so all actions updating the state will be safe from layout trashing.

@Jeevan-Kishore
Copy link
Author

Jeevan-Kishore commented Jul 10, 2018

^^ @akiran did you get a chance to look at this?

@aaronkrohn
Copy link

Any more news on this issue?

@justinstander
Copy link

Any update on a fix?

@aripekkako
Copy link

@justinstander This repo has not been updated for 4 months and last release was 6 months ago. No activity from the maintainer on any issues or PRs during the last 4 months.

@alaminut
Copy link

Just tried this component for a project and styling does not work. Haven't noticed repo is not being maintained anymore. Sad I had to dig through issues to find this as a comment.

@Jeevan-Kishore
Copy link
Author

It's been a while since the issue has been up there, leading to question. Is react-slick on it's way to be deprecated?

@antoniomiotto
Copy link

Any update on this?

@Tsury
Copy link

Tsury commented Jun 28, 2019

Suffering from the exact same issue.

@carlitosz
Copy link

Still getting this issue :[

@MuhammadHasham23
Copy link

Same issue...

@markparky
Copy link

+1

@BHARANA4
Copy link

same issue

@jcmnavia
Copy link

Same here, nothing new on this?

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