Repository navigation
[Performance] Class based HOC implementations are costly #4549
Description
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
What performance issue do we need to solve?
- inefficient React component rendering
What is the impact of this on end-users?
- app boot time
- chat switch time
List any benchmarks that show the severity of the issue
- Start the web app and be on a chat
- Open the dev console
- Go to the Performance tab
- Start capturing data
- Switch to another chat
- Wait a few seconds for loading to settle
- Stop capturing data
- Review results
- Task 1 - is processing the click and transitioning to another chat - it totals to 868ms for me (Starting on reportID: 71955477)
- A huge part of Task 1 is dedicated to unmounting components - 546ms. All the little needles under
unmountHostComponents- About ~450 of those are
withWindowDimensionsHOCs - And around 80 are
withOnyxHOCs
- About ~450 of those are
Profile data (you can load this in Chrome/Performance): https://u.pcloud.link/publink/show?code=XZwvKwXZpXUhxs7Vdr7kaco3HDxznYmcP7sX
Proposed solution (if any)
HOC usages should be gradually reduced as much as possible
They cause 1+ component for each component that uses them
I've described how to change withWindowDimensions here: #2326 (DimensionsContextProvider)
This will leave withWindowDimensions usages throughout the app unchanged, but they will at least not be class based components and would not run class component lifecycle
ReportActionItem should be updated not to be HOC based (if possible) as they are the most rendered component
They can receive window related props from the parent renderItem as well as Onyx related props
List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)
Note: These should be the same as the benchmarks collected before any changes.
This is not at all the final result, but it does show some improvement after converting the withWindowDimensions wrappings not to be class based
Platform:
Where is this issue occurring?
- Web (Affected more due to higher count of HOCs - popups, hoverable items etc...)
- iOS
- Android
- Desktop App (Affected more due to higher count of HOCs - popups, hoverable items etc...)
- Mobile Web
Version Number: 1.0.85-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Activity
MelvinBot commentedon Aug 11, 2021
Triggered auto assignment to @MonilBhavsar (
Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.kidroca commentedon Aug 11, 2021
The other large portion of execution time is taken by
sendDataToConnectioncallsIt will be the focus of a separate ticket - #4592
sendDataToConnectionshould be the easier problem to address as it isolated toOnyx.connectkidroca commentedon Aug 11, 2021
cc @marcaaron
marcaaron commentedon Aug 11, 2021
Should we maybe limit the scope of this to just refactoring
withWindowDimensionsand create a separate ticket forReportActionItem?I'd also be curious to see the effects on chat switching since we have done the test for web but not Android. I'd be curious to see what it does for us on the native side. Is it possible to do a similar trace? I don't have much experience with the hermes debugger but I thought maybe it would be possible to profile there?
kidroca commentedon Aug 11, 2021
I thought it would be possible to do this with the Hermes debugger in Flipper, but starting the profiler there seems to hang with "Initialising ..."
For Android there's another way - it's part of the development menu. It would create a file, like the one shared here, that you can load inside Chrome
I'll try it and post results
Since all of the hoverable popups should be noop on native, there would be less
withWindowDimensionscomponentsAfter analysing this more the HOCs might only be 30-50% to blame, and the rest is due to the many instances of components that use them.
Most of the
withWindowDimensionsare used by popups and modalsWhen we switch to a new chat all the components in the old chat that have
componentWillUnmountwould run itFor example
ContextMenuItemuseswillUnmountand so there will be a lot of unmounting going on as each action item would have at least one menu with multiple menu items - that seems to be what's happening in the screenshot I've shared, but originally I've only traced it towithWindowDimensionsYes the current scope is whatever we can do without massive changes and so far that seems only the
withWindowDimensionsrefactorWe had a test where all popups we removed. Perhaps for a future ticket we can make the same snapshot captured here
and see if this big bar of unmounts gets smaller with no popup components
marcaaron commentedon Aug 12, 2021
We can just try the
withWindowDimensionsidea and see what happens - doesn't seem hard to do and we can eliminate it as a potential improvement. Worst case scenario some component will have less overhead. 🤷marcaaron commentedon Aug 12, 2021
Managed to get the profiler to work on Android and adding some tips for that in this PR if it helps at all
https://github.com/Expensify/App/pull/4617/files
MonilBhavsar commentedon Aug 13, 2021
Marc, I assigned you as you are having context and working on this
MelvinBot commentedon Aug 16, 2021
Whoops! This issue is 2 days overdue. Let's get this updated quick!
9 remaining items
MelvinBot commentedon Aug 20, 2021
Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
marcaaron commentedon Aug 20, 2021
Gonna drop this to weekly for now. We are adding more metrics now which should make it easier to test theories.
kidroca commentedon Sep 3, 2021
I still think many HOCs could be bad, I'll re-capture data now that we have a new way of measuring performance (CAPTURE_METRICS)
I'll again measure chat switching (we have perf marks for those)
Both pre/post tests should happen after fresh login (storage is cleared on log out) and without scrolling to the past (which would add more data to storage and force you to pre-render more)
MelvinBot commentedon Sep 13, 2021
This issue has not been updated in over 15 days. eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
kidroca commentedon Sep 22, 2021
Run some benchmarks on Android, it seems it's almost unaffected by the
withWindowDimensionsHOCSetup
Before
After
Apart from that I've measured chat switch time, where I couldn't see any improvement
I also captured memory usage:
kidroca commentedon Sep 22, 2021
I'm closing this one
We can reopen if we find anything new