optimize: initial render, ladder effect when loading data

[mbilenko-florio]

Initial render has little bit unpleasant effect of displaying element one by one.
Slowed down 4x video https://github.com/user-attachments/assets/373d6c93-5b98-44fe-b9b2-daac09f5b1f9

More desired would be to display all items at once, when elements was measured.

[jmeistrich]

Is this new? I don't think I remember seeing behavior like that before... I'm going to try to devote a day this weekend to optimizing so I'll take a look, unless you get there before me 😅

[mbilenko-florio]

@jmeistrich No, this is old issue, it was moderately annoyed by it for few weeks now. Also please note that this video is slowed down four times, so usually it's barely visible(and mostly on android). Also home tab of example app just became long enough, so issue is visible now.

I suspect it was introduced by 28ef4f7#diff-2ced1af601733dfe729a60b7ff933e704c1740861e5bbe43066fb909bd51b483

 onLayout={(event: LayoutChangeEvent) => {
                const key = peek$<string>(ctx, `containerItemKey${id}`);
                if (key !== undefined) {
                    // Round to nearest quater pixel to avoid accumulating rounding errors
                    const size = Math.floor(event.nativeEvent.layout[horizontal ? "width" : "height"] * 8) / 8;

                    updateItemSize(id, key, size); <-- calls calculateItemsInView for each element

                    const measured = peek$(ctx, `containerDidLayout${id}`);
                    if (!measured) {
                        requestAnimationFrame(() => {
                            set$(ctx, `containerDidLayout${id}`, true); <- request animation frame for next container
                        });
                    }
                }
            }}

We even request animation frame to make each container visible :)

[jmeistrich]

Oh yeah, I think you're right that that's the cause, and that it's just not super visible unless in slow motion 😅. So we probably should be queuing multiple updates into one batch instead of multiple animation frames. Should be easy to fix :)

[mbilenko-florio]

@jmeistrich Interesting fact @shopify/flashlist contains two native components, AutoLayoutView and CellContainer, which pretty much can be used by the LegendList. Using those we can avoid displaying containers in the invalid positions all together.
Will check if it has performance impact.

[jmeistrich]

I am hoping to solve layout purely in JS. I think it would be ideal for reducing installation complexity and supporting out of tree platforms.

I think we should get as far as we possibly can with JS, and identify what things if any could be significantly improved with native code. It is definitely likely that a native auto layout view would fix that one frame delay from the position changing after being measured. But I think it might also be possible with use of Animated values or other JS-only tricks.

So I think for now we should focus on making sure all the behavior is correct and get to a stable 1.0 soon, and then we can experiment with further optimizations?

[jmeistrich]

Do those native components from flashlist just drop straight in and work with no changes?

[jmeistrich]

I have this fixed. I'm just testing it more and doing other optimizations. I'll release an update shortly.

[mbilenko-florio]

@jmeistrich I've made lot's of experimenting with Animated and Reanimated, so far didn't found any performance benefit over just using just changing state.

Thing that also wasn't yet explored is using sync measurement of the containers yet(in the new architecture):

 useLayoutEffect(() => {
    const { width, height } = ref.current.unstable_getBoundingClientRect();
    console.log('The view measures %sx%s', width, height);
  }, []);

I've checked that, and heights of all containers are actually already known before the first render, but modifying it is a bit of the problem, that's why I've took a look how Flashlist does it, and they solved it with a native module in a very similar manner.

FlashList components are not readily available to be used by the LegendList because of slight differences in how recycled containers are handled, plus it's not compatible with maintainVisibleContentPosition.

Things that would be hard to do with pure JS and may require native module:

• instant first render (with LegendList we probably need to display at least one blank frame until we can show correctly measured containers.
• performant scrolling up with maintainVisibleContentPosition feature.

My idea now is to create separate npm package with FlashLists AutoLayout components completely separated, and experiment a little bit more with it. If there is any good results with it, maybe we can create peerDependency to this module and optional prop for LegendList like unstableUseFlashListContainers which allow to enable feature for people who really need it. For now including any native code in the LegendList does not make sense to me too.

[jmeistrich]

@mbilenko-florio Very interesting! I wasn't aware that useLayoutEffect fires so much earlier than onLayout... But it seems like it won't really help because updating the positions needs another render anyway?

I think you're right that it could solve the blank first frame, but I don't think that's a major issue for most apps. Of course there are certainly some where it could be a huge improvement, like in the movies example where it's rendering lots of horizontal lists.

I still think there's a lot of room for optimizations before we reach for native as the way to improve performance. I just spent today optimizing and #58 seems to have a pretty big impact! It has a lot of small changes, but the last one significantly reduces how often calculateItemsInView runs, and that seems to be a huge improvement (at least on my phone). I'm curious if you see good results from that?

As we talked about before, when switching between your "Implement advanced maintainVisibleContentPosition algorithm" commit and the one before it, I see it caused a drop in performance. But I was looking into it today and I can't figure out what specifically caused it. Especially since the reduction in number of calls to calculateItemsInView should reduce the overhead of the extra computations you added...

Do you have any ideas on how to optimize further?

I think the current level of performance is shippable, so even though I want to keep optimizing, I think we should focus on stabilizing and making sure all features work as they should. So please let me know if you find any more release-blocking issues!

I believe that #58 should fix the original problem in this post so please close this issue if it looks good to you? But these performance discussions are super interesting and we should continue them in a new issue?

[mbilenko-florio]

Issues seems to be fixed, moved discussion here #59