Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture #8503

Closed
36 of 38 tasks
mvtglobally opened this issue Apr 6, 2022 · 150 comments
Closed
36 of 38 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Apr 6, 2022

GitHub Project: https://github.com/orgs/Expensify/projects/35

Main New Arch PR: #13767

Context

New render engine: https://reactnative.dev/architecture/fabric-renderer
Summary of findings

  1. There are 4 main components of RN: React, JavaScript, a series of elements collectively known as the “bridge”, and the native elements/modules. Each of these components are getting complimentary upgrades.
  2. The notable React upgrades discussed are concurrent rendering, synchronous event callbacks, and priority rendering queues. Together those improvements basically allow for the UI thread to perform heavy rendering work in the background without compromising quick response times to user events such as scrolling and navigation gestures.
  3. In the existing RN architecture, the JS and native sides are essentially unaware of each other. All communications are serialized JSON messages sent asynchronously over the bridge.
  4. When JavaScript communicates with a web browser, the browser objects hold direct references to native input elements which are controlled internally by the browser. So an HTML checkbox in a browser actually internally holds a reference to a native macOS or Windows checkbox that’s managed by the browser. Similarly, JSI (JavaScript Interface) is a new piece of RN that enables direct communication between JS and the native side, because JSI internally manages direct references to C++ host objects (similar to the HTMLInput element on a browser). This means that messages sent between JS and native modules no longer need to be JSON-serialized or sent over the single bottleneck communication channel. It also means messages can be sent either synchronously or asynchronously.
  5. At a high level, the traditional RN bridge manages two tasks: computing layout via a component tree called the “shadow tree” (that shadows the React tree), and managing native modules such as the camera. In the new architecture, these are broken into two distinct pieces which facebook is calling Fabric and TurboModules, respectively.
  6. Fabric, the new UI manager, leverages JSI to create the shadow tree in C++ directly, cutting out the middle man and dramatically improving performance. It also exposes UI operations on both sides, so the native side and JS side can both directly manipulate the shadow tree without having to send serialized messages back and forth across the bridge.
  7. In the existing native module system, all the native modules need to be loaded when the app launches so that they’re ready and waiting for commands to be sent over the bridge. Turbo Modules have direct references to native modules via JSI, which gives two main benefits. First, native modules can be lazy-loaded (only when really needed), which should dramatically improve app startup time, especially on apps that use lots of different native modules. Second, once loaded the modules will be more performant because communications no longer need to be JSON-serialized and sent across the bridge.
  8. Lastly, there is a tool I don’t fully understand called CodeGen. It leverages TypeScript to generate the interface files needed by Fabric and TurboModules. This compile-time type safety means that the code counterparts from both the native and JS worlds can trust eachother without any runtime checks, which means smaller bundle sizes and faster execution.

Requirements

Migrate off setNativeProps

Migrate off findNodeHandle

Bidirectional scrolling

Dependency upgrades

Known bugs

Expensify/Expensify Issue URL:
Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1648743262264529

@melvin-bot
Copy link

melvin-bot bot commented Apr 6, 2022

Triggered auto assignment to @stitesExpensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@roryabraham
Copy link
Contributor

This can definitely be external. I went ahead and flagged it as a potential issue for the Margelo team to work on: https://expensify.slack.com/archives/C035J5C9FAP/p1649217447621559

@mrousavy
Copy link
Contributor

mrousavy commented Apr 6, 2022

Awesome, Margelo would love to work on this! We noticed that you use a lot of third party React Native libraries, so it'll be a bit difficult to get those migrated as well. Especially problematic is Reanimated, but the SWM team is working on bringing that to Fabric.

Let's coordinate internally which issue we want to prioritize :)

@stitesExpensify
Copy link
Contributor

Making this a weekly while we prioritize

@stitesExpensify stitesExpensify added Weekly KSv2 and removed Daily KSv2 labels Apr 6, 2022
@melvin-bot melvin-bot bot added the Overdue label Apr 14, 2022
@stitesExpensify
Copy link
Contributor

Moving to monthly

@melvin-bot melvin-bot bot removed the Overdue label Apr 14, 2022
@stitesExpensify stitesExpensify added Monthly KSv2 and removed Weekly KSv2 labels Apr 14, 2022
@melvin-bot melvin-bot bot added the Overdue label May 16, 2022
@stitesExpensify
Copy link
Contributor

@mrousavy just for clarity on this issue, where is it currently prioritized?

@melvin-bot melvin-bot bot removed the Overdue label May 16, 2022
@mrousavy
Copy link
Contributor

I believe currently we are not working on this issue, maybe let's discuss this in the Slack channel? We are currently working a lot with Coinbase and have very limited availability, but we expect to have some in two weeks!

@kidroca
Copy link
Contributor

kidroca commented Jun 10, 2022

I've stumbled on this ticket thanks to experimenting with Fabric and the new RN Architecture.
It turns out migrating would resolve one top paying issue here: #8349 (comment)

I think we can

I'm posting a comment to keep in touch
I'll be happy to work or take part in landing the new architecture in App

@stitesExpensify
Copy link
Contributor

Thanks for volunteering @kidroca! There is still a lot of internal conversation happening around this, so I'll post an update when I have one 😄

@melvin-bot melvin-bot bot removed the Overdue label Jun 20, 2022
@roryabraham
Copy link
Contributor

@stitesExpensify
Copy link
Contributor

No updateyet

@melvin-bot melvin-bot bot removed the Overdue label Jul 25, 2022
@melvin-bot melvin-bot bot added the Awaiting Payment Auto-added when associated PR is deployed to production label Apr 18, 2024
@melvin-bot melvin-bot bot changed the title [Tracking] [Performance] Enable the new Fabric architecture [HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture Apr 18, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 18, 2024
Copy link

melvin-bot bot commented Apr 18, 2024

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

Copy link

melvin-bot bot commented Apr 18, 2024

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

If no regressions arise, payment will be issued on 2024-04-25. 🎊

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

Copy link

melvin-bot bot commented Apr 18, 2024

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:

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

@mallenexpensify
Copy link
Contributor

@ishpaul777 , can you let me know when I should issue payment here? Lotta PRs keep auto-posting and pushing back the payment date. Thx

@ishpaul777
Copy link
Contributor

@roryabraham Can you please clarify payment date here, i think we blocked deploy for android yesterday until #40048 is fixed, does that effect the payment date

@roryabraham
Copy link
Contributor

I don't think it should, because it's an inconsistently reproducible bug in the RN Fabric renderer C++ code. I think we should pay this out on 2024-04-25, one week after the main Fabric PR went to prod (yesterday).

@mallenexpensify
Copy link
Contributor

Groovy, I'll pay on 4/25, thx

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 24, 2024
Copy link

melvin-bot bot commented Apr 25, 2024

Issue is ready for payment but no BZ is assigned. @joekaufmanexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

Copy link

melvin-bot bot commented Apr 25, 2024

Payment Summary

Upwork Job

  • Contributor: @j-piasecki is from an agency-contributor and not due payment
  • Contributor: @WoLewicki is from an agency-contributor and not due payment
  • ROLE: @ishpaul777 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@joekaufmanexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@joekaufmanexpensify
Copy link
Contributor

@mallenexpensify I see based on above comments you have been tracking payments/BZ for this one, so assigned to you. LMK if there is anything I can help with though!

@mallenexpensify
Copy link
Contributor

@ishpaul777 can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~0112754e710d14595c

@ishpaul777
Copy link
Contributor

Offer accepted😄 Thanks

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
@mallenexpensify
Copy link
Contributor

Contributor+: @ishpaul777 paid $2000 via Upwork.

Thanks! Are there any new regression tests we want for this? cc @roryabraham

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 30, 2024
@roryabraham
Copy link
Contributor

roryabraham commented May 3, 2024

No additional regression tests needed for this. Thanks for the great work everyone. This was a big one. Closing this out!

@melvin-bot melvin-bot bot removed the Overdue label May 3, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
Development

No branches or pull requests