Page MenuHomePhabricator

[native] Add some memoization to `Timestamp` component
ClosedPublic

Authored by atul on Aug 31 2023, 8:41 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 3, 7:44 PM
Unknown Object (File)
Sun, Nov 3, 7:44 PM
Unknown Object (File)
Sun, Nov 3, 7:41 PM
Unknown Object (File)
Sun, Nov 3, 7:23 PM
Unknown Object (File)
Mon, Oct 28, 10:41 AM
Unknown Object (File)
Fri, Oct 18, 3:18 PM
Unknown Object (File)
Tue, Oct 15, 8:43 AM
Unknown Object (File)
Fri, Oct 11, 1:51 PM
Subscribers

Details

Summary

Looking at the Flamegraph in the React Developer tools I was surprised to see that A. rendering Timestamp took a good amount of time and B. re-renders were expensive.

The memoization here ensured that Timestamp only renders once when we navigate to the MessageList.

This isn't a huge deal compared to some of the other components that can be optimized, but it was a super quick improvement (~3ms per message... which when multipled by eg 8 messages is ~24ms which is a bit more than a frame)

Test Plan

In the below flamegraph screenshot you can see how the Timestamp component ("Times..." in yellow) is a pretty large relative to the rest of ComposedMessage

dc96a9.png (2×3 px, 961 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.EditedAug 31 2023, 8:42 AM
atul edited the test plan for this revision. (Show Details)

4cf68c.png (238×2 px, 47 KB)

On re-render of MessageList, Timestamp is actually the component that takes the longest to render.

Cool find, wouldn't have expected this!

native/chat/timestamp.react.js
32 ↗(On Diff #30659)

I think this should be fine off of the top of my head

This revision is now accepted and ready to land.Aug 31 2023, 8:52 AM

code looks good, @rohan when you have a chance could you take a look at my inline comment, thanks!

native/chat/timestamp.react.js
32 ↗(On Diff #30659)

I think we still need to pass absoluteDate as children to SingleLine... I'm pretty sure that we always need opening and closing tags when we pass children in

https://github.com/CommE2E/comm/blob/master/native/components/single-line.react.js#L10

https://chat.openai.com/share/e8eab677-7000-46a5-9f54-8e5db0af93c4

native/chat/timestamp.react.js
18–24 ↗(On Diff #30659)

Nit, but I generally find it easier to read memoized styles when we push into an array rather than using a ternary operator. I also think it makes it easier to add more conditional styles in the future

native/chat/timestamp.react.js
32 ↗(On Diff #30659)

Oh yeah I misread absoluteDate is a prop not a child lol, my bad

You can consider using React.memo instead - might be a lot simpler.

In D9050#266876, @tomek wrote:

You can consider using React.memo instead - might be a lot simpler.

Personal preference is to use React.useMemo hook rather than wrapping the whole component in React.Memo so it's possible to memoize things w/ more granularity.

For example w/ this component, there's no reason for absoluteDate to be recomputed (which is expensive-ish) if we eg change the color theme and styles changes