Replace _.merge function with faster alternative#11185
Replace _.merge function with faster alternative#11185Szymon20000 wants to merge 3 commits intoExpensify:mainfrom
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Szymon20000
force-pushed
the
@szymon/speedup_merge
branch
from
1c3c1c6 to
0c77d81
Compare
melvin-bot
bot
requested review from
Justicea83
and removed request for
a team
|
Merging is definitely much faster now the only thing that needs to be check ed if it's correct. |
|
@Szymon20000 Can you try the CLA check in this PR? I cant see why it should not work in the previous PR of yours... |
| +* @returns {*} | ||
| +*/ | ||
| +function cloneIfNecessary(value, optionsArgument) { | ||
| + var clone = optionsArgument && optionsArgument.clone === true; |
There was a problem hiding this comment.
Where does the .clone argument come from? Is that really needed? I don't think it's something we use today
There was a problem hiding this comment.
The whole code is mostly taken from here https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 That's why it's here. It's not used currently in onyx and can be removed/simplified.
| +*/ | ||
| +export function merge(target, source, optionsArgument) { | ||
| + var array = Array.isArray(source); | ||
| + var options = optionsArgument || {arrayMerge: defaultArrayMerge}; |
There was a problem hiding this comment.
Is this so that you could pass a custom merge function?
There was a problem hiding this comment.
The whole code is mostly taken from here https://medium.com/@lubaka.a/how-to-remove-lodash-performance-improvement-b306669ad0e1 That's why it's here
| + var options = optionsArgument || {arrayMerge: defaultArrayMerge}; | ||
| + var arrayMerge = options.arrayMerge || defaultArrayMerge; | ||
| + if (array) { | ||
| + return Array.isArray(target) ? arrayMerge(target, source, optionsArgument) : cloneIfNecessary(source, optionsArgument); |
There was a problem hiding this comment.
Can you explain this logic a little bit? I'm not sure that I understand the intention of it.
There was a problem hiding this comment.
Basically if there is no array then just clone the one from source. If there is then merge every index separately.
Not sure if merging indices is necessary here but _.merge merges them so that's why it's here. I changed only arrayMerge as it was O(n^2) the rest is copied from the website I linked above.
There was a problem hiding this comment.
Ok this makes a lot of sense to me just really odd lodash would not use this approach as well.
@Szymon20000 is there any case this method handles differently/ returns different result than lodash merge?
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
I know this is easier for your testing @Szymon20000 but just to make sure, could you make these changes (once decided about them) in the react-native-onyx repo? https://github.com/Expensify/react-native-onyx
|
I have read the CLA Document and I hereby sign the CLA |
Sure it's only for testing |
|
recheck |
|
Wohoo, passed now, thanks! 🙇 |
|
Given the Medium article, are there any other lodash methods you would say are slowing down the App and replacing them with custom function would be noticeable? I assume merge is clearly the most noticeable because of Onyx, but maybe there are other places/bottlenecks where removing lodash could be beneficial. |
I think It's always better to use built in solution for instance _.each -> array.forEach but the difference is not that big. |
tgolen
left a comment
tgolen
left a comment
There was a problem hiding this comment.
OK, I think the next step here is actually to close out this PR and make a PR directly in react-native-onyx and then we will continue doing a complete review of it over there 👍
|
Will do it tomorrow morning |
|
It looks like the problem maybe already solved by @iwiznia Will update react-native-onyx locally and see if it solves the problem. |
|
No it does not solve the problem: |
|
Closing this issue in favor of Expensify/react-native-onyx#186 |
4 participants
I find out that merge calls take a lot of time.
I don't think onyx should merge anything during the initialization as most state management solutions I know just read read state from db. Before I understand How we can get rid of those merges I used _.merge replacement and optimised it so it's linear.
Details
Fixed Issues
$ GH_LINK
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android