-
Notifications
You must be signed in to change notification settings - Fork 4
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
Avoid creation of list instances in a while true loop #196
Conversation
WalkthroughThe changes in Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Attachments
participant SyncFlow
Client->>Attachments: Get entries
Attachments-->>Client: Return entries
Client->>SyncFlow: Call asSyncFlow(entries, realTimeOnly)
SyncFlow-->>Client: Return Flow<SyncResult>
Client->>SyncFlow: Collect { (document, result) -> ... }
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (6 hunks)
Additional comments not posted (3)
yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (3)
72-72
: Added importkotlinx.coroutines.flow.mapNotNull
.This import is necessary for the changes in the
asSyncFlow
method to utilizemapNotNull
.
171-171
: RefactoredrunSyncLoop
andsyncAsync
methods to useattachments.value.entries
and updatedasSyncFlow
.The refactoring aligns with the PR's objective to optimize performance by avoiding unnecessary list instantiations. The use of
entries
directly is a good practice in this context.Also applies to: 193-198, 215-232
641-645
: AddedAttachmentEntry
class implementingMap.Entry<Document.Key, Attachment>
.This addition is crucial for the changes in
asSyncFlow
to work withCollection<Map.Entry<Document.Key, Attachment>>
instead ofCollection<Attachment>
. It ensures type safety and clarity in the handling of document keys and attachments.
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.
you should check on lint errors
done 30055a6 |
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (7 hunks)
Additional comments not posted (6)
yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (6)
43-43
: Added import forkotlin.collections.Map.Entry
.This import is necessary for the new
AttachmentEntry
class implementation.
73-73
: Added import forkotlinx.coroutines.flow.mapNotNull
.This import supports the new implementation in the
asSyncFlow
method, allowing for more efficient flow operations.
172-172
: RefactoredrunSyncLoop
to useattachments.value.entries
directly.This change aligns with the PR's objective to optimize performance by reducing unnecessary list creations.
194-198
: Introduced conditional logic to handle specific document synchronization.This change is well-implemented, providing a clear path for handling both specific and general document synchronization scenarios.
216-235
: RefactoredasSyncFlow
to handle real-time synchronization conditions.The method now correctly handles different synchronization modes based on the
realTimeOnly
parameter, improving clarity and functionality.
644-647
: Added a newAttachmentEntry
class implementingMap.Entry
.This addition is necessary for the new handling of attachments as entries rather than standalone objects, which helps in optimizing memory usage.
What this PR does / why we need it?
Avoids creation of list instances in a while true loop.
Any background context you want to provide?
What are the relevant tickets?
Fixes #
Checklist
Summary by CodeRabbit
Refactor
runSyncLoop
method for better performance and reliability.New Features
AttachmentEntry
class to streamline attachment handling.