Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Try to synchronize operations within one update #490
Changes from 18 commits
b4c739b
e9966f0
9cb563e
c7247ea
1d32cc2
748c51f
d0d261b
44617ed
f537212
c160503
e32f0e3
b56d236
f2513d8
a98aeb1
150a16e
94ec428
b954d72
2796388
10ab402
90473be
3447e6f
a153237
290b4b7
00ddb11
bb3acaa
6fea4d3
918d2e9
47fc3cb
c986e51
3381ff8
31fd63e
e9abdd5
ad25727
4169591
d1b6b95
ca3cd29
8ea4699
28c6d5c
230ef57
c2e6351
153bc79
2f74bfd
45b0523
d11cbd4
873443c
4fabc64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm interested in the decision-making process for using
mergeCollection
versus individual merges based on the count of items in the update.A few questions arise from this approach:
Rationale Behind Individual Merges for Single Items: Could you provide more details on the decision to favor individual updates over
mergeCollection
when there's only one item in the update? Understanding the rationale could clarify whether it's a performance consideration, a matter of update specificity, or another reason.Notification to Collection Subscribers: How does this approach impact the notification of collection subscribers, especially in scenarios where a key is added or removed? The decision to handle single and multiple item updates differently could affect how subscribers are notified, potentially leading to inconsistencies.
Consistency in Handling Updates: If bypassing
mergeCollection
for single-item updates achieves the intended functionality and subscriber notification, could a similar logic not be applied to multiple item updates? Understanding the distinction between handling one versus multiple items could help in assessing potential improvements or optimizations in the update mechanism.Your insights on these points would enhance my understanding and ensure that the logic aligns with the intended behavior for collection updates.
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.
Thanks for the questions.
1. Rationale Behind Individual Merges for Single Items: I've performed a bunch of performance tests for both single- and collection merges when there is a single item to merge. The difference is not huge, but in general the single
merge
is about 2-3ms faster when the operation takes 100-120ms.2. Notification to Collection Subscribers: from what I managed to test, both single and collection updates notify the subscribers of a specific collection item as well as the whole collection subscriber.
For
mergeCollection
, we pass the collection key and then check all the single-item subscribers to notify:react-native-onyx/lib/Onyx.js
Lines 434 to 588 in 4f9cebf
For
merge
, we pass the item key, and "go upwards" – check all the other subscribers that may be subscribed to the whole collection:react-native-onyx/lib/Onyx.js
Lines 590 to 722 in 4f9cebf
In both cases, we iterate over every subscriber.
3. Consistency in Handling Updates:
If I understood you correctly, do you mean that we could potentially use
merge
operations for every enqueued collection item? If so – no, we cannot – it will trigger the collection subscriber callback N times, when the mergeCollection does it only once, with the fully updated snapshot of the collection.The reason why I'm safe with using the
merge
instead ofmergeCollection
when there's only 1 item is that the callback will be called the same amount of times – 1.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.
Thank you for your comprehensive explanations, @paultsimura.
I see the logic behind the current method and its potential for performance improvement. However, I'd like to offer a suggestion - not as a directive but as a point for consideration - that we might not need to differentiate between single-item and multiple-item updates right now. This thought is guided by a preference for simplicity, aiming to ease the review from others.
Perhaps the special handling for "1 collection key" scenarios could be tabled as an area for future exploration. This way, we can focus on the core functionality at present, leaving room for optimization down the line. Of course, this is just my perspective, and I'm open to further discussion or to hear other team members' thoughts on this matter.