-
Notifications
You must be signed in to change notification settings - Fork 24
fix(backup-performance): message creation - WPB-20905 #3711
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
base: release/cycle-4.8
Are you sure you want to change the base?
fix(backup-performance): message creation - WPB-20905 #3711
Conversation
Test Results 6 files 637 suites 10m 26s ⏱️ For more details on these failures, see this check. Results for commit 71e3224. |
conversationLocalStore: any ConversationLocalStoreProtocol, | ||
userLocalStore: any UserLocalStoreProtocol | ||
userLocalStore: any UserLocalStoreProtocol, | ||
isProcessingBackup: Bool |
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.
Could you please make the purpose of this flag somehow more clear?
E.g. leave a doc comment somewhere when the flag should be set to true
or false
.
Also we could rename it somehow, like
skipFetching...
skipChecking...
alwaysCreate...
...?
or similar.
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.
+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.
good point ! I was considering introducing a configuration object with flags such as skipFetching
as I may need to introduce more conditions
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.
LGTM. Nice performance improvement.
conversationLocalStore: any ConversationLocalStoreProtocol, | ||
userLocalStore: any UserLocalStoreProtocol | ||
userLocalStore: any UserLocalStoreProtocol, | ||
isProcessingBackup: Bool |
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.
+1
question: If this checking somehow fails, what would happen if we attempt to create a message when it already exists? |
Since there's no uniqueness constraint on the message entity in core data, it would create a duplicate entry in the db |
Issue
Using the time profiler with a backup sample of ~15k messages, I observed that 34% of the time importing a backup is spent doing a fetch request to fetch messages we’re importing. (in
MessageLocalStore.fetchOrCreateClientMessage
)However, since we’re already checking if messages exist before creating them (as part of backup import), we don’t need to try to fetch them, we just need to create them.
Solution
Add methods:
MessageLocalStore.createClientMessage
MessageLocalStore.createAssetClientMessage
And use these when processing messages from the backup in
ConversationProtobuMessageProcessor.processProtobufMessage
Result
Measured the performance of importing a backup of ~15k messages, using a baseline build with no performance improvements and another build with the improvements from this PR (#3694) + the current PR.
Note: The change from this PR (#3694) was between 1,2 - 1,31 times faster
Checklist
[WPB-XXX]
.