[HOLD for payment 2024-07-10] [MEDIUM] Investigate performance improvements of adopting React Compiler

muttmuure commented 4 months ago

The latest promising performance feature for react has just been made public: React Compiler. It enables us to automatically inject performance aids such as useMemo, React.memo, useCallback etc without explicitly needing to type it out. it goes through our code and automagically applies all performance aids in the best possible way (at least thats whats being promised).

cc @hannojg @kirillzyusko

Issue Owner

Current Issue Owner: @joekaufmanexpensify

kirillzyusko commented 4 months ago

I'm starting to work on it 👀

kirillzyusko commented 4 months ago

Did a quick experiment (in https://github.com/Expensify/App/pull/42287) and here is the outcome:

1️⃣ Healthcheck report

npx react-compiler-healthcheck produces next output:

[!WARNING] That would be good to have a support for StrictMode in place before turning on react-compiler.

2️⃣ `eslint-plugin-react-compiler`

New eslint plugin doesn't report any violation in Expensify codebase 🚀 (but I had to apply a patch for that - otherwise it crashes eslint).

3️⃣ Turning on `react-compiler` in the codebase

I've tried to enable react-compiler, but it will crash the app immediately, because of this error:

react/compiler-runtime is supported only starting from React 19 beta. I've tried to force update Expensify app to React 19, but in this case I'm also receiving a crash.

react-native-web is also not compatible with react@19 so it also throws the same exceptions, but with different wordings:

[web-server] Module not found: Error: Package path ./compiler-runtime is not exported from package /Users/kirylziusko/IdeaProjects/margelo/expensify/expensify-app-fork/node_modules/react (see exports field in /Users/kirylziusko/IdeaProjects/margelo/expensify/expensify-app-fork/node_modules/react/package.json)

and

export 'findDOMNode' (imported as 'findDOMNode') was not found in 'react-dom'

Summary

I think the best thing here would be to wait until react-native and react-native-web starts to support react@19 and after that we can enable react-compiler.

In a meantime it would be good to focus on supporting StrictMode in the app.

kirillzyusko commented 4 months ago

I managed to run react-compiler with React 18:

Memo ✨ === memoized component by react-compiler

Right now it works only on web (Memo ✨ is shown on web only), but I'm trying to make it running on mobile too 🚀

kirillzyusko commented 4 months ago

Right now it works only on web (Memo ✨ is shown on web only), but I'm trying to make it running on mobile too 🚀

I investigated further. Turns out that compiler actually works (it injects c hook into optimized components with an actual payload). The only difference is that react-native@0.73 render is not adding necessary label to optimized nodes, so they are not getting highlighted in react-devtools- starting from react-native@0.74 everything will be shown in react-devtools:

Template app 0.73	Template app 0.74	Expensify app 0.73.4

I also assembled both apps (from main and feat/react-compiler branches) and run perf e2e tests (60 runs, equals to what we have on CI):

❇️ Performance comparison results: ➡️ Significant changes to duration

Open Chat Finder Page TTI: 2758.066 ms → 2639.980 ms (-118.086 ms, -4.3%)

➡️ Meaningless changes to duration

App start nativeLaunch: 91.750 ms → 93.517 ms (+1.767 ms, +1.9%)
App start nativeLaunchEnd_To_appCreationStart: 91.737 ms → 90.877 ms (-0.860 ms, -0.9%)
App start appCreation: 108.133 ms → 111.817 ms (+3.683 ms, +3.4%)
App start appCreationEnd_To_contentAppeared: 1116.733 ms → 1108.483 ms (-8.250 ms, -0.7%)
App start contentAppeared_To_screenTTI: 2324.047 ms → 2343.516 ms (+19.469 ms, +0.8%)
App start runJsBundle: 863.983 ms → 849.467 ms (-14.517 ms, -1.7%)
App start TTI: 3745.847 ms → 3750.249 ms (+4.403 ms, ±0.0%)
App start regularAppStart: 0.060 ms → 0.061 ms (+0.000 ms, ±0.0%)
Load Search Options: 403.670 ms → 415.041 ms (+11.370 ms, +2.8%)
Chat opening: 84.929 ms → 87.507 ms (+2.578 ms, +3.0%)
Chat TTI: 1522.559 ms → 1513.895 ms (-8.664 ms, -0.6%)

I also tested app and for me it looks stable - I haven't caught any unexpected crashes or something like that (though additional QA check would be necessary I guess if we decide to roll it out to prod/merge into main).

adhorodyski commented 4 months ago

I'm thinking it'd be nice to also create a backlog of items for making the other ~40% of components to follow the rules of react better so they also compile. We might see some more gains, it's likely that our high level components are noncompliant yet.

cc @mountiny

mountiny commented 4 months ago

Yeah, I think its something we should capture in the checklist/ linter rules as much as possible

adhorodyski commented 4 months ago

@kirillzyusko I'm not yet onboarded to how this works, will we have to manually see what compiles/what doesn't? Is there a better way?

kirillzyusko commented 4 months ago

I'm not yet onboarded to how this works, will we have to manually see what compiles/what doesn't? Is there a better way?

@adhorodyski we can run npx react-compiler-healthcheck and using changes from https://github.com/facebook/react/pull/29080 we can see which files can be compiled and which are not (and it'll print a reason why fiel can not be compiled).

I haven't patched react-compiler-healthcheck to see why (and which) files can not be compiled, but I'll do it for sure later 👀

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @hayata-suenaga, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

hayata-suenaga commented 4 months ago

yaaay the new react compiler! I think @mountiny might be better equipped to handle this. I'll un assign myself but let me know if you need any help from my side 😄

mountiny commented 4 months ago

Most of the expert team is ooo so we can continue discussing this on Monday

This is a bit hard to prioritize but given we are updating to RN 9.74 and working on enabling the concurrent mode here, what would you say are the best next steps here @kirillzyusko, should we wait for any of those or is there something meaningful we can do now already with react-compiler

I assume we could already work on implementing the best practices

kirillzyusko commented 4 months ago

@mountiny from what I understand - we already can use react-compiler and get benefits from it:

RN 0.74 will add "Memo ✨" badge to auto-memoized components (in devtools)
concurrent mode is not a mandatory requirement for a react-compiler (though support for StrictMode is desirable, but I quickly went through the app with enabled StrictMode and haven't found any serious violations (especially in Expensify codebase - I had some reports in node_modules, but not in Expensify codebase)).

Again, if react-compiler can not optimize anything -> it will skip optimizations for a particular component. Moreover if react-compiler optimizes anything and it break current codebase -> we always can force disable an optimization via "use no memo" directive.

So in my opinion it's pretty safe to enable react-compiler and see how it behaves. Let me know what do you think 🙌

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

hannojg commented 3 months ago

@kirillzyusko whats the status of our PR for using react-compiler ?

hannojg commented 3 months ago

okay, sorry, I overlooked the "reviewing" label - seems like its being worked on - all good!

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @joekaufmanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

mountiny commented 3 months ago

PR is very close to be merged.

joekaufmanexpensify commented 3 months ago

Sounds good!

mountiny commented 3 months ago

Merged!

melvin-bot[bot] commented 3 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

joekaufmanexpensify commented 3 months ago

PR is on staging

melvin-bot[bot] commented 3 months ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 3 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.3-7 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/42287

If no regressions arise, payment will be issued on 2024-07-10. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@Ollyws requires payment through NewDot Manual Requests
@kirillzyusko does not require payment (Contractor)

melvin-bot[bot] commented 3 months ago

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[x] [@Ollyws] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
[x] [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

joekaufmanexpensify commented 2 months ago

Performance improvement, no checklist needed here.

joekaufmanexpensify commented 2 months ago

Only payment here is $250 to @Ollyws for C+ via NewDot.

joekaufmanexpensify commented 2 months ago

@Ollyws you're all set to request your payment via NewDot. I'm gonna close this one out for now as nothing else is needed besides that. Thanks everyone!

Ollyws commented 2 months ago

Requested in ND.

JmillsExpensify commented 2 months ago

$250 approved for @Ollyws

Expensify / App