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.

Sign up for GitHub

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

Jump to bottom

Improve performance of ChatChannel database model conversions ~7 times #3325

Merged
merged 12 commits into from
Jul 25, 2024
Merged

Improve performance of ChatChannel database model conversions ~7 times #3325

merged 12 commits into from
Jul 25, 2024

Conversation

laevandus
Copy link
Contributor

@laevandus laevandus commented Jul 22, 2024 •
edited
Loading

🔗 Issue Links

Related to PBE-5004

🎯 Goal

Improve performance of ChatChannel database model conversions by removing additional fetch requests

📝 Summary

  • Roughly 7x CPU usage reduction when converting channel database models to immutable ChatChannel
  • CPU time is saved by reducing additional fetch requests and using the DTOs data directly

🛠 Implementation

In #3299 we removed CoreDataLazy and this opened up an opportunity to optimise asModel() functions (these convert local database data to immutable models like ChatChannel). This PR replaces additional DB calls with accessing the NSManagedObject's data directly.

🎨 Showcase

Profiling steps: log out Chewbacca, log in with Chewbacca, wait until CPU usage settles. Observe the time spent in converting ChannelDTO to ChatChannel when the user is opening the channel list.

Measured: iPhone 12 Pro Max
279 ms > 38 ms 7x improvement

Before After
Baseline Final

🧪 Manual Testing Notes

Exploratory testing around variety of channel actions, e.g. send a message > does everything look allright.

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change should be manually QAed
  • Changelog is updated with client-facing changes
  • Changelog is updated with new localization keys
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (docusaurus, tutorial, CMS)

@laevandus laevandus added 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK ⚡ Performance An Issue or PR related to performance improvements labels Jul 22, 2024
@laevandus laevandus requested a review from a team as a code owner July 22, 2024 11:28
@laevandus laevandus marked this pull request as draft July 22, 2024 11:38
@laevandus laevandus changed the title Improve performance of ChatChannel database model conversions more than two times Improve performance of ChatChannel database model conversions ~3.5x Jul 22, 2024
@laevandus laevandus changed the title Improve performance of ChatChannel database model conversions ~3.5x Improve performance of ChatChannel database model conversions ~4.5x Jul 23, 2024
@laevandus laevandus changed the title Improve performance of ChatChannel database model conversions ~4.5x Improve performance of ChatChannel database model conversions ~4.4x Jul 23, 2024
@laevandus laevandus force-pushed the perf/reduce-fetch-requests-in-as-model branch 2 times, most recently from 0531f55 to 180eaaa Compare July 23, 2024 08:10
@laevandus laevandus marked this pull request as ready for review July 23, 2024 10:43
@GetStream GetStream deleted a comment from Stream-SDK-Bot Jul 23, 2024
@laevandus laevandus force-pushed the perf/reduce-fetch-requests-in-as-model branch from a1ae7ff to 6601b38 Compare July 24, 2024 08:33
nuno-vieira
nuno-vieira reviewed Jul 24, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
nuno-vieira
nuno-vieira reviewed Jul 24, 2024
View reviewed changes
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

It is looking good, but just added a couple more suggestions 👍

Sources/StreamChat/Database/DTOs/ChannelDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/ChannelDTO.swift Outdated Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/ChannelDTO.swift Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/MessageDTO.swift Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/MessageDTO.swift Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/MessageDTO.swift Show resolved Hide resolved
Sources/StreamChat/Database/DTOs/MessageDTO.swift Show resolved Hide resolved
Sources/StreamChat/Utils/Database/NSManagedObject+Extensions.swift Outdated Show resolved Hide resolved
laevandus
laevandus commented Jul 25, 2024
View reviewed changes
Sources/StreamChat/Database/DTOs/ChannelDTO.swift
Comment on lines +464 to +470
if let oldest = dto.oldestMessageAt?.bridgeDate {
messages = messages.filter { $0.createdAt >= oldest }
}
if let truncated = dto.truncatedAt?.bridgeDate {
messages = messages.filter { $0.createdAt >= truncated }
}
return messages
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some tests for these, so I kept this part to match what tests are expecting. Ideally we should add latestMessages to the ChannelDTO but I would prefer to do so in a separate PR.

@laevandus laevandus changed the title Improve performance of ChatChannel database model conversions ~4.4x Improve performance of ChatChannel database model conversions ~7x Jul 25, 2024
@laevandus laevandus changed the title Improve performance of ChatChannel database model conversions ~7x Improve performance of ChatChannel database model conversions ~7 times Jul 25, 2024
@GetStream GetStream deleted a comment from Stream-SDK-Bot Jul 25, 2024
nuno-vieira
nuno-vieira approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@nuno-vieira nuno-vieira left a comment

Choose a reason for hiding this comment

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

LGTM! ✅ 🚀 Really happy to see we are cleaning up these things 🚀

nuno-vieira
nuno-vieira reviewed Jul 25, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
martinmitrevski
martinmitrevski approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@martinmitrevski martinmitrevski left a comment

Choose a reason for hiding this comment

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

Great work 👏

@laevandus laevandus enabled auto-merge (squash) July 25, 2024 12:54
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
145 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.1% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarCloud

@laevandus laevandus merged commit 5ea8062 into develop Jul 25, 2024
14 checks passed
@laevandus laevandus deleted the perf/reduce-fetch-requests-in-as-model branch July 25, 2024 15:26
@Stream-SDK-Bot Stream-SDK-Bot mentioned this pull request Jul 30, 2024
Merged
nuno-vieira added a commit that referenced this pull request Jul 30, 2024
@nuno-vieira
Revert "Improve performance of ChatChannel database model conversio…
480b946 
…ns ~7 times (#3325)"

This reverts commit 5ea8062.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuno-vieira nuno-vieira nuno-vieira approved these changes

@martinmitrevski martinmitrevski martinmitrevski approved these changes

Assignees
No one assigned
Labels
⚡ Performance An Issue or PR related to performance improvements 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@laevandus @martinmitrevski @nuno-vieira