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

Authored by atul on Aug 31 2023, 8:41 AM.

Details

Reviewers

ginsu
rohan
tomek

Commits

rCOMMe630ac26992e: [native] Add some memoization to `Timestamp` component

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

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Aug 31 2023, 8:41 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 31 2023, 8:41 AM

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

atul added a reviewer: tomek.Aug 31 2023, 8:46 AM

atul attached a referenced file: F736258: 4cf68c.png. (Show Details)Aug 31 2023, 8:48 AM

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

Harbormaster completed remote builds in B22318: Diff 30659.Aug 31 2023, 8:58 AM

atul added a child revision: D9051: [native] Convert `ConnectedMessage` class component to functional `Message`.Aug 31 2023, 9:38 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

ginsu added inline comments.Aug 31 2023, 9:50 AM

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

rohan added inline comments.Aug 31 2023, 9:56 AM

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

Closed by commit rCOMMe630ac26992e: [native] Add some memoization to `Timestamp` component (authored by atul). · Explain WhySep 1 2023, 2:24 PM

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMMe630ac26992e: [native] Add some memoization to `Timestamp` component.