-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
perf: remove sorting from findLastAccessedReport
#44948
perf: remove sorting from findLastAccessedReport
#44948
Conversation
src/libs/ReportUtils.ts
Outdated
return adminReport ?? sortedReports.at(-1); | ||
// We are getting the last read report from the metadata of the report. | ||
const lastRead = lodashMaxBy(filteredReports, (a) => | ||
new Date(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${a?.reportID}`]?.lastVisitTime ?? a?.lastReadTime ?? '').valueOf(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are 100% certain that the lastReadTime
format will always be the same (2024-07-08 08:55:01.665
), we can avoid parsing it into the Date
object, as it is slow.
cc @mountiny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, if there is a chance that the format might change, it is safer to leave it as it is.
This change will probably have a minor performance boost, as the process is now O(n)
and will only be called once, and only if the reportID
is not in the URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructing Date shouldn't have a big impact on the performance 🤔 Can you verify if it really affects the speed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now the difference is minor, but when we sorted the whole array, it had a significant impact overall.
Comparison | main |
branch |
---|---|---|
Date |
0.51 ms | 0.12 ms |
string |
0.23 ms | 0.09 ms |
Less time: | 55.41% | 25.37% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's verify if the format is always the same, it might be worth not using Date. Thank you for providing the benchmark 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the format should be the same always, but I think we can make that change in a follow up PR just to make sure we can easily catch any regressions
findLastAccessedReport
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari *web.movMacOS: Desktop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tests well in all cases! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kosmydel One comment only to rename the method and you got ocnfclits, please ping me in slack when you resolve so we can merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.6-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.6-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Cherry-picked to staging by https://github.com/Julesssss in version: 9.0.6-1 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
const filteredReports = reports.filter( | ||
(report) => !!report?.reportID && !!(reportMetadata?.[`${ONYXKEYS.COLLECTION.REPORT_METADATA}${report.reportID}`]?.lastVisitTime ?? report?.lastReadTime), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have added a filter to exclude the left chat reports as well. This later caused #46085
Details
This is the first PR of
ReportScreen
performance improvement.In this PR we are improving the
findLastAccessedReport
function improvement, by removing the sorting of the reports. Instead, we perform filtering first and then search for the latest report.This change improves the
findLastAccessedReport
execution time: 0.51ms -> 0.12ms (76.56% less time).Fixed Issues
$ #44925
PROPOSAL:
Tests
Note
When testing, sometimes the
lastVisitedPath
might be loaded and thefindLastAccessedReport
function might not be called. To avoid this, you can addreturn undefined;
at the beggining of theNavigationRoot
->initialState
function.Web:
Last accessed report
https://staging.new.expensify.com/
and remove thereportID
from the URL)New account
Other platforms
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop