react-waypoint icon indicating copy to clipboard operation
react-waypoint copied to clipboard

Call out in Twitter Lite post

Open lencioni opened this issue 8 years ago • 12 comments

Up until recently, we were using react-waypoint, which worked well for us. However, in chasing the best possible performance for one of the main underlying components of our application, it just wasn’t fast enough.

Waypoints work by calculating many different heights, widths, and positions of elements in order to determine your current scroll position, how far from each end you are, and which direction you’re going. All of this information is useful, but since it’s done on every scroll event it comes at a cost: making those calculations causes jank–and lots of it.

https://medium.com/@paularmstrong/twitter-lite-and-high-performance-react-progressive-web-apps-at-scale-d28a00e780a3

Is it possible that we've fixed these problems or do they still exist and if so are they fixable?

cc @paularmstrong

lencioni avatar Apr 11 '17 23:04 lencioni

It looks like some of the changes they've made have been outlined here: http://itsze.ro/blog/2017/04/09/infinite-list-and-react.html

lencioni avatar Apr 11 '17 23:04 lencioni

Hope this didn't spring up any incorrect assumptions. I wanted to point out that react-waypoints were great for us. However, the problem was that with them, for infinite scrolling lists as big and complex as ours, we had to do too many calculations and queries that caused re-render/re-paints in the browser.

By no means does that mean anyone that is using this package should be concerned. The findings we had were localized to our application and won't necessarily be reflected in another.

paularmstrong avatar Apr 11 '17 23:04 paularmstrong

Thanks for the context @paularmstrong. I just want to make sure that if there are any performance opportunities for this project that we are taking them--whether they are in code improvements, better defaults, or documentation.

Looking at your screenshot and waypoint code, this particular bit of jank seems to happen because of the getBounds call in handleScroll. https://github.com/brigade/react-waypoint/blob/b23770d57fa66b4e20eeb3e8d80fa60d85135f36/src/waypoint.jsx#L161

It would be really helpful for me to know what version of react-waypoint you were using when you did your profiling.

lencioni avatar Apr 11 '17 23:04 lencioni

Also thanks for the article. We've been doing a bunch of performance work at Airbnb and have done a lot of similar things that you've outlined. Got a long way to go, but we are making progress all the time!

lencioni avatar Apr 11 '17 23:04 lencioni

It would be really helpful for me to know what version of react-waypoint you were using when you did your profiling.

We had a fork from around v2. I don't really remember the context for why we forked it, as that was a loooong time ago. However, that getBounds really was the biggest hindrance to us. We already have to calculate the height of every single item rendered into a timeline up front, so doing an extra call that triggers jank is problematic and unnecessary.

We also had the issue that we had to either create and re-create waypoints often with the InfiniteList (see explanation), or move them around in the DOM a lot, and this was extra problematic. If they had just been static and we weren't constantly changing the size of the scrollable area, it'd probably be totally fine.

paularmstrong avatar Apr 12 '17 00:04 paularmstrong

Thanks @paularmstrong and @lencioni for bringing this to our attention. I see this as a challenge, we have the opportunity to affect scrolling performance in a lot of web apps, so let's do it!

I added a test page so that it's easier to profile in #178. The first step will be to work out a good repro case (the current test page performs 60fps despite 1000 waypoints).

trotzig avatar Apr 12 '17 11:04 trotzig

@trotzig I think you'll only experience the performance issues we were having if you start firing off other tasks and mounting/unmounting components while scrolling (and from being triggered by waypoints).

paularmstrong avatar Apr 12 '17 13:04 paularmstrong

I think that's right. I believe this is caused by layout thrashing, where one operation invalidates layout (e.g. mounting/unmounting components) followed by an operation that needs to compute layout (e.g. waypoint calling getBoundingClientRect). I think the best we can do is try to cluster the operations that compute layout so we end up with AAAABBBB instead of ABABABAB.

Practically speaking, this would probably amount to deferring calling the callbacks in the scroll handler until the next animation frame. I'm on the fence about whether this async behavior should be the responsibility of waypoint or the consumer. I'd like things to be fast by default though, so perhaps we could do this and then provide a prop for folks to make it synchronous if they need it? Or maybe we don't need the prop--what might be the use cases for such a thing?

Similarly, it might be worth deferring the call to find the scrollable ancestor in componentDidMount so they can be clustered. This might be even more important if you have multiple react trees on the same page that render waypoints.

lencioni avatar Apr 12 '17 15:04 lencioni

Similarly, it might be worth deferring the call to find the scrollable ancestor in componentDidMount so they can be clustered.

We actually forced a prop to either be an element or a function that finds the element for scrollableAncestor and cached it in componentDidMount (since the ancestor can't really change in non-fiber versions of react).

this would probably amount to deferring calling the callbacks in the scroll handler until the next animation frame

We also already did this. In our case, it might have been more practical to wait for 2 animation frames.

I'd maybe avoid too much optimization. You all have already done a great job and react-waypoints is fast. If someone needs to get out that little bit of better performance like we did, I think it should be on them.

paularmstrong avatar Apr 12 '17 15:04 paularmstrong

Yeah that's fair. Maybe we could have a section in the readme that discusses performance and offers tips to help folks optimize.

lencioni avatar Apr 12 '17 15:04 lencioni

Talking about performance: am I miss something or there is reason to not use use debounce? I scanned source code and haven't found it.

Related:

  • What forces layout / reflow
  • https://github.com/wilsonpage/fastdom
  • https://stackoverflow.com/questions/23123138/perform-debounce-in-react-js
  • https://csstriggers.com/

stereobooster avatar May 29 '17 22:05 stereobooster

Yeah, those are all valid concerns. We've taken the approach of making react-waypoint fast without resorting to debouncing (although there was a time when we had support for injecting a debounce method). Are you noticing slowness in your app at the moment that's being caused by react-waypoint?

If you're interested in doing some profiling/benchmarking, there's a test page included in this repo.

trotzig avatar May 30 '17 10:05 trotzig