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

[CIS-638] Decrease channel launch times #802

Merged
merged 6 commits into from Feb 8, 2021
Merged

Conversation

ManWithBear
Copy link
Contributor

Average channel open time (duration from tap to idle state) is 1.3s.
A lot of time takes data base updates and table reloads.
But in most cases such reloads are redundant because object haven;t been changed, only touched and marked as dirty.

By reseting dirty, but not changed, objects I was able to reduce open time from 1.3s to 0.6s.

Rest of time is initial layout setup (VC and message list). It doesn't make much sense to invest time in this, since layout policy is about to change.

@ManWithBear ManWithBear added 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK labels Feb 7, 2021
Comment on lines +159 to +161
if object.changedValues().isEmpty {
self.writableContext.refresh(object, mergeChanges: false)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand it correctly, that object.hasChanges is set whenever you set a value on the object, but object.changedValues() actually evaluates the value and returns only when the value was actually changed?

Does it mean that we can actually revert the changes from #752 ? cc @b-onc

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's ⬆️ the case, we definitely don't need that persisted hash workaround, this would be a much cleaner one.
Also, the existence of this PR proves that #752 did not resolve the issue fully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically yes. hasChanges is just dirty flag, so whenever you touch object, it gets marked as with changes.
changedValues contains pairs of keys-values of actually changed values.
Here exist additional property hasPersistentChangedValues that should work as you would expect from hasChanges, but documentation is lucking, so I went with current solution

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #802 (c64a272) into main (5e97f8a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #802      +/-   ##
==========================================
+ Coverage   88.45%   88.48%   +0.02%     
==========================================
  Files         220      220              
  Lines        8611     8632      +21     
==========================================
+ Hits         7617     7638      +21     
  Misses        994      994              
Impacted Files Coverage Δ
...amChat/Workers/Background/AttachmentUploader.swift 92.80% <ø> (-0.12%) ⬇️
...urces/StreamChat/Database/DTOs/AttachmentDTO.swift 81.39% <100.00%> (+0.67%) ⬆️
Sources/StreamChat/Database/DTOs/UserDTO.swift 96.52% <100.00%> (+0.36%) ⬆️
...ources/StreamChat/Database/DatabaseContainer.swift 96.00% <100.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e97f8a...c64a272. Read the comment docs.

Copy link
Contributor

@VojtaStavik VojtaStavik left a comment

Choose a reason for hiding this comment

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

Looking good, I have mostly nit comments. One thing I don't understand is

How come this change is not removed by df7983b ?

Sources/StreamChat/Database/DTOs/UserDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO_Tests.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO_Tests.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO_Tests.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO_Tests.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO_Tests.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/UserDTO_Tests.swift Outdated Show resolved Hide resolved
@VojtaStavik
Copy link
Contributor

@ManWithBear All right, let's merge this!

@VojtaStavik VojtaStavik merged commit cb802e7 into main Feb 8, 2021
@VojtaStavik VojtaStavik deleted the CIS-638-channel-launch branch February 8, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants