[HOLD for Payment 2024-09-06] [$500] [Performance] Create Onyx connection Manager

[mountiny]

Problem

A new connection is made to Onyx every time we subscribe to a Onyx key (by using Onyx.connect(), useOnyx() or withOnyx), even if we are subscribing to the same key e.g. a collection. When the Onyx data changes, we loop through each connection ( here and here ) and perform some heavy operations in order to prepare the data to send to all the subscribers listening to that key. This approach is inefficient as we need to build the same data countless times for every subscriber, leaving to the developer to build custom solutions in their codebase if they want to optimize and reuse the connections.

Solution

Let’s reuse the connections automatically inside Onyx with the Onyx Connection Manager. This manager will take care of identifying and reuse the connections every time we have multiple subscribers to the same Onyx key with the same connect configuration. When the data changes we’ll only need to prepare the data one time and send to all the subscribers immediately instead of doing this operation for each one, saving memory, processing and reducing the overall JS overhead.

This solution requires pratically no significant changes in E/App and will benefit both Onyx.connect() and useOnyx() subscribers. Unfortunately the withOnyx() HOC connects to Onyx in a different way than the first ones and therefore they can’t be reused in the same way, so they will function normally as it is today (one connection per subscriber).

With this solution applied to E/App, the average amount of connections will be reduced in about 30% 🚀 When the entire codebase is migrated from withOnyx() to useOnyx(), the expected reduction will be around 80-90% 🤯

Before:
Opening a report: 1749 connections
Switching between reports: 4121 connections

After:
Opening a report: 1238 connections (-30%)
Opening a report (projection after migration from withOnyx() to useOnyx()): 340 connections (-80%)
Switching between reports: 2604 connections (-36%)
Switching between reports (projection after migration from withOnyx() to useOnyx()): 399 connections (-90%)

Here’s the Onyx PR and the E/App PR!

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0142380df7c5dd6bd5
• Upwork Job ID: 1823838590285150645
• Last Price Increase: 2024-08-14

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

Current assignee @mountiny is eligible for the AutoAssignerNewDotQuality assigner, not assigning anyone new.

[fabioh8010]

Hi, I'm Fábio - expert agency contributor - and I would like to work on this issue!

[codewaseem]

Also, I noticed sometimes the withOnyx() causes infinite re-renders. #47041 (comment)

[melvin-bot]

@fabioh8010, @lschurr, @mountiny Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[lschurr]

Is there an update on this issue @fabioh8010?

[fabioh8010]

@lschurr I'm going to check the reviews on the PR and open it tomorrow

[fabioh8010]

PR was opened!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0142380df7c5dd6bd5

[melvin-bot]

Current assignee @ikevin127 is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Upwork job price has been updated to $500

[ikevin127]

♻️ Will start reviewing tomorrow (Pacific), will look at the code and also thoroughly test the in-app behaviour of the implementation in different scenarios.

[lschurr]

Any update on this one @ikevin127?

[ikevin127]

♻️ Almost done testing, will resume today and get back with the results!

• FYI: I'm testing a list of previously triaged issues from Bump Onyx to 2.0.41 #42057 (comment) which was a similar onyx related PR triaged by QA

@mountiny Any idea if QA triaged this one, or we don't have an ad-hoc build for this one yet ? It would help a lot if QA will triage this as well.

[fabioh8010]

The E/App PR is ready for testing, it's including the Onyx changes.

[ikevin127]

Dropped my test results so far all 🟢 Passing, see #44987 (comment) for details.
Moving forward I think the next thing to do is have an ad-hoc build of the E/App PR and have the QA team's results to go off and triage all found issues (if any) before moving forward with merging.

[mountiny]

We have merged and deployed this PR to staging. Investigating if the was any performance regression from this PR noticed

[ikevin127]

⚠️ We just got confirmation on Slack that the Deploy Checklist: New Expensify 2024-08-26 which includes the PR of this issue was only deployed to production today in Deploy Checklist: New Expensify 2024-08-28. More context on why this happened can be found in this Slack thread and this Slack comment.

Given the context above, this issue should be on [HOLD for Payment 2024-09-6] according to today’s production deploy from Deploy Checklist: New Expensify 2024-08-28.

cc @lschurr @mountiny

[mountiny]

Updated

$500 to @ikevin127

[lschurr]

Payment summary:

• Contributor: @fabioh8010 (contractor - no payment needed)
• Reviewer: @ikevin127 $500 (paid in Upwork)

[ikevin127]

@lschurr Offer accepted, thank you!

[lschurr]

All set!