Page MenuHomePhabricator

[native] Add some memoization to `Timestamp` component

Authored by atul on Thu, Aug 31, 8:41 AM.
Referenced Files
Unknown Object (File)
Fri, Sep 15, 7:44 PM
Unknown Object (File)
Fri, Sep 15, 6:29 PM
Unknown Object (File)
Fri, Sep 15, 12:12 AM
Unknown Object (File)
Mon, Sep 11, 5:57 PM
Unknown Object (File)
Sat, Sep 2, 4:43 AM
Unknown Object (File)
Sat, Sep 2, 4:41 AM
Unknown Object (File)
Fri, Sep 1, 8:39 PM
F736258: 4cf68c.png
Thu, Aug 31, 8:48 AM



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

atul published this revision for review.EditedThu, Aug 31, 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!

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.Thu, Aug 31, 8:52 AM

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

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

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

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