Try to synchronize operations within one update

[paultsimura]

• Held on Bump onyx to 2.0.48 (after revert) App#42772 which updates the version of Onyx and includes this fix.

Details

This PR changes how we process the Onyx.update operation:

1. Enqueue all the operations per individual key;
2. Group the collection-related keys into a single mergeCollection call;
3. Process the rest of the individual keys with merge calls;

Related Issues

Expensify/App#37560

Automated Tests

I've added tests that cover the Onyx.update() with multiple merge operations of the same keys to verify that all the changes are batched into a single mergeCollection operation.

Manual Tests

Warning

Since this change alters one of the most common Onyx operations – we should perform a general regression testing of the system.

1. Perform a complex Onyx.update() operation with multiple updates of the same item in merge and mergeCollection operations.
2. Verify the collection items are updated with all the changes combined.

Operation example ref:

react-native-onyx/tests/unit/onyxTest.js

Lines 1046 to 1103 in eb17a5d

	return Onyx.update([
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: routineRoute,
	value: {
	waypoints: {
	1: 'Home',
	2: 'Work',
	3: 'Gym',
	},
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: holidayRoute,
	value: {
	waypoints: {
	1: 'Home',
	2: 'Beach',
	3: 'Restaurant',
	},
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE_COLLECTION,
	key: ONYX_KEYS.COLLECTION.ROUTES,
	value: {
	[holidayRoute]: {
	waypoints: {
	0: 'Bed',
	},
	},
	[routineRoute]: {
	waypoints: {
	0: 'Bed',
	},
	}
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: holidayRoute,
	value: {
	waypoints: {
	4: 'Home',
	},
	},
	},
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: routineRoute,
	value: {
	waypoints: {
	3: 'Gym',
	},
	},
	},
	]).then(() => {

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

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[paultsimura]

@tgolen I think I found a way to fix the issue of the out-of-order updates.

The main idea is the following: when an update call comes with multiple merge/set/mergeCollection operations, – do not process anything asynchronously. Instead, treat it as one transaction: stack all the operations by onject keys in the same exact order they come in the update data, calculate the diff in-memory, and then persist everything in one multiGet + multiSet take.

This PR is super draft, it lacks the validations as well as the broadcasting of the updates, but I would appreciate your opinion on the idea as a whole.

[tgolen]

I love the direction this is going in and I think your approach is a good one. I hope it holds up as you keep working on it!

[paultsimura]

In general, it does work – I gave it a little test run.
However, there are a lot of different callbacks in the current implementation and I'd appreciate your guidance on a more general level @tgolen.

Calls like these:

react-native-onyx/lib/Onyx.js

Lines 1511 to 1516 in 7cbf836

	// Prefill cache if necessary by calling get() on any existing keys and then merge original data to cache
	// and update all subscribers
	const promiseUpdate = Promise.all(_.map(existingKeys, get)).then(() => {
	cache.merge(collection);
	return scheduleNotifyCollectionSubscribers(collectionKey, collection);
	});

react-native-onyx/lib/Onyx.js

Lines 1153 to 1154 in 7cbf836

	// This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
	const updatePromise = broadcastUpdate(key, valueAfterRemoving, 'set', hasChanged, wasRemoved);

And all the places where we call the sendActionToDevTools. With the new approach, if we treat update as a single transaction with the eventual merge, it will be much harder to differentiate mergeCollection from merge.

What broadcasting approach would you suggest using here? If I broadcast each key update separately – would the collection listener handle each update individually?
Should I instead broadcast all collection-related keys as a single collection update, and non-collection keys as individual updates?

[tgolen]

If I broadcast each key update separately – would the collection listener handle each update individually?

I believe this would work yes, and the collection listener will handle each individual update. This would be the safest approach against causing regressions.

Should I instead broadcast all collection-related keys as a single collection update, and non-collection keys as individual updates?

The collection updates get a little tricky because some collection mappings will be expecting the callback to be triggered with the entire collection, while others might be expecting the callback to be triggered with each object in the collection. This is dependent on the waitForCollectionCallback mapping option.

I think I prefer the first method because that will make the least amount of behavior changes for Onyx.

[paultsimura]

This looks like a promising picture to me: when coming online, 46 operations were batched into one update:

Some of the keys had up to 7 merge operations stacked:

But eventually, the broadcast was called for each key only once, containing the final updated version of the object:

[paultsimura]

I have read the CLA Document and I hereby sign the CLA

[paultsimura]

@tgolen please make a sanity check when you have a minute, along with this comment.

The only thing that bothers me is that one failing test. I don't fully understand the nature of those subscriptions, and based on the comment in code, that first call is expected to be with null, undefined params 🤔

react-native-onyx/lib/Onyx.js

Lines 915 to 918 in 7cbf836

	// Here we cannot use batching because the null value is expected to be set immediately for default props
	// or they will be undefined.
	sendDataToConnection(mapping, null, undefined, false);
	return;

[tgolen]

Wow, that is a great improvement!

I don't think the test is too much to worry about. You can update the tests to change the nth to be 0 instead of 1 now. It makes sense in the context of the change you're making.

Before your PR:

• This Onyx.update() would have triggered each subscriber twice (first with null, second with the actual value)

After your PR:

• The updates are squished into a single callback with the final value 🎉

I think the important part of null that needs to be kept in mind (and you might want to add this to the unit tests with these changes) is that if I subscribe to a key that doesn't have any data stored on it, then the subscriber should be triggered once with null for a value. This allows the component to render and then the null value is substituted for undefined in withOnyx here. That way, when the component gets the undefined prop, the defaultProp will be picked up and used.

Does that all make sense?

[paultsimura]

Does that all make sense?

Yes, updated the test as you recommended – to verify that the first call for the non-existing keys is made with undefined.

[paultsimura]

@tgolen how terrible is the idea to make the following change?

case METHOD.MERGE_COLLECTION: 
    _.each(value, (_value, _key) => promises.push(() => merge(_key, _value)));
    break; 

react-native-onyx/lib/Onyx.js

Lines 1560 to 1562 in 69b2d59

	case METHOD.MERGE_COLLECTION:
	promises.push(() => mergeCollection(key, value));
	break;

From what I understand, it will trigger the re-render of components listening to a collection N times?

[tgolen]

We use mergecollection quite a bit so I would definitely be worried about performance issues with that.

[paultsimura]

@tgolen I think I've made a working solution: it updates the data in the correct order, but it's a bit slower than the current version.

Would it be possible to request a look from the side at the potential bottlenecks and improvements, especially the cache-related operations?
This PR is risky because it changes one of the mostly used Onyx functions, so an extra pair of eyes would really be helpful here.

[tgolen]

OK, great. Let me ping some of our experts on Slack to see if they can have a look at this PR. You can also see my comment here about how it can be reliably tested to help protect against major regressions.

[kidroca]

This Pull Request represents a positive step forward in enhancing performance by batching updates into a single persist-to-storage operation.

Considerations for follow-up work:

• Refactoring for Code Deduplication: There's an opportunity to further refine our codebase by reducing duplicate logic. Specifically, methods like set could be simplified to merely forward calls to Onyx.update with the appropriate parameters. Similarly, Onyx.merge might be refactored to leverage Onyx.update for batched updates. These changes would streamline our code and potentially simplify future maintenance.

• Optimizing Merge Logic: The current implementation's use of multiGet to read current values seems to diverge from our optimized merge strategy that emphasizes delta merges. Previously, we focused on reading full values only as needed to facilitate updates via keyChanged, while ensuring that only the delta of the merge was persisted to storage. The observed logic in this PR, which appears to send back the full JSON rather than just the delta, might warrant a reevaluation to ensure it aligns with our efficiency goals.

The overall changes introduced by this PR are commendable and contribute to ongoing efforts to improve performance.

[paultsimura]

@kidroca could you please help me understand whether the mergeQueue was introduced to chain the merge operations within one update call, or if there's more to it? How broad is the "single tick" definition?

react-native-onyx/lib/Onyx.js

Lines 23 to 25 in 3f77c6b

	// Key/value store of Onyx key and arrays of values to merge
	const mergeQueue = {};
	const mergeQueuePromise = {};

react-native-onyx/lib/Onyx.js

Lines 1285 to 1291 in 3f77c6b

	// Merge attempts are batched together. The delta should be applied after a single call to get() to prevent a race condition.
	// Using the initial value from storage in subsequent merge attempts will lead to an incorrect final merged value.
	if (mergeQueue[key]) {
	mergeQueue[key].push(changes);
	return mergeQueuePromise[key];
	}
	mergeQueue[key] = [changes];

[paultsimura]

Hey @tgolen, this is ready for the re-review. I performed some tests for the functionality this change was aimed to fix, but it would be awesome to conduct a full system test for this bump as this PR has been a loong time in the making and it introduces a somewhat breaking change.

[tgolen]

OK, that's probably wise. In order to do that, you need to make an App PR that points to this specific branch. Then I can create an ad-hoc build that we can give to QA for full regression testing.

[paultsimura]

make an App PR that points to this specific branch

Could you please share an example PR or give me a hint on how to do that?
I'm trying to use the following in the package.json:

"react-native-onyx": "git+https://github.com/paultsimura/react-native-onyx#28c6d5c165ab1ea109423f1119cee0be85a2ab3b",

But this way, npm i generates a directory with no src:

[tgolen]

That's the correct way to do it, yeah. There should be a couple other examples in the package.json which do this. I think, what I forgot to mention is that you need to temporarily include the built source files on your branch. I believe you can do this with the git add -f command (-f to force the file to be added and bypass the .ignore file).

[paultsimura]

Here it is: Expensify/App#44010

[tgolen]

From the issue, it sounds like this can be closed for now.

[paultsimura]

Hey @tgolen, I'm both ashamed and happy to say that apparently the bust I encountered were because I configured the App incorrectly after being OOO for a long time. I'd like to reopen this PR and the related issue and give it a shot.

[paultsimura]

Hey @tgolen @srikarparsi, could you please give this PR a final look and proceed to merge based on this test result?

[tgolen]

Looks great, thank you! Here we go!

[github-actions]

🚀Published to npm in v2.0.57