Page MenuHomePhabricator

[native] Add more memoization to `SwipeableMessage`
ClosedPublic

Authored by atul on Aug 31 2023, 9:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Sep 14, 10:20 PM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Unknown Object (File)
Wed, Aug 28, 3:47 AM
Subscribers

Details

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
arcpatch-D9054 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.Aug 31 2023, 10:06 AM

db4b8c.png (810×748 px, 80 KB)

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)

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

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 ↗(On Diff #30668)

Is it really beneficial to memoize like this?

294–301 ↗(On Diff #30668)

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
native/chat/swipeable-message.react.js
262–282 ↗(On Diff #30668)

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 CommIcons 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

atul edited the test plan for this revision. (Show Details)
atul added inline comments.
native/chat/swipeable-message.react.js
294–301 ↗(On Diff #30668)

I think we're okay with ternaries being included in props but we're not okay with EG

bool && <Component />

in JSX

This revision was landed with ongoing or failed builds.Sep 6 2023, 1:16 PM
This revision was automatically updated to reflect the committed changes.