[Tracking] [Performance] Margelo/Expensify - Performance Improvements

[trjExpensify]

Top Priorities

• [HOLD for payment 2023-01-19] [Performance] Use react-native-quick-sqlite instead of asyncStorage #10793 - @Szymon20000 @roryabraham
• [HOLD for payment 2023-01-09] [Tracking] Add performance CI tests #11711 @hannojg @AndrewGable
• [Tracking][Image][Polish] Image-related feature request and improvements #10894
• Image: support HTTP properties for 'source' objects necolas/react-native-web#1019
• [HOLD for payment][Bug] After typing even a mildly long comment in the Android compose box, the whole app starts to hang #11321 @Beamanator @hannojg

Other high-value items

• [$4000] Logo shifts to bottom (Bootsplash) when launched - reported by Ekram Khan #8236
• [$4000] [iOS] - Page freezes when creating New Group and selecting 8 members #9809
• [$250] Safari mWeb - Keyboard won't open for some inputs (New Chat, Change Password, New Room) #10414
• And a bunch of crashes we haven’t resolved yet.
• Add better performance metrics to Grafana #11997

Done

• [Performance] Optimise navigation  #10789 - @Szymon20000
• Optimize the left-hand-nav to only render options when they need to be visible #10800
• [No QA] Upgrade RN to 0.70.4 (aka 0.70.1) #10860
• [Performance] md5.js takes a lot of time #10794 - @Szymon20000 @aldo-expensify
• Replace _.merge function with faster alternative #11185 - @Szymon20000

[Szymon20000]

I would add:

• CI Tests (reassure, startup time)
• SideBarLinks
• HeaderView Doesn't use onyx context context (I think it's also the case with SideBarLinks) It means onyx will send them another copy od personalDetails as far as I know

[trjExpensify]

Awesome. Added those. Presumably they don't have issues to link yet, right? Outside of SideBarLinks, as @tgolen is working on PRs for that.

P.S - I can co-assign you this tracker now that you've commented. GH wasn't allowing me to do that yesterday.

[roryabraham]

Upgrade RN fork to 0.70

Finished this last week

[roryabraham]

@tgolen @Szymon20000 I know a lot of progress has been made on the LHN improvements, is there more to do here or should we check that off?

[tgolen]

I have a bunch of random cleanup items, but I feel pretty confident to check it off for now.

[roryabraham]

Update:

• [Performance] md5.js takes a lot of time #10794 is done
• The value of [HOLD for payment 2023-01-19] [Performance] Use react-native-quick-sqlite instead of asyncStorage #10793 is in question/unclear. I think we'll likely want to continue investigating this, but there's a lot of context in this thread
• dev: performance regression tests #11327 progress seems to have stalled, waiting for a summary of the CI stability issues from @hannojg
• Replace _.merge function with faster alternative – PR merged yesterday and published to npm in react-native-onyx@1.0.19. However, we need to bump the Onyx version in App to see this change take effect.
• Open and close the same chat again and again on Android native and it will gradually slow down for no reason – Last we heard, this was no longer reproducible, so I'm checking it off.
• HeaderView Doesn't use onyx context context (I think it's also the case with SideBarLinks) It means onyx will send them another copy of personalDetails as far as I know - nobody is assigned to this one, but there have been a few investigations into more low-level Onyx improvements, particularly by @marcaaron. I like the idea of trying to dynamically generate Onyx contexts within withOnyx so that we can use the context API for all subscribers universally.

[marcaaron]

@roryabraham In my PR to start using the modern "drawer" navigator I refactor the HeaderView a little so that the props are already available and passed via the ReportScreen - just minimizes a connection or two. I think a context based solution would be interesting though but had trouble thinking of how it would work.

[roryabraham]

In my #11550 I refactor the HeaderView a little so that the props are already available and passed via the ReportScreen - just minimizes a connection or two.

Yeah, we could probably extract that change out of the drawer PR because it's not directly related AFAICT?

[marcaaron]

Could do that yep

[hannojg]

I'd add this PR as well:

#11684

The reason for investigation was this described issue: https://expensify.slack.com/archives/C035J5C9FAP/p1664523169910029?thread_ts=1664522847.768359&cid=C035J5C9FAP

[roryabraham]

Put up this PR to glean the performance benefits from recent upgrades made in Onyx.

[roryabraham]

Removing #4656 and instead replacing it with #11997, which will provide us with clear metrics that we can point to when we want to know if we need to improve. The deliverables there are more clear and should help align us on how to make performance improvements.

[JmillsExpensify]

Added the issue for the comment box freezing the app following quick typing.

[roryabraham]

• [Image][Performance] Better caching for iOS images #10792 – on HOLD
• [HOLD for payment 2023-01-19] [Performance] Use react-native-quick-sqlite instead of asyncStorage #10793 – @Szymon20000 will be meeting with the SQLite team sometime this week to investigate potential inefficiencies in the underlying SQLite implementation of quick-sqlite
• [HOLD for payment 2023-01-09] [Tracking] Add performance CI tests #11711 – @AndrewGable and @hannojg could probably say more, but my summary is that the original E2E performance test suite worked well locally, but due to use of emulators in CI machines, was too variable/unreliable to be useful. Emulators had unpredictable and substantial variance in performance. Even with the larger GitHub Actions runners, performance of emulators was unreliable. Now @AndrewGable is working on setting up AWS Device Farm w/ real cloud devices to run E2E performance tests and @hannojg is migrating the test suite to Appium to interface with AWS Device Farm. Please correct me if any of that was inaccurate.
• Add better performance metrics to Grafana #11997 – No update really. We would need an internal infra volunteer to migrate graphs.expensify.com to make it public, so it's possible we'll use some other service to create a separate set of public graphs. Maybe host w/ GitHub Pages for simplicity
• [HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture #8503 – We've been making steady progress on this. It's also worth noting that some recent benchmarks have raised some concerns about the performance of Fabric. The only example I've seen of a Fabric-enabled app is rn-tester, and it's palpably faster and more responsive with Fabric enabled. We will have to see if that's really the case for our app. We'll still do this migration at some point, but it's possible that more work is required in the upstream to get better performance "out of the box" on Fabric. Long-term, when React 18 is enabled in react-native-web, that should open up new opportunities for better performance as well, and we won't be able to do that w/o Fabric enabled.
• [HOLD for payment][Bug] After typing even a mildly long comment in the Android compose box, the whole app starts to hang #11321 – This PR has been in review for 3 weeks, let's work to get that one merged this week!

[JmillsExpensify]

Updated the tracking issue to try and capture the latest convos!

[roryabraham]

• [HOLD for payment 2023-01-19] [Performance] Use react-native-quick-sqlite instead of asyncStorage #10793 – latest discussion with @Szymon20000 here
• [HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture #8503 – progress here seems stalled. Maybe we should pass this off so that @hannojg can focus on:
  • [HOLD for payment 2023-01-09] [Tracking] Add performance CI tests #11711
  • [HOLD for payment][Bug] After typing even a mildly long comment in the Android compose box, the whole app starts to hang #11321
  • Add better performance metrics to Grafana #11997
• Let's see if we can get @thomas-coldwell engaged in:
  • [Tracking][Image][Polish] Image-related feature request and improvements #10894
  • [Image][Performance] Better caching for iOS images #10792

For necolas/react-native-web#1019, maybe we can get @kidroca working on that piece?

Also @janicduplessis can we work on upstreaming the bidirectional scrolling features in FlatList?

[JmillsExpensify]

Most of what's written above is still valid this week. I also also linked the tracking issue for KeyboardAvoidingView, as we have about 9 bugs there that have gone stale over the last week.

[roryabraham]

• @janicduplessis has been making progress getting VirtualizedList code merged upstream
• @Szymon20000 has been mostly focused on [HOLD for payment 2023-01-19] [Performance] Use react-native-quick-sqlite instead of asyncStorage #10793, reviews underway
• @hannojg has a few open PRs up for performance improvements, but the Fabric upgrade has been passed off. @hannojg has a fair amount assigned to him, so let's make sure we prioritize getting any open PRs over the finish line.

[roryabraham]

Removing #8503 because it's being handled by SWM now

[roryabraham]

Removing #10273 because it was primarily done by @tgolen, not Margelo

[roryabraham]

Chatted with @JmillsExpensify – this issue is prettymuch serving the same purpose as Margelo's regular slack updates here. Rather than maintaining these multiple sources of truth, I'm actually going to just close this issue for now.