Page MenuHomePhabricator
Differential D9100

[native] Memoize `SwipeableThread`
ClosedPublic
Actions

Authored by atul on Sep 7 2023, 10:05 AM.
Tags
None
Referenced Files
F3399283: D9100.id30848.diff
Mon, Dec 2, 2:50 AM
F3399274: D9100.id30879.diff
Mon, Dec 2, 2:47 AM
F3399061: D9100.id30880.diff
Mon, Dec 2, 1:37 AM
F3398698: D9100.diff
Mon, Dec 2, 12:05 AM
Unknown Object (File)
Fri, Nov 29, 4:01 PM
Unknown Object (File)
Mon, Nov 25, 10:15 PM
Unknown Object (File)
Thu, Nov 21, 6:51 AM
Unknown Object (File)
Thu, Nov 21, 6:51 AM
View All 77 Files
Subscribers
ashoat

Details

Reviewers
ginsu
rohan
tomek
Commits
rCOMM1c5e50b949b6: [native] Memoize `SwipeableThread`
Summary

The real performance gains will come from adding memoization to ChatThreadListItem and ChatThreadListSidebar, but this is a prerequisite for those optimizations to make a difference.

Depends on D9099

Test Plan

Continue to look and behave as expected.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Sep 7 2023, 10:05 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 7 2023, 10:05 AM
Harbormaster completed remote builds in B22432: Diff 30848.Sep 7 2023, 10:23 AM
atul requested review of this revision.Sep 7 2023, 10:23 AM
atul added a child revision: D9102: [native] Memoize construction of `sidebars` in `ChatThreadListItem`.Sep 7 2023, 10:51 AM
tomek accepted this revision.Sep 8 2023, 2:22 AM
tomek added inline comments.
native/chat/swipeable-thread.react.js
82–93

Creating a memo that depends on children doesn't always work. The problem appears when SwipeableThread has more than one child

<SwipeableThread>
  <View .../>
</SwipeableThread>

vs

<SwipeableThread>
  <View .../>
  <View .../>
</SwipeableThread>

In the second case, props.children is an array that is created on every render, which means that this memo will be recomputed every time. Not sure if it can be fixed from this component, but we can check the usages of SwipeableThread.

This revision is now accepted and ready to land.Sep 8 2023, 2:22 AM
atul added inline comments.Sep 8 2023, 7:36 AM
native/chat/swipeable-thread.react.js
82–93

I think we should be fine based on

a0eadb.png (1×2 px, 208 KB)

atul attached a referenced file: F748040: a0eadb.png. (Show Details)Sep 8 2023, 7:41 AM
atul updated this revision to Diff 30879.Sep 8 2023, 1:23 PM

land

Harbormaster completed remote builds in B22461: Diff 30879.Sep 8 2023, 1:41 PM
Closed by commit rCOMM1c5e50b949b6: [native] Memoize `SwipeableThread` (authored by atul). · Explain WhySep 8 2023, 2:36 PM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM1c5e50b949b6: [native] Memoize `SwipeableThread`.

Revision Contents
Changeset List

PathSize
native/
chat/
22 lines

Diff 30848

View Options

native/chat/swipeable-thread.react.js

Loading...