-
Notifications
You must be signed in to change notification settings - Fork 70
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
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
@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 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. |
There was a problem hiding this 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!
In general, it does work – I gave it a little test run. Calls like these: Lines 1511 to 1516 in 7cbf836
Lines 1153 to 1154 in 7cbf836
And all the places where we call the What broadcasting approach would you suggest using here? 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.
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 I think I prefer the first method because that will make the least amount of behavior changes for Onyx. |
I have read the CLA Document and I hereby sign the CLA |
@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 Lines 915 to 918 in 7cbf836
|
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 Before your PR:
After your PR:
I think the important part of 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 |
0628fe5
to
d0d261b
Compare
@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; Lines 1560 to 1562 in 69b2d59
From what I understand, it will trigger the re-render of components listening to a collection N times? |
We use |
@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? |
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. |
There was a problem hiding this 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 toOnyx.update
with the appropriate parameters. Similarly,Onyx.merge
might be refactored to leverageOnyx.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 viakeyChanged
, 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.
@kidroca could you please help me understand whether the Lines 23 to 25 in 3f77c6b
Lines 1285 to 1291 in 3f77c6b
|
Here it is: Expensify/App#44010 |
From the issue, it sounds like this can be closed for now. |
# Conflicts: # lib/Onyx.ts # lib/OnyxUtils.ts
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. |
Hey @tgolen @srikarparsi, could you please give this PR a final look and proceed to merge based on this test result? |
There was a problem hiding this 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!
🚀Published to npm in v2.0.57 |
if (operations[0] === null) { | ||
promises.push(() => set(key, batchedChanges)); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref
@paultsimura with these changes, does |
Hi @aldo-expensify, if we are talking about the operations within |
Nice, thanks! |
@paultsimura I found a bug in the version of When are we planning to release the changes made here? |
I'm planning to open a bump PR in the nearest days. |
Details
This PR changes how we process the
Onyx.update
operation:mergeCollection
call;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 singlemergeCollection
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.
Onyx.update()
operation with multiple updates of the same item inmerge
andmergeCollection
operations.Operation example ref:
react-native-onyx/tests/unit/onyxTest.js
Lines 1046 to 1103 in eb17a5d
Author Checklist
### Related Issues
section aboveTests
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop