Details

Reviewers

ginsu
tomek
rohan

Commits

rCOMM402c80e99845: [native] Add more memoization to `SwipeableMessage`

Summary

As mentioned in D9053, SwipeableMessage is "expensive" to render. As I've been looking through the flamegraphs it seems like a general trend that gesture handlers/Reanimated components are expensive to render and we should memoize as agressively as possible so we only "eat the cost" once.

In this diff I add more memoization to SwipeableMessage to reduce re-renders of the nested SwipeSnake and PanGestureHandler components.

Depends on D9053

Test Plan

Things continue to work as expected, profiler confirmed that this reduced re-renders.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Aug 31 2023, 9:56 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 31 2023, 9:56 AM

Okay looks like that stopped re-render of one of the SwipeSnakes, but we still have another SwipeSnake and panGestureHandler re-rendering.. will take another look and update Test Plan with before/after flamegraphs

(As an aside, love that I can validate whether changes are effective w/ the profiler instead of just going off of often mistaken instinct)

Harbormaster completed remote builds in B22323: Diff 30664.Aug 31 2023, 10:14 AM

a little more memoization. i think i'm going to "call this diff" for now. the core issue is the children React.Node that gets drilled down that contains InnerTextMessage. I'll need to memoize that "futher up the component tree" to really make an impact

Harbormaster completed remote builds in B22324: Diff 30668.Aug 31 2023, 11:24 AM

atul added a child revision: D9055: [native] Add some memoization to `InnerTextMessage` component.Aug 31 2023, 11:38 AM

Shouldn't this diff have a dependency on D9053? Also, the test plan seems to be incomplete

"Before":

"After":

The code looks ok, but I'm not sure if all the memoizations introduced here are really beneficial.

native/chat/swipeable-message.react.js
262–282	Is it really beneficial to memoize like this?
294–301	Maybe we can compute `translateXInterpolator` as separate values inside this memo? Not sure if this syntax follows our rules https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e

This revision is now accepted and ready to land.Sep 1 2023, 5:30 AM

atul added inline comments.Sep 1 2023, 1:03 PM

native/chat/swipeable-message.react.js
262–282	Yeah it turned out to be beneficial. This `React.Memo` block was being recomputed every time `translateX` changed which resulted in `<CommIcon name="reply-filled" size={14} />` being re-rendered. It took about ~2.5ms for each of the `CommIcon`s to be recreated and at 2/message and 8 messages on screen it came to ~40ms (on my dev machine on iOS Simulator). Not a massive deal, but change was easy enough to make IMO