perf: avoid re-calculating collection key length and use key.startsWith #576
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
This PR is part of performance improvements based on Expensify/App#45528. Based on the provided trace, we see that the
isCollectionMemberKey
takes around ~270ms as a whole. For a function, which is called repeatedly in a loop, this seems too much.We couldn't reproduce the same behaviour as of the provided trace because we have accounts with less data, so we decided to benchmark the current implementation of
isCollectionMemberKey
and the improved implementation. In the latter, we propose to pass thecollectionKey.length
as a constant, which means do not re-calculate this in a loop. Which can happen whenisCollectionMemberKey
is called in a loop.Let's take a look below at the results:
The improved case outperforms the baseline by being able to perform 4k more operations per second. The two cases are just the same with just one exception and that is in the improved one, we are not accessing the
collectionKey.length
each time.The next improvement we propose is to avoid using
Str.startsWith
as it has additional checks usingtypeof
operators. We can instead usekey.startsWith(collectionKey)
which is able to perform better on large data. See the benchmarks of above improvement combined against the baseline:With both improvements combined, we get 8k more operations per second as compared to the baseline.
Related Issues
Expensify/App#45528
Automated Tests
Manual Tests
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov