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

[fix] Fix some performance bottlenecks #59

Merged
merged 2 commits into from
Jan 20, 2020
Merged

[fix] Fix some performance bottlenecks #59

merged 2 commits into from
Jan 20, 2020

Conversation

Whotakesmyname
Copy link
Contributor

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).

Signed-off-by: Harrison Chen <[email protected]>
@Whotakesmyname Whotakesmyname requested a review from Stanko January 15, 2020 03:22
Copy link
Owner

@Stanko Stanko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@Whotakesmyname
Copy link
Contributor Author

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
Copy link
Owner

Stanko commented Jan 15, 2020

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
Copy link
Contributor Author

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 Stanko merged commit 6a13007 into Stanko:master Jan 20, 2020
@Stanko
Copy link
Owner

Stanko commented Jan 20, 2020

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
Copy link
Contributor Author

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

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

Successfully merging this pull request may close these issues.

2 participants