Page MenuHomePhabricator

[native] Add memoization to `SidebarItem`
ClosedPublic

Authored by atul on Thu, Sep 7, 9:31 AM.
Tags
None
Referenced Files
F774041: D9099.id30876.diff
Mon, Sep 25, 5:09 PM
Unknown Object (File)
Sun, Sep 24, 7:14 PM
Unknown Object (File)
Sun, Sep 17, 4:38 PM
Unknown Object (File)
Fri, Sep 15, 6:05 PM
F746618: 3798ef.png
Thu, Sep 7, 10:13 AM
F746620: 014d54.png
Thu, Sep 7, 10:13 AM
Subscribers

Details

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

Disregard, pasted flame graph for incorrect render

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.

This revision was automatically updated to reflect the committed changes.