[WIP] Replace old value after `null` merges in nested properties when batching merges/updates

[fabioh8010]

Details

Related Issues

https://github.com/Expensify/react-native-onyx/issues/615

Automated Tests

Manual Tests

Author Checklist

• [ ] I linked the correct issue in the ### Related Issues section above
• [ ] I wrote clear testing steps that cover the changes made in this PR
  • [ ] I added steps for local testing in the Tests section
  • [ ] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [ ] I included screenshots or videos for tests on all platforms
• [ ] I ran the tests on all platforms & verified they passed on:
  • [ ] Android / native
  • [ ] Android / Chrome
  • [ ] iOS / native
  • [ ] iOS / Safari
  • [ ] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop
• [ ] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [ ] I followed proper code patterns (see Reviewing the code)
  • [ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [ ] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [ ] I verified that comments were added to code that is not self explanatory
  • [ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [ ] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [ ] I followed the guidelines as stated in the Review Guidelines
• [ ] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [ ] I verified that if a function's arguments changed that all usages have also been updated correctly
• [ ] If a new component is created I verified that:
  • [ ] A similar component doesn't exist in the codebase
  • [ ] All props are defined accurately and each prop has a /** comment above it */
  • [ ] The file is named correctly
  • [ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [ ] The only data being stored in the state is data necessary for rendering and nothing else
  • [ ] If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
  • [ ] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [ ] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [ ] All JSX used for rendering exists in the render method
  • [ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [ ] If any new file was added I verified that:
  • [ ] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [ ] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• [ ] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

[fabioh8010]

Strangely the should replace the old value after a null merge in a nested property when batching updates and should replace the old value after a null merge in a nested property when batching merges tests are partially passing, which I was not expecting to happen.

Maybe the fastMerge/mergeObject original logic are partially working somehow? I need to investigate.

[fabioh8010]

My tests were "wrong", now it will output the errors in the two scenarios of both Onyx.update() and Onyx.merge()

[fabioh8010]

cc @chrispader @neil-marcellini

[marcaaron]

now we have some customizations (this very PR) that is impossible to replicate with JSON_PATCH

Can you please update the description to mention which customizations are not compatible with JSON_PATCH? I will review today, thanks!

[marcaaron]

Is it possible to reword some of this for clarity?

Changes our merging strategy (defined in applyMerge/fastMerge/mergeObject) to have two additional flags: isBatchingMergeChanges and shouldReplaceMarkedObjects. The isBatchingMergeChanges flag will be used when batching any queued merge changes, as it has a special logic to handle the replacement of old values after null merges.

Specifically, "handle the replacement of old values after null merges" didn't quite make sense to me.

The shouldReplaceMarkedObjects flag will be used when applying this batched merge changes to the existing value, using this special logic to effectively replace the old values we desire.

A part of me also finds the description a bit confusing. Maybe you can also explain what a "marked object" is in the description? Thanks!

[neil-marcellini]

@fabioh8010 please request our reviews again when it's ready

[fabioh8010]

@fabioh8010 please request our reviews again when it's ready

Sure, I started working through the comments today and preparing better explanation for you before requesting.

[fabioh8010]

Updates:

• [x] Simplified comments in general
• [x] Simplied logic of applyMerge() and created separate function to merge batch changes
• [x] Simplified and structured better the tests regarding these changes.
• [x] Addressed some of the comments.

TODO

• [ ] Add better explanation about the custom merging strategy
• [ ] Do benchmarks to evaluate performance before/after this solution, targeting SQLiteProvider.multiMerge()
• [ ] Address remaining comments not related to the items above

[neil-marcellini]

@fabioh8010 what is the latest update? How is it coming along?

[fabioh8010]

@neil-marcellini Sorry for the late, I was working on two high priority design docs and it was a bit difficult to focus on this, but now I have more capacity and will be addressing the remaining stuff today/tomorrow at most.

[fabioh8010]

Performance Analysis

For these analysis I tested on Android (the only provider that have substantial changes is the native one) and profiled Storage.multiMerge and Storage.mergeItem using the Performance API.

Baseline (main)

Test	Method	Duration (ms)
Merging 1000 records with mergeCollection()	Storage.multiMerge	83
Merging 1 record with mergeCollection()	Storage.multiMerge	4
Merging the same record 10 times with merge()	Storage.mergeItem (uses Storage.multiMerge under the hood)	3

Delta (this PR)

Test	Method	Duration (ms)
Merging 1000 records with mergeCollection()	Storage.multiMerge	118 (+42,17%)
Merging 1 record with mergeCollection()	Storage.multiMerge	37 (+89,19%)
Merging the same record 10 times with merge()	Storage.mergeItem (uses Storage.setItem under the hood)	30 (+86,67%)

---

As you can see the operations are quite more expensive now, probably due to having to use this.multiGet() and then this.multiSet() instead of JSON_PATCH.

I will try to look for another solution for the native provider, maybe I can keep the JSON_PATCH and only do additional SQL operations for the properties we need to fully replace.

[fabioh8010]

Updates:

• Investigating and experimenting a way to keep JSON_PATCH operation and then execute the object replacements with separate SQL operations
• Also investigating why a simple setItem operation is taking whole 30 ms even on main, while mergeItem takes around 3-4 ms on main. I feel that a SQL "SET" operation should have the same or even less execution time than a JSON_PATCH one. This might be because of executeAsync vs executeBatchAsync, will confirm it when I'm back.

[fabioh8010]

Just FYI that I will be OOO next week and will return on May 5th 🌴

[fabioh8010]

Updates:

• Resuming work on this one this week.

[fabioh8010]

Updates:

• Designing/working on the JSON_PATCH + separate JSON_REPLACE operations solution.

[fabioh8010]

Updates:

• I think I have a working solution for JSON_PATCH + separate JSON_REPLACE operations approach, will do a cleanup, more testing and finally the benchmarks.

[chrispader]

Updates:

• I think I have a working solution for JSON_PATCH + separate JSON_REPLACE operations approach, will do a cleanup, more testing and finally the benchmarks.

Cool! Lmk if you need any help with the Onyx storage layer or SQLite related stuff, since i'm the maintainer of react-native-nitro-sqlite i might be able to help there! 🙌🏼

[fabioh8010]

Updates:

• I think I have a working solution for JSON_PATCH + separate JSON_REPLACE operations approach, will do a cleanup, more testing and finally the benchmarks.

Cool! Lmk if you need any help with the Onyx storage layer or SQLite related stuff, since i'm the maintainer of react-native-nitro-sqlite i might be able to help there! 🙌🏼

For sure man, thanks! I need to do some cleanup before, and for now I won't merge with main yet since I don't want to pollute the benchmarks.

[fabioh8010]

Updates:

• Did the benchmarks and the logic I created to generate the JSON_REPLACE operations are causing some performance impact when merging big collections (like the Merging 1000 records with mergeCollection() case) because we need to traverse the objects again to search for the markers.
• To improve that I passed this logic to mergeObject because we can reuse the traverse object logic that already happen there to remove the performance impact.
• It seems to be working but needs more testing. Also started some discussion with @chrispader in DM.

[fabioh8010]

Updates:

• I had a call with @chrispader yesterday and we agree that the new JSON_PATCH + JSON_REPLACE seems to be the right direction now, he suggested some improvements in the code and he'll be supporting me to get to the final code ❤️
• Today I reviewed Chris suggestions (my solution branch is here which I will later merge to this one, and Chris PR is here), looks great but we have some tests failings that we need to check
• Meanwhile I'm working in update both Onyx and E/App testing branch to use latest changes (with Nitro SQLite), and will do the final benchmarks once our Onyx code is ready.

[fabioh8010]

Updates:

• @chrispader did updates in his PR and the tests are now passing.
• I will check them tomorrow and probably we'll be able to merge and continue with the main developments here.

[fabioh8010]

Updates:

• I will try to get back to this as soon as I can, I have been working on Critical Test Drive feature/issues since last week.

[fabioh8010]

Updates:

• Did the baseline measurements of E/App main with latest Onyx code (that uses NitroSQL).
• Checking Chris' PR changes.

[fabioh8010]

Updates:

• Merged Chris' changes into my JSON_PATCH solution branch.
• Fixing some TS errors and doing cleanup before proceeding with tests.

[fabioh8010]

Updates:

• Merged JSON_PATCH solution branch into this PR.
• Did manual tests with Native and Web:
  • Unit tests are passing in E/App ✅
  • Solution works in Native (SQLiteProvider) ✅
  • Solution works in Web (IDBKeyValProvider) ✅
• Did a couple of minor TS, tests and logic fixes.

Seems that Reassure is now reporting some performance degradation, I will investigate it further.

🔴 OnyxCache merge one call merging 10k keys > Duration deviation of 40.12 ms (40.74%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 Onyx merge 10k calls with heavy objects > Duration deviation of 152.49 ms (35.04%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 OnyxUtils initializeWithDefaultKeyStates one call initializing 10k heavy objects > Duration deviation of 108.72 ms (33.77%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 Onyx init one call with 10k records to init > Duration deviation of 77.55 ms (33.54%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 Onyx mergeCollection one call with 10k heavy objects > Duration deviation of 75.94 ms (28.81%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 Onyx set 10k calls with heavy objects > Duration deviation of 65.38 ms (26.47%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 Onyx multiSet one call with 10k heavy objects > Duration deviation of 49.66 ms (26.11%) exceeded the allowed range of 10.00 ms (20.00%).
🔴 Onyx update one call with 5k sets and 5k merges updates > Duration deviation of 91.49 ms (23.49%) exceeded the allowed range of 10.00 ms (20.00%).
🟢 OnyxUtils subscribeToKey one call subscribing to a whole collection of 10k heavy objects > Duration deviation of 25.56 ms (18.27%) is within the allowed range of 10.00 ms (20.00%).
🟢 Onyx setCollection one call with 10k heavy objects > Duration deviation of 36.80 ms (17.42%) is within the allowed range of 10.00 ms (20.00%).
🟢 OnyxUtils multiGet one call getting 10k heavy objects from storage > Duration deviation of 18.30 ms (13.81%) is within the allowed range of 10.00 ms (20.00%).

[fabioh8010]

Updates:

• I suspect removeNestedNullValues function is causing the majority of the performance degradations here, as it basically uses fastMerge to remove the deep null values. I believe we could instead use a separate logic for this function that would be very simple and focused on only removing the null values. I came up with something and looks like results are promising, will test it more before pushing it.

[fabioh8010]

Updates:

Reassure tests are reporting bit crazy results on CI, so I did a couple of runs locally and I'm getting this mean results consistently.

 - Onyx merge 10k calls with heavy objects [async function]: 248.5 ms → 308.3 ms (+59.7 ms, +24.0) 🔴 | 1 → 1 
 - OnyxUtils initializeWithDefaultKeyStates one call initializing 10k heavy objects [async function]: 201.8 ms → 249.1 ms (+47.3 ms, +23.4) 🔴 | 1 → 1 
 - Onyx init one call with 10k records to init [async function]: 135.3 ms → 178.2 ms (+42.9 ms, +31.7) 🔴 | 1 → 1 
 - Onyx mergeCollection one call with 10k heavy objects [async function]: 156.5 ms → 190.8 ms (+34.4 ms, +22.0) 🔴 | 1 → 1
 - OnyxCache merge one call merging 10k keys [function]: 54.4 ms → 74.6 ms (+20.2 ms, +37.1) 🔴🔴 | 1 → 1 

They are taking longer probably because of fastMerge which is more expensive now.

 - utils fastMerge one call [function]: 2.3 ms → 3.2 ms (+0.9 ms, +38.3) 🔴🔴 | 1 → 1 

On the other hand, removeNestedNullValues is being consistently being reported as more cheaper with my new implementation.

 - utils removeNestedNullValues one call [function]: 0.0 ms → 0.0 ms (-0.0 ms, -49.8) 🟢🟢 | 1 → 1 

Regarding manual profiling, here are the results.

Native

Baseline (main)

Test	Method	Duration (ms)
Merging 1000 records with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	45 / 76
Merging 1 record with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	5 / 49
Merging the same record 10 times with merge()	Storage.mergeItem / Onyx.merge	2 / 42

Delta (this PR)

Test	Method	Duration (ms)
Merging 1000 records with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	65 (+20) / 95 (+19)
Merging 1 record with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	5 / 51 (+2)
Merging the same record 10 times with merge()	Storage.mergeItem / Onyx.merge	2 / 42

Web

Baseline (main)

Test	Method	Duration (ms)
Merging 1000 records with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	97 / 108
Merging 1 record with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	6 / 7
Merging the same record 10 times with merge()	Storage.mergeItem / Onyx.merge	8 / 10

Delta (this PR)

Test	Method	Duration (ms)
Merging 1000 records with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	93 (-4) / 104 (-4)
Merging 1 record with mergeCollection()	Storage.multiMerge / Onyx.mergeCollection	6 / 7
Merging the same record 10 times with merge()	Storage.mergeItem / Onyx.merge	6 / 8 (-2)

As you can see, there's a little increase in duration when merging 1000 records with mergeCollection() on Native, which I think is expected due to our changes. For the other tests the difference is irrelevant.

For Web, all tests show irrelevant differences, which is good and also expected.

---

As next steps, I will check what else we can improve in fastMerge in order to decrease this difference reported in Reassure, but I don't expect to achieve the same results as baseline because we have new logic added.

[neil-marcellini]

Those performance hits don't look too bad to me! Maybe we have done enough optimization? It might be good to see how this works in App on some large accounts.

[fabioh8010]

Ok, I'm finishing last touches to the PR (mostly comments) and re-testing solution with mergeCollection (looks like for web it's not being applied, i'm double-checking), and will re-open this PR very soon.

[fabioh8010]

Updates:

• Fixed the case where mergeCollection was not applying the solution in Web platform, and added a unit test to cover it.
• Add additional Storage checks in the unit tests covering this solution, to make sure both data in Storage and cache are the same.
• For next and final steps, add the remaining comments and open PR to review.

Note: I will be OOO tomorrow and Friday due to holidays, but I'll be back on Monday to focus on this.