[Performance] [Audit] ThumbnailImage triggers layout shifts, impeding FlatList performance

[jsamr]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

This report is part of #3957, scenario "Rendering Individual chat messages".

Flamegraph

The full commit log can be inspected with Flipper or React Devtools, see #3957 (comment) for instructions.

You can witness the layout shift behavior in the below Framegraph (c244, select VirtualizedList component)

The four commits (c222, c244, c266 and c280) are caused by last state of VirtualizedList component being changed, this is the signature of layout shifts. There are exactly 3 images in the report. So we have 1 initial commit + 3 layout shifts. Those 3 layout shifts span over 1.3s, this is considerable.

To get more into the details of VirtualizedList internals, a layout shift causes _onCellLayout callback to be invoked,
which calls _scheduleCellsToRenderUpdate which calls _updateCellsToRender, which in turn causes last state to change.

Cause

https://github.com/Expensify/Expensify.cash/blob/76c3157a41d86beb89c8cf107a68573280c06ce7/src/components/ThumbnailImage.js#L59-L63

The ThumbnailImage component first draws a 100x100 box to display the image, then waits for physical dimensions to be available and redraws the image with the newfound dimensions. This causes the FlatList cell layout to shift, and requires FlatList to recompute the position of each cell. This can have a sensible impact on performance if a discussion has many image attachments, since every shift will cause the FlatList to recompute the position of each cell, potentially re-rendering some cells.

Proposal: store images physical dimensions in markup

Store the physical dimensions of the image as attributes (width, height) of the <img> element. Thus, we can render the image one and only once and avoid those costly shifts.

[MelvinBot]

Triggered auto assignment to @stephanieelliott (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[marcaaron]

This can have a sensible impact on performance if a discussion has many image attachments, since every shift will cause the FlatList to recompute the position of each cell, potentially re-rendering some cells.

Oh wow, yeah this sounds like it could be pretty bad depending on how costly it is to recompute the position. I feel like we should be able to test the impacts with a very simple test case where 2 chats are switched back and forth and timed. e.g. one will have 500 regular comments the other has 499 comments and 1 attachments (or more) then we compare the difference.

Store the physical dimensions of the image as attributes (width, height) of the element. Thus, we can render the image one and only once and avoid those costly shifts.

Open to hearing thoughts on how we can store this information. It would be nice if there is some way to do this at the server level when generating the HTML for the image. Otherwise I think we will need to cache the size or delay rendering the FlatList until we have the sizes available. cc @thienlnam

[thienlnam]

Very interesting findings!

I feel like we should be able to test the impacts with a very simple test case where 2 chats are switched back and forth and timed. e.g. one will have 500 regular comments the other has 499 comments and 1 attachments (or more) then we compare the difference.

To add to this I'm pretty sure we have some kind of internal caching going on so this is probably more apparent the first time the chat loads as consecutive loads of the chat with images will be much speedier. Additionally, I am curious is this shifts all cells, or just visible cells around a resized image

It would be nice if there is some way to do this at the server level when generating the HTML for the image.

So we can store the image sizes when we generate the HTML, but before we render the image we still need to scale the image so that it is relative to the screen size. Additionally, when a user resizes the window we'd want to account for that and show the image resizing as well instead of showing a white screen until the resize is done. Something I'm thinking of that we could do is in conjunction to storing the full image size in the HTML, we have some ONYX.KEY store the device dimensions and then when rendering the image we do some scaling of the size inside the directly and so it only renders a single time with that specific size

[kidroca]

Additionally, I am curious is this shifts all cells, or just visible cells around a resized image

I'm pretty sure this would affect the position of every cell in the virtualized portion that is loaded, but I'm not certain it will recompute/re-render cells besides the ones containing images that change

The reason is we don't implement getItemLayout which means we leave the FlatList to compute how much height a cell takes and then calculate it's absolute position (offset) in the list. To do that the list has to render the cells.
After the cells are rendered if some change (the image containing ones) the list doesn't have to re-render the rest, but only change their offset position which should be quick, but maybe not quick enough

[jsamr]

@thienlnam

before we render the image we still need to scale the image so that it is relative to the screen size

Just want to point out that this scaling behavior is already implemented in react-native-render-html internal Image component. You can reuse the internal component in the custom renderer such as described here; just replace useIMGElementState with useIMGElementStateWithCache since you would have dimensions beforehand (and thus no "loading" state). Final note: the max width of the scaled image will be derived from computeEmbeddedMaxWidth prop.

See also default implementation:
https://github.com/meliorence/react-native-render-html/blob/d6e29365439a335891b3da98a6782d8f4dfc0a8f/packages/render-html/src/elements/IMGElement.tsx#L26-L48

[thienlnam]

Oh nice! In that case we should just be able to store the height/width with the image when we generate it and remove a lot of the imageWithSizeCalculation code

[MelvinBot]

Triggered auto assignment to @robertjchen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[deetergp]

Still no internal takers on https://github.com/Expensify/Expensify/issues/173677. I may pick it up before week's end if nobody beats me to it.

[deetergp]

Holding for N6

[deetergp]

Still no takers on the internal GH.

[deetergp]

No progress to speak of.

[deetergp]

No takers yet. Bumping this down to Monthly.

[deetergp]

Still no internal takers on https://github.com/Expensify/Expensify/issues/173677

[deetergp]

That internal GH has has been closed by Melvin. There have been some other discussions in the contributor sphere around ways to speed up image performance that may address the findings from this audit, so maybe by the next time it gets marked overdue we can just close it.

[kidroca]

I see we now have react-native-fast-image installed which might help with caching. We're also planning on caching images in Onyx. Caching might solve the problem, but only for consecutive loads

When you switch to different device you'll probably experience the problem again, or when you're cache expires

Adding the width and height to the img tag (as suggested) should address the layout shift problem

[deetergp]

Agreed, @kidroca. Going to close this since we've taken differing approaches to speeding up image handling.