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-09-06] [$500] [Performance] Create Onyx connection Manager #47143

Closed
mountiny opened this issue Aug 9, 2024 · 22 comments
Assignees
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mountiny
Copy link
Contributor

mountiny commented Aug 9, 2024

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 OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @lschurr
@mountiny mountiny added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 9, 2024
@mountiny mountiny self-assigned this Aug 9, 2024
@mountiny mountiny added the AutoAssignerNewDotQuality Used to assign quality issues to engineers label Aug 9, 2024
Copy link

melvin-bot bot commented Aug 9, 2024

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.

Copy link

melvin-bot bot commented Aug 9, 2024

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

@fabioh8010
Copy link
Contributor

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

@codewaseem
Copy link
Contributor

codewaseem commented Aug 9, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

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

@lschurr
Copy link
Contributor

lschurr commented Aug 12, 2024

Is there an update on this issue @fabioh8010?

@melvin-bot melvin-bot bot removed the Overdue label Aug 12, 2024
@fabioh8010
Copy link
Contributor

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

@fabioh8010
Copy link
Contributor

PR was opened!

@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

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

@melvin-bot melvin-bot bot changed the title [Performance] Create Onyx connection Manager [$250] [Performance] Create Onyx connection Manager Aug 14, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

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

@mountiny mountiny changed the title [$250] [Performance] Create Onyx connection Manager [$500] [Performance] Create Onyx connection Manager Aug 14, 2024
@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 14, 2024
Copy link

melvin-bot bot commented Aug 14, 2024

Upwork job price has been updated to $500

@ikevin127
Copy link
Contributor

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

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
@lschurr
Copy link
Contributor

lschurr commented Aug 19, 2024

Any update on this one @ikevin127?

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@ikevin127
Copy link
Contributor

ikevin127 commented Aug 19, 2024

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

@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
Copy link
Contributor

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

@ikevin127
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 23, 2024
@mountiny
Copy link
Contributor Author

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

@ikevin127
Copy link
Contributor

⚠️ 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 mountiny changed the title [$500] [Performance] Create Onyx connection Manager [HOLD for Payment 2024-09-06] [$500] [Performance] Create Onyx connection Manager Sep 2, 2024
@mountiny mountiny added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Sep 2, 2024
@mountiny
Copy link
Contributor Author

mountiny commented Sep 2, 2024

Updated

$500 to @ikevin127

@lschurr
Copy link
Contributor

lschurr commented Sep 6, 2024

Payment summary:

@ikevin127
Copy link
Contributor

@lschurr Offer accepted, thank you!

@lschurr
Copy link
Contributor

lschurr commented Sep 6, 2024

All set!

@lschurr lschurr closed this as completed Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerNewDotQuality Used to assign quality issues to engineers Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Development

No branches or pull requests

5 participants