search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.09k stars 2.59k forks source link

[HOLD for payment 2024-07-02] [$250] [HOLD for Payment 2024-06-21] LOW: [Performance] Improve the performance of getCachedCollection #41895

Closed mountiny closed 4 days ago

mountiny commented 1 month ago

Slack here

Problem

We have many calls to keysChanged function throughout the session and on an average it will be around ~800 ms. That’s a lot for a function which is called this many times. From the trace, this function is called roughly around 15 times.

Inside of this function we have getCachedCollection , which first calls Array.from on the Set then filter it out and then reduce the returned value.

Solution

We can improve this by doing only 1 loop directly on Set as it gives us forEach.

cc @hurali97

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0145b3587af847aed1
  • Upwork Job ID: 1804230619308310738
  • Last Price Increase: 2024-06-21
Issue OwnerCurrent Issue Owner: @puneetlath
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @puneetlath (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.

hurali97 commented 1 month ago

@mountiny Please assign this to me 👋

mountiny commented 1 month ago

Onyx PR merged, asking Ali to raise App onyx bump PR https://expensify.slack.com/archives/C05LX9D6E07/p1715605661167679?thread_ts=1715178705.054759&cid=C05LX9D6E07

mountiny commented 1 month ago

PR has gone through Applause testing. @ikevin127 volunteered to help triaging the found bugs and so we identify what are real bugs stemming from the onyx bump quicker

ikevin127 commented 1 month ago

Overdue notes: I'm next in line for triaging the issues found and posted by QA in issue https://github.com/Expensify/App/pull/42057. Will try and come up with an update by EOD (PST).

ikevin127 commented 1 month ago

Triaged half of the reported issues and summarized in https://github.com/Expensify/App/pull/42057#issuecomment-2118617952.

Will finish up the other half of the list tomorrow!

ikevin127 commented 1 month ago

✅ Triaged all the reported PR issues and summarized in https://github.com/Expensify/App/pull/42057#issuecomment-2118617952. cc @mountiny

mountiny commented 1 month ago

Thanks, very helpful!

melvin-bot[bot] commented 1 month ago

@puneetlath @marcaaron @mountiny @hurali97 @ikevin127 this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

mountiny commented 1 month ago

Merged 🎉

melvin-bot[bot] commented 4 weeks ago

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

melvin-bot[bot] commented 4 weeks ago

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

If no regressions arise, payment will be issued on 2024-06-05. :confetti_ball:

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

melvin-bot[bot] commented 4 weeks ago

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

ikevin127 commented 3 weeks ago

This was version bump of the react-native-onyx library used within E/App.

No regression test needed as this was version bump of the react-native-onyx library used within E/App.

melvin-bot[bot] commented 3 weeks ago

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

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

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

melvin-bot[bot] commented 3 weeks ago

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

ikevin127 commented 3 weeks ago

Callback for the checklist: https://github.com/Expensify/App/issues/41895#issuecomment-2140877580.

mountiny commented 3 weeks ago

We have had to revert the onyx bump so we are going to hold with the payment too until the next try

mountiny commented 2 weeks ago

Still working hard on resolving issues found by Applause

puneetlath commented 2 weeks ago

Just to make sure I understand, the fix for this is in https://github.com/Expensify/App/pull/42772 is that right?

mountiny commented 2 weeks ago

yes, this PR is included in the bump https://github.com/Expensify/react-native-onyx/pull/546

melvin-bot[bot] commented 1 week 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.

ikevin127 commented 1 week ago

Wasn't sure if the payment is going to be handled here or in the other issue that's attached to the PRs Fixed Issues:

therefore I posted there https://github.com/Expensify/App/issues/41531#issuecomment-2170593766.

cc @puneetlath

@srikarparsi Changed the HOLD date and assigned Bug label for issue:

so it looks like the payment will be handled there (hopefully) since the issue is closed.

mountiny commented 6 days ago

This was a large PR and big effort, with multiple Onyx PRs and multiple rounds of testing of the App PR

I suggest $1000 to @ikevin127 for their exemplary work on the onyx bump, testing, reviewing and bringing clarity into regressions raised from Applause helping Chris to get this PR over the finish line soon.

$250 to @s77rt since they have entered this task at later phase, also very helpful though

mountiny commented 6 days ago

I think @puneetlath is assigned as BZ here, let me know if not. Thanks!

ikevin127 commented 5 days ago

Can somebody please add Daily label here and reassign BZ since @puneetlath will not be available for most of today (Slack status: ⛔️ until Today at 21:30) ?

melvin-bot[bot] commented 4 days ago

Job added to Upwork: https://www.upwork.com/jobs/~0145b3587af847aed1

melvin-bot[bot] commented 4 days ago

Current assignees @s77rt and @ikevin127 are eligible for the External assigner, not assigning anyone new.

mallenexpensify commented 4 days ago

@ikevin127 can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~0145b3587af847aed1

mallenexpensify commented 4 days ago

Contributor: @ikevin127 paid $1000 via Upwork (per @mountiny 's comment above) Contributor+: @s77rt due $250 via NewDot (also in Vit's comment)

Doesn't look like this needs a regression test, comment/reopen if anyone disagrees.

JmillsExpensify commented 3 days ago

$250 approved for @s77rt

melvin-bot[bot] commented 21 hours ago

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

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

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

melvin-bot[bot] commented 21 hours ago

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