Replace old value after `null` merges in nested properties when batching merges/updates #620

fabioh8010 · 2025-03-03T21:38:46Z

Details

#615 (comment)

This PR:

Applies first part of the solution described in the proposal:

Change Onyx.merge()/Onyx.update() batching logic (applyMerge/fastMerge/mergeObject) to replace the current value of a nested property after a null change in it, so the subsequent updates of that batch in this nested property can fully reset its data.
Removes JSON_PATCH usage from native provider. Onyx will be now always responsible for handling the merging strategy, since now we have some customizations (this very PR) that is impossible to replicate with JSON_PATCH.
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. 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.

Related Issues

#615

Automated Tests

Unit tests were added to cover these changes.

Manual Tests

Same as Expensify/App#55199.

Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2025-03-25.at.22.13.11-compressed.mov

Android: mWeb Chrome

I'm having problems with my emulators when opening the Chrome app (they crash instantly), so I couldn't record videos for this platform.

iOS: Native

Screen.Recording.2025-03-25.at.22.29.49-compressed.mov

iOS: mWeb Safari

Screen.Recording.2025-03-25.at.22.34.02-compressed.mov

MacOS: Chrome / Safari

Screen.Recording.2025-03-25.at.22.36.15-compressed.mov

Screen.Recording.2025-03-25.at.22.37.28-compressed.mov

MacOS: Desktop

Screen.Recording.2025-03-25.at.22.43.39-compressed.mov

fabioh8010 · 2025-03-03T21:41:58Z

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 · 2025-03-06T19:12:23Z

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

…icable

fabioh8010 · 2025-03-30T18:51:34Z

cc @chrispader @neil-marcellini

lakchote

Left some comments

lib/Onyx.ts

lib/storage/providers/SQLiteProvider.ts

lakchote · 2025-03-31T11:41:55Z

lib/utils.ts

+                // To achieve this, we first mark these nested objects with an internal flag. With the desired objects
+                // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed
+                // to effectively replace them in the next condition.
+                if (isBatchingMergeChanges && targetValue === null) {


Could we do something like this?

Suggested change

if (isBatchingMergeChanges && targetValue === null) {

if (isBatchingMergeChanges && !targetValue) {

We should explicitly check for null. undefined values are not supposed to delete a key. Effectively, they should never be in store anyway, but i think making this explicit makes more sense

We did that previously, but actually for this logic, we might want to rethink that.

If we merge a nested object with an existing key, we would expect undefined values to be set, rather than discarded.

So in this case i would still explicitly check for if (targetValue == null) (which includes undefined values)

lib/utils.ts

chrispader

LGTM! I added a few comments around the SQLite part and tests, but this has a GO from me!

lib/utils.ts

chrispader · 2025-03-31T12:39:54Z

lib/utils.ts

+                // To achieve this, we first mark these nested objects with an internal flag. With the desired objects
+                // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed
+                // to effectively replace them in the next condition.
+                if (isBatchingMergeChanges && targetValue === null) {


We should explicitly check for null. undefined values are not supposed to delete a key. Effectively, they should never be in store anyway, but i think making this explicit makes more sense

chrispader · 2025-03-31T12:53:07Z

lib/utils.ts

+                // To achieve this, we first mark these nested objects with an internal flag. With the desired objects
+                // marked, when calling this method again with "shouldReplaceMarkedObjects" set to true we can proceed
+                // to effectively replace them in the next condition.
+                if (isBatchingMergeChanges && targetValue === null) {


We did that previously, but actually for this logic, we might want to rethink that.

If we merge a nested object with an existing key, we would expect undefined values to be set, rather than discarded.

So in this case i would still explicitly check for if (targetValue == null) (which includes undefined values)

chrispader · 2025-03-31T13:51:47Z

lib/storage/providers/SQLiteProvider.ts

+        return this.multiGet(nonNullishPairsKeys).then((storagePairs) => {
+            // multiGet() is not guaranteed to return the data in the same order we asked with "nonNullishPairsKeys",
+            // so we use a map to associate keys to their existing values correctly.
+            const existingMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < storagePairs.length; i++) {
+                existingMap.set(storagePairs[i][0], storagePairs[i][1]);
+            }
+
+            const newPairs: KeyValuePairList = [];
+
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < nonNullishPairs.length; i++) {
+                const key = nonNullishPairs[i][0];
+                const newValue = nonNullishPairs[i][1];
+
+                const existingValue = existingMap.get(key) ?? {};
+
+                const mergedValue = utils.fastMerge(existingValue, newValue, true, false, true);
+
+                newPairs.push([key, mergedValue]);
+            }
+
+            return this.multiSet(newPairs);
+        });


I'm really not sure about the performance implications of this change on native.

I know it is hard to have SQLite handle merges with through ON CONFLICT DO UPDATE and JSON_PATCH, but this was supposed to add significant performance gains, since we can basically offload all of the merging (at least with the existing values in store) to the low-level C++ implementation of SQLite. Ideally we would want to let SQLite handle as much of merging/batching as possible.

I see though, that we already had a lot of exceptions to this before and that we still always needed to "pre-merge" in order to broadcast the update, so i think it should be fine for now. In a bigger re-design we could tackle and improve all of this, to make sure that each platform is facilitated to it's fullest.

Still, do we have any benchmarks around how this affects performance of simple Onyx.merge operations?

Yes I think it would be a good idea to measure performance in the app before and after this change to make sure it doesn't cause a large performance hit.

+1 but I am less convinced that this is "fine for now". It feels like we are undoing work that we had a good reason to do at some point. I trust that we are moving in a good direction, but would rather let some benchmarks do the talking.

tests/unit/fastMergeTest.ts

neil-marcellini

I agree with the changes others suggested. I would like to see the tests cleaned up a little and agree with Chris that we should measure the performance impact of this.

I'm also going to request a review from @marcaaron now that he is back and because he has a good perspective about how Onyx should work.

lib/Onyx.ts

tests/unit/onyxTest.ts

neil-marcellini · 2025-04-02T18:46:01Z

lib/storage/providers/SQLiteProvider.ts

+        return this.multiGet(nonNullishPairsKeys).then((storagePairs) => {
+            // multiGet() is not guaranteed to return the data in the same order we asked with "nonNullishPairsKeys",
+            // so we use a map to associate keys to their existing values correctly.
+            const existingMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < storagePairs.length; i++) {
+                existingMap.set(storagePairs[i][0], storagePairs[i][1]);
+            }
+
+            const newPairs: KeyValuePairList = [];
+
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < nonNullishPairs.length; i++) {
+                const key = nonNullishPairs[i][0];
+                const newValue = nonNullishPairs[i][1];
+
+                const existingValue = existingMap.get(key) ?? {};
+
+                const mergedValue = utils.fastMerge(existingValue, newValue, true, false, true);
+
+                newPairs.push([key, mergedValue]);
+            }
+
+            return this.multiSet(newPairs);
+        });


Yes I think it would be a good idea to measure performance in the app before and after this change to make sure it doesn't cause a large performance hit.

marcaaron · 2025-04-02T21:07:41Z

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 · 2025-04-02T21:14:34Z

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!

marcaaron

This is generally looking good. Thanks for the work on it @fabioh8010 @chrispader. My main feedback (that @neil-marcellini already called out) would be to make sure that JSON_PATCH in the native provider is something we can confidently get rid of. If so, then 👍.

marcaaron · 2025-04-02T21:26:18Z

lib/Onyx.ts

@@ -307,8 +307,10 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
        }

        try {
-            // We first only merge the changes, so we can provide these to the native implementation (SQLite uses only delta changes in "JSON_PATCH" to merge)


A part of me is confused about why we are getting rid of JSON_PATCH. Another part believes that this was a benefit since when we do this we end up passing a smaller delta change instead of a potentially large object and therefore it should be more efficient (hand wave there as I do not entirely recall anymore).

lib/Onyx.ts

marcaaron · 2025-04-02T21:33:36Z

lib/Onyx.ts

@@ -346,11 +348,12 @@ function merge<TKey extends OnyxKey>(key: TKey, changes: OnyxMergeInput<TKey>):
                return Promise.resolve();
            }

-            // For providers that can't handle delta changes, we need to merge the batched changes with the existing value beforehand.


flagging this since I still have questions about JSON_PATCH before we remove it...

lib/Onyx.ts

lib/OnyxUtils.ts

marcaaron · 2025-04-02T21:50:49Z

lib/storage/providers/SQLiteProvider.ts

+        return this.multiGet(nonNullishPairsKeys).then((storagePairs) => {
+            // multiGet() is not guaranteed to return the data in the same order we asked with "nonNullishPairsKeys",
+            // so we use a map to associate keys to their existing values correctly.
+            const existingMap = new Map<OnyxKey, OnyxValue<OnyxKey>>();
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < storagePairs.length; i++) {
+                existingMap.set(storagePairs[i][0], storagePairs[i][1]);
+            }
+
+            const newPairs: KeyValuePairList = [];
+
+            // eslint-disable-next-line @typescript-eslint/prefer-for-of
+            for (let i = 0; i < nonNullishPairs.length; i++) {
+                const key = nonNullishPairs[i][0];
+                const newValue = nonNullishPairs[i][1];
+
+                const existingValue = existingMap.get(key) ?? {};
+
+                const mergedValue = utils.fastMerge(existingValue, newValue, true, false, true);
+
+                newPairs.push([key, mergedValue]);
+            }
+
+            return this.multiSet(newPairs);
+        });


+1 but I am less convinced that this is "fine for now". It feels like we are undoing work that we had a good reason to do at some point. I trust that we are moving in a good direction, but would rather let some benchmarks do the talking.

neil-marcellini · 2025-04-07T18:11:53Z

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

fabioh8010 · 2025-04-07T18:29:51Z

@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.

… batchMergeChanges() function

fabioh8010 · 2025-04-13T18:43:18Z

Updates:

Simplified comments in general
Simplied logic of applyMerge() and created separate function to merge batch changes
Simplified and structured better the tests regarding these changes.
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 · 2025-04-21T19:25:25Z

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

fabioh8010 · 2025-04-22T07:47:26Z

@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 · 2025-04-23T10:11:55Z

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 · 2025-04-25T20:43:50Z

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 · 2025-04-25T20:44:13Z

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

fabioh8010 · 2025-05-05T19:29:39Z

Updates:

Resuming work on this one this week.

fabioh8010 · 2025-05-06T17:29:44Z

Updates:

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

fabioh8010 · 2025-05-07T11:32:26Z

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 · 2025-05-08T10:11:35Z

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 · 2025-05-09T13:50:41Z

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 · 2025-05-12T19:29:39Z

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 · 2025-05-16T18:33:47Z

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 · 2025-05-21T17:46:42Z

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 · 2025-05-27T20:03:57Z

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 added 2 commits March 3, 2025 19:07

Add tests for replacing old value after top-level null merges

ebfad39

Add tests for replacing old value after nested property null merges

cf70ee0

fabioh8010 mentioned this pull request Mar 3, 2025

Incorrect result for Onyx update with delete and object property delete #615

Open

Fix tests

2eca1c6

fabioh8010 added 7 commits March 10, 2025 18:11

Use a temporary marker during merging to replace old values when appl…

4e61d18

…icable

Change mergeItem() to not use delta changes anymore

404af80

Change multiMerge() to not use JSON_PATCH anymore

2d1ceb1

Merge branch 'main' into bugfix/615-solution2

34bb448

Add comments

a619757

Add tests to applyMerge()

aa28a4d

Improve and fix native multiMerge logic

934e768

fabioh8010 changed the title ~~[WIP] Replace old value after null merges in nested properties when batching merges/updates~~ Replace old value after null merges in nested properties when batching merges/updates Mar 30, 2025

fabioh8010 marked this pull request as ready for review March 30, 2025 18:51

fabioh8010 requested a review from a team as a code owner March 30, 2025 18:51

melvin-bot bot requested review from lakchote and removed request for a team March 30, 2025 18:51

lakchote reviewed Mar 31, 2025

View reviewed changes

chrispader suggested changes Mar 31, 2025

View reviewed changes

neil-marcellini self-requested a review April 1, 2025 21:14

neil-marcellini requested changes Apr 2, 2025

View reviewed changes

neil-marcellini requested a review from marcaaron April 2, 2025 18:54

marcaaron requested changes Apr 2, 2025

View reviewed changes

marcaaron assigned fabioh8010 Apr 2, 2025

fabioh8010 added 2 commits April 6, 2025 18:14

Address small review comments

1b8c804

Add tests for fastMerge

19e2d20

fabioh8010 added 3 commits April 13, 2025 17:48

Improve tests

befaaeb

Simplify and extract applyMerge() batching merges logic to a separate…

df65c33

… batchMergeChanges() function

Improve comments in Onyx.merge()

6301dfd

neil-marcellini marked this pull request as draft April 28, 2025 15:45

chrispader mentioned this pull request May 13, 2025

fix: Remove batching of merge delta changes, simplify code and remove unnecessary recursion #635

Draft

42 tasks

	if (isBatchingMergeChanges && targetValue === null) {
	if (isBatchingMergeChanges && !targetValue) {

Replace old value after null merges in nested properties when batching merges/updates #620

Are you sure you want to change the base?

Replace old value after null merges in nested properties when batching merges/updates #620

Uh oh!

Conversation

fabioh8010 commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Manual Tests

Author Checklist

Screenshots/Videos

Uh oh!

fabioh8010 commented Mar 3, 2025

Uh oh!

fabioh8010 commented Mar 6, 2025

Uh oh!

fabioh8010 commented Mar 30, 2025

Uh oh!

lakchote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrispader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrispader Mar 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

neil-marcellini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Apr 2, 2025

Uh oh!

marcaaron commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neil-marcellini commented Apr 7, 2025

Replace old value after `null` merges in nested properties when batching merges/updates #620

Replace old value after `null` merges in nested properties when batching merges/updates #620

fabioh8010 commented Mar 3, 2025 •

edited

Loading

chrispader Mar 31, 2025 •

edited

Loading

marcaaron commented Apr 2, 2025 •

edited

Loading

fabioh8010 commented Apr 25, 2025 •

edited

Loading