search

Stanko / react-plx

React parallax component, powerful and lightweight
https://muffinman.io/react-plx/
MIT License
689 stars 51 forks source link

[fix] Fix some performance bottlenecks #59

Closed Whotakesmyname closed 4 years ago

Whotakesmyname commented 4 years ago

Fix some bottlenecks according to profiling in Chrome. Calculation time per frame is reduced by nearly 20ms, improve FPS from <40fps to 60fps in my case.

  1. Refactor omit function using Set instead of array to improve lookup performance
  2. Rewrite the algorithm of rounding to 3 decimal places replacing converting from float to string then to float again, which is quite expensive
  3. [Open to discuss] Only compare the scrollHeights of two element when use Math.max, because in my personal understanding, the scrollHeight is always equal to or greater than the clientHeight or offsetHeight. This comsumes several milliseconds.

I am not so familiar with Web dev and CSS things, so I am not 100% sure about the 3rd point. You can argue that this does not always hold water and remove these three lines' comment, although it works fine for me so far.

BTW I crafted a declaration file of Typescript for myself. I am happy to share it if knowing how I am supposed to do. But it has a little flaw that I have not figured out the proper way to assign tag type according to the tagName string in props (or I did but it was too lengthy to please me to get it done, and I chose to use forced type conversion as a workaround).

Whotakesmyname commented 4 years ago

Hi @Whotakesmyname, Thank you for the PR! In general, I like these changes. I have some concerns about older browsers (mainly IE) and I will try to test it more this week.

Do you have a good way of benchmarking this? That was always the biggest problem for me.

Cheers!

Thanks for your review~ Actually I am not any pro in web dev and I am merely refactoring my homepage while playing with React on a whim. I did hear that compatibility is a big concern here but I did not expect that old IE is still a concern. Sorry to hear that but I just have no good idea about an easy standard benchmark solution so far. I just profile when I feel it janky, and find possible improvement among those bottlenecks.

Thanks for your awesome module! That's the most ideal parallax package with React I have found so far. I really like those detailed comments in which I may not do so well. I may be able to help improve it if I am free.

Stanko commented 4 years ago

Great stuff, I added rounding to make it easier on browser to process CSS, but I will try it with and without rounding to see if there are any changes.

If you can please make changes to maxScroll calculations and I'll merge this one. Then I'll test it some more (probably this weekend) and release a new version.

As for old-browser support, unfortunately it is a must for libraries. I'll look to create a better build system to have two version, but I'm not sure when I will catch time to do it.

Whotakesmyname commented 4 years ago

Sorry for the late response! The maxScroll part has been edited. Check if they meet expectations.

About the use of Set or object, I decided to just keep using Set, despite babel will transpile it into array. Because using object here is quite a dirty trick, let alone I have not benchmarked the actual improvement. The transpiled result's degeneration is attributed to the target version and platform, and should not be compensated by drty tricks which depends on V8's specific implementation.

At least if such compensations are needed, they should not be mixed in this PR, and should be done separately and systematically.

Stanko commented 4 years ago

I just published v1.3.15, I made some changes to make sure to keep IE support, but to my naked eye it seems more performant. Thank you for your work! 🎉

Whotakesmyname commented 4 years ago

I just published v1.3.15, I made some changes to make sure to keep IE support, but to my naked eye it seems more performant. Thank you for your work! 🎉

Cheers