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-733] Load expensive CoreData properties lazily #906

Merged
merged 4 commits into from Mar 23, 2021

Conversation

olejnjak
Copy link
Contributor

@olejnjak olejnjak commented Mar 22, 2021

Improve some asModel function performance by loading expensive top level properties lazily. This means that affected model structs are now thread-confined which should be addressed later as it does not make much sense for value types.

Some other changes were necessary:

  • @Cached wrapper now has computeValue closure as projectedValue as otherwise it cannot be set from outside as _cachedWrapper property is private ➡️ this will not be possible as __mentionedUsers is inaccessible because it is private automatically
__mentionedUsers.computeValue = { Set(dto.mentionedUsers.map { $0.asModel() }) }
  • some model inits needed to be reorder to make compiler happy

I consider the semantic change of _ChatChannel and _ChatMessage to be breaking so it is marked this way in changelog, even though the API remains unchanged.

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #906 (f463c08) into main (1fc0603) will decrease coverage by 0.06%.
The diff coverage is 90.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #906      +/-   ##
==========================================
- Coverage   87.21%   87.14%   -0.07%     
==========================================
  Files         238      238              
  Lines        9102     9147      +45     
==========================================
+ Hits         7938     7971      +33     
- Misses       1164     1176      +12     
Flag Coverage Δ
llc-tests 87.14% <90.24%> (-0.07%) ⬇️
llc-tests-ios12 87.14% <90.24%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Sources/StreamChat/Utils/Cached.swift 82.35% <25.00%> (-17.65%) ⬇️
Sources/StreamChat/Models/Channel.swift 78.94% <75.00%> (-1.06%) ⬇️
Sources/StreamChat/Database/DTOs/MessageDTO.swift 95.27% <91.42%> (-3.36%) ⬇️
Sources/StreamChat/Models/Message.swift 87.17% <96.96%> (+53.84%) ⬆️
Sources/StreamChat/Database/DTOs/ChannelDTO.swift 100.00% <100.00%> (ø)
...Tests/MockNetwork/RequestRecorderURLProtocol.swift 80.55% <0.00%> (+8.33%) ⬆️

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 1fc0603...f463c08. 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.

Great 🎉 Before merging this, let's address the regressions to our internal API.

Sources/StreamChat/Models/Channel.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/ChannelDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Models/Channel.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Utils/Cached.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Models/Message.swift Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@olejnjak olejnjak force-pushed the CIS-733-cd-performance branch 3 times, most recently from b0f6559 to 844f116 Compare March 23, 2021 12:27
Copy link
Contributor

@DominikBucher12 DominikBucher12 left a comment

Choose a reason for hiding this comment

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

At first, this spinned my head around. After seeing the bigger picture of @Cached property wrapper and the implementation I see this is awesome 👍

$_latestReactions = {
Set(
MessageReactionDTO
.loadLatestReactions(for: dto.id, limit: 5, context: context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoded limit is okay? Same for line 459.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, just moved the original code to make compiler happy. In ChannelDTO there is originally a TODO for that.

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.

Great 👍 Just a couple of nit comments and then please merge 🎉

Sources/StreamChat/Models/Channel.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Models/Message.swift Outdated Show resolved Hide resolved
@VojtaStavik VojtaStavik merged commit be2f34f into main Mar 23, 2021
@VojtaStavik VojtaStavik deleted the CIS-733-cd-performance branch March 23, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants