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

[mountiny]

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 Owner

Current 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 Owner

Current Issue Owner: @puneetlath

[melvin-bot]

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]

@mountiny Please assign this to me 👋

[mountiny]

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]

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]

✅ Triaged all the reported PR issues and summarized in #42057 (comment).
cc @mountiny

[mountiny]

Thanks, very helpful!

[melvin-bot]

@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]

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]

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

[melvin-bot]

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

[melvin-bot]

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

[mallenexpensify]

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

[mallenexpensify]

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]

$250 approved for @s77rt

[melvin-bot]

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

• [NoQA] chore: bump onyx to version 2.0.52 #44065

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

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

• @s77rt requires payment (Needs manual offer from BZ)
• @hurali97 does not require payment (Contractor)
• @ikevin127 requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@s77rt / @ikevin127] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt / @ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt / @ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt / @ikevin127] Determine if we should create a regression test for this bug.
• [@s77rt / @ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

Checklist ^ does not apply here. Not a bug

[melvin-bot]

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

• [NoQA] chore: bump onyx to version 2.0.52 #44065

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

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

• @s77rt requires payment through NewDot Manual Requests
• @hurali97 does not require payment (Contractor)
• @ikevin127 requires payment (Needs manual offer from BZ)

[melvin-bot]

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:

• [@s77rt / @ikevin127] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt / @ikevin127] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt / @ikevin127] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt / @ikevin127] Determine if we should create a regression test for this bug.
• [@s77rt / @ikevin127] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: