Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to synchronize operations within one update #490

Merged
merged 46 commits into from
Jul 16, 2024

Conversation

paultsimura
Copy link
Contributor

@paultsimura paultsimura commented Mar 3, 2024

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:

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

Copy link
Contributor

github-actions bot commented Mar 3, 2024

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

@paultsimura
Copy link
Contributor Author

paultsimura commented Mar 3, 2024

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

Copy link
Collaborator

@tgolen tgolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

lib/Onyx.js Outdated Show resolved Hide resolved
lib/Onyx.js Outdated Show resolved Hide resolved
@paultsimura
Copy link
Contributor Author

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
Copy link
Collaborator

tgolen commented Mar 4, 2024

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
Copy link
Contributor Author

paultsimura commented Mar 4, 2024

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

image

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

image

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

@paultsimura paultsimura requested a review from tgolen March 4, 2024 19:11
@paultsimura
Copy link
Contributor Author

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

@paultsimura
Copy link
Contributor Author

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

// 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
Copy link
Collaborator

tgolen commented Mar 4, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Collaborator

tgolen commented Mar 6, 2024

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

@paultsimura
Copy link
Contributor Author

@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
Copy link
Collaborator

tgolen commented Mar 7, 2024

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.

Copy link
Contributor

@kidroca kidroca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/Onyx.js Outdated Show resolved Hide resolved
lib/Onyx.js Outdated Show resolved Hide resolved
lib/Onyx.js Outdated Show resolved Hide resolved
@paultsimura
Copy link
Contributor Author

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

// 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
Copy link
Contributor Author

Here it is: Expensify/App#44010

@tgolen
Copy link
Collaborator

tgolen commented Jun 25, 2024

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

@tgolen tgolen closed this Jun 25, 2024
# Conflicts:
#	lib/Onyx.ts
#	lib/OnyxUtils.ts
@paultsimura
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Copy link
Collaborator

@tgolen tgolen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you! Here we go!

@tgolen tgolen merged commit 6317b21 into Expensify:main Jul 16, 2024
5 checks passed
Copy link
Contributor

🚀Published to npm in v2.0.57

Comment on lines +801 to +803
if (operations[0] === null) {
promises.push(() => set(key, batchedChanges));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref

@aldo-expensify
Copy link
Contributor

@paultsimura with these changes, does mergecollection onyx update have a performance advantage vs a bunch of merge updates if they belong to the same collection? or it should be roughly the same because the separate merge updates will be put together as a mergecollection?

@paultsimura
Copy link
Contributor Author

Hi @aldo-expensify, if we are talking about the operations within Onyx.update call - there shouldn't be any performance difference between a mergecollection and multiple singular merge operations as they will be treated as one mergecollection eventually.

@aldo-expensify
Copy link
Contributor

Nice, thanks!

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Jul 24, 2024

@paultsimura I found a bug in the version of react-native-onyx we are currently using (2.0.56) related to mergecollection and merge, and it seems like your PR here is fixing it. The bug: Expensify/App#46135 (comment)

When are we planning to release the changes made here?

@paultsimura
Copy link
Contributor Author

I'm planning to open a bump PR in the nearest days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.