perf: improve perf of isActionOfType by limiting calls to includes #46497
Conversation
adhorodyski
changed the title
|
@aldo-expensify 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] |
adhorodyski
changed the title
muttmuure
added
the
External
muttmuure
requested review from
mkhutornyi and
allgandalf
and removed request for
mkhutornyi
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-07-30.at.10.23.09.PM.movAndroid: mWeb ChromeScreen.Recording.2024-07-30.at.10.24.28.PM.moviOS: NativeScreen.Recording.2024-07-30.at.10.34.02.PM.moviOS: mWeb SafariScreen.Recording.2024-07-30.at.10.27.58.PM.movMacOS: Chrome / SafariScreen.Recording.2024-07-30.at.10.17.37.PM.movMacOS: DesktopScreen.Recording.2024-07-30.at.10.20.33.PM.mov |
There was a problem hiding this comment.
I approve off this PR, The functionality tests well on all platforms.
For testing the function where the current changes are made isActionOfType is used to check the action type when room description is updated:
Lines 426 to 427 in d2fdb27
So for testing i used that action to check, this looks good to be shipped 🚀
|
🎯 @allgandalf, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #46519. |
src/libs/ReportActionsUtils.ts
Outdated
| // This is purely a performance optimization to limit the 'includes()' calls on Hermes | ||
| if (actionNames.length === 1) { | ||
| return actionNames[0] === actionName; | ||
| } | ||
|
|
||
| return actionNames.includes(actionName); |
There was a problem hiding this comment.
I'm unfamiliar with the includes performance issue on Hermes, but what about having done this instead:
| // This is purely a performance optimization to limit the 'includes()' calls on Hermes | |
| if (actionNames.length === 1) { | |
| return actionNames[0] === actionName; | |
| } | |
| return actionNames.includes(actionName); | |
| // This is purely a performance optimization to limit the 'includes()' calls on Hermes | |
| for (let i = 0; i < actionNames.length; i++) { | |
| if (actionNames[i] === actionName) { | |
| return true; | |
| } | |
| } | |
| return false; |
if the objective is to avoid includes wouldn't this help for the case length === 1 but also for bigger arrays?
Also, should we add some linter rules to avoid includes?
There was a problem hiding this comment.
@adhorodyski , can you please have a look at the suggestion, thanks :)
There was a problem hiding this comment.
I'm checking in to benchmark all these on Hermes once again, also trying to reproduce the exact workload of ~50 calls to this function with different payloads that Jason has to hit in order to get the initial results.
Not sure if starting an iterator for all these will be for better or for worse, will come back here with numbers so we avoid the guess work.
My bottom line i that we should default to basic equality checks as ~60% of function calls to isActionOfType are eventually just x === y and this should be instant, though it now takes 6-15ms per call.
Just stripping down these 60% should save ~300ms of compute time for these 50 calls.
There was a problem hiding this comment.
@aldo-expensify I think I have to step back with this 'includes is slow' statement, my bad! Diving deeper into this to test all the scenarios, please see the above ⬆️ comment.
There was a problem hiding this comment.
Not sure if starting an iterator for all these will be for better or for worse, will come back here with numbers so we avoid the guess work.
My bottom line i that we should default to basic equality checks as ~60% of function calls to isActionOfType are eventually just x === y and this should be instant, though it now takes 6-15ms per call.
Just stripping down these 60% should save ~300ms of compute time for these 50 calls.
I'm not sure what you mean by "starting an iterator" here... are you referring to for (let i = 0; as an iterator? or some iterator created within includes.
I wouldn't expect the for loop with an index to be any slower that what you proposed in the PR, except that it should help for bigger arrays too instead of only the ones with length. Of course this is assuming that includes is really slower than using an index to check actionNames[0] === actionName (or any other index).
There was a problem hiding this comment.
@allgandalf I'm currently involved in other initiatives, as this is purely a perf improvement are you ok with holding off for a few days? There's no rush with this change.
There was a problem hiding this comment.
yeah no worries @adhorodyski , let me know when you come up with an update
There was a problem hiding this comment.
@aldo-expensify you mentioned a liner rule to avoid includes - I don't think that'd be necessary, it really depends on the use case and I only see this being a bottleneck when called repeatedly over and over again.
Tested this out, and my take is:
- go with a
for ()loop implementation, it's no slower & indeed can also help with 2+ items passed down - deploy this atomic improvement and double-check on Jason's account to see how it impacts the execution time for an account of this size on a production environment
- expect a ~50-60% improvement as provided before
There was a problem hiding this comment.
Just pushed an updated version.
adhorodyski
force-pushed
the
fix/perf-is-action-of-type
branch
from
f1d2200
to
de7aec3
Compare
|
✋ 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/aldo-expensify in version: 9.0.18-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.18-10 🚀
|
Details
This PR limits the amount of calls to
includes()as most of the time we're callingisActionOfTypewith a singleactionNameparameter to compare the input against.In general (eg. on V8) there shouldn't be much of a difference between O(n) and O(1) here as the datasets are really small - it likely all boils down to Hermes' current implementation that runs slower (we can confirm this in real life by merging this atomic improvement).
In case of the latest Jason's trace, this function took ~400-600ms to execute as part of the taken workflow.
This PR aims to cut this combined computation time down to ~100ms providing a ~400-500ms gain for a larger account with lots of transactions.
Fixed Issues
$ #45528
PROPOSAL: N/A
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop