[native] Add memoization to `SidebarItem`
ClosedPublic
Actions

Authored by atul on Thu, Sep 7, 9:31 AM.

Details

Reviewers

ginsu
tomek
rohan

Commits

rCOMM3eae57836282: [native] Add memoization to `SidebarItem`

Summary

Call to shortAbsoluteDate in particular takes surprisingly long (like 5ms each time and it's re-rendering like 5 times and there are 5 instances so 5*5*5ms = 125ms total when doing the "navigate to thread w/ varun scenario").

Test Plan

Continues to work/appear as before.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Thu, Sep 7, 9:31 AM

Herald added a subscriber: ashoat. · View Herald TranscriptThu, Sep 7, 9:31 AM

atul edited the summary of this revision. (Show Details)Thu, Sep 7, 9:35 AM

atul published this revision for review.Thu, Sep 7, 9:37 AM

Please wait for CI to pass before landing

native/chat/sidebar-item.react.js
19–22 ↗	(On Diff #30847)	Doing a global search on this I've noticed that this pattern is repeated a few times. As a follow up diff, should we make `shortAbsoluteDate` into a hook (maybe like `useShortAbsoluteDate`) so that we can use `useMemo` in the hook and simplify this call here and in other places?

This revision is now accepted and ready to land.Thu, Sep 7, 9:46 AM

Harbormaster completed remote builds in B22431: Diff 30847.Thu, Sep 7, 9:48 AM

atul added a child revision: D9100: [native] Memoize `SwipeableThread`.Thu, Sep 7, 10:05 AM

Disregard, pasted flame graph for incorrect render

atul added inline comments.Fri, Sep 8, 11:02 AM

native/chat/sidebar-item.react.js
19–22 ↗	(On Diff #30847)	Yeah I'll jot down that we should look at all `shortAbsoluteDate` callsites. There's other low-hanging fruit so I'll probably prioritize that first.