Improve database model conversion performance by caching extra data and keeping row cache #3534

laevandus · 2024-12-17T10:26:30Z

🔗 Issue Links

Resolves: IOS-610

🎯 Goal

Improve performance of model conversion with more extreme use-cases (e.g. extra data is used a lot)

📝 Summary

Cache extra data because it can be large and makes JSONDecoder way too busy when DB changes occur
Remove reading the model state from the persistent storage in DatabaseContainer.write because it degrades performance a lot (other managed object contexts need to refetch everything, affects row cache)

🛠 Implementation

Performance measurement setup

Demo user Chewbacca has 19 muted channels and has a channel named "Performance Testing" with 12 members where each of the member has extra data filled with 162 fields (nested data).

var myData = [String: RawJSON]()
for index in 0..<40 {
    myData["extra_data_key_\(index)"] = .dictionary([
        "timestamp": .number(Date().timeIntervalSinceReferenceDate),
        "value": .string("value_\((0..<100).randomElement()!)"),
        "number": .number(Double(index)),
        "flag": .bool(true)
    ])
}
let updates = MemberUpdatePayload(extraData: [
    "extra_data_testing": .dictionary(myData),
    "extra_data_testing_2": .string("just testing stuff")
])

I added a simple snippet to the demo app (locally) which sent 20 messages with 1 second delay. While the message sending was taking place, I used time profiler to measure how busy threads are.

Time spent for extra data decoding went down a lot as expected, but time profiler was also showing that Core Data is busy fetching data for DTOs when asModel() functions were called (for ChannelDTO.asModel it was around 40% of time). What triggered this behaviour is the snippet in the DatabaseContainer.write where updated objects are fully refetched from the persistent store. This seems to affect the Core Data's row cache and makes it to discard it which in turn means that backgroundReadOnlyContext needs to refetch everything (because each context has its own DTO objects). The current state is that all the writes go through the writableContext, therefore there is not any need to actually sync update objects in the write call because there is no-one else changing the persistent store.

🎨 Showcase

Baseline
Cache - only extra data caching
Cache & merge off - extra data caching and not force updating updated objects
Cache & merge off & data prefetching - extra data caching, not force updating updated objects, and relationship prefetching enabled (StreamRuntimeCheck._isDatabasePrefetchingEnabled)

Measurement	Baseline	Cache	Cache & merge off	Cache & merge off & data prefetching
Total time (all threads)	6.45	5.39	4.46	4.35
`ChannelDTO.asModel()` (from channel controller)	1.97	1.44	0.80	0.82
`ChannelDTO.asModel()` (total)	4.07	2.96	2.34	1.75
`CurrentUserDTO.asModel()` (total)	1.82	1.49	1.46	0.84
Extra data decode (total)	1.33	0.12	0.13	0.13

Caching extra data also reduces the memory footprint for RawJSON

Baseline	Caching

Takeaways

Extra data caching is needed when extra data is used a lot (here we had an extreme case)
refresh:mergeChanges: affects a lot (~45% improvement in channel controller)
In general, this use-case saw ~33% of improvement in CPU usage
-CurrentChatUser should not reference full ChatChannel models (cids would be better)
Channel updates trigger current user model changes because updating a UserDTO referenced by CurrentUserDTO leads to CurrentUserDTO to trigger an update

🧪 Manual Testing Notes

☑️ 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)

Stream-SDK-Bot · 2024-12-17T10:35:22Z

SDK Size

`title`	`develop`	`branch`	`diff`	`status`
StreamChat	7.03 MB	7.03 MB	+1 KB	🟢
StreamChatUI	4.77 MB	4.77 MB	0 KB	🟢

Stream-SDK-Bot · 2024-12-17T10:41:40Z

SDK Performance

`target`	`metric`	`benchmark`	`branch`	`performance`	`status`
MessageList	Hitches total duration	10 ms	3.34 ms	66.6% 🔼	🟢
	Duration	2.6 s	2.55 s	1.92% 🔼	🟢
	Hitch time ratio	4 ms per s	1.31 ms per s	67.25% 🔼	🟢
	Frame rate	75 fps	78.26 fps	4.35% 🔼	🟢
	Number of hitches	1	0.4	60.0% 🔼	🟢

Stream-SDK-Bot · 2024-12-17T12:01:55Z

SDK Size

`title`	`develop`	`branch`	`diff`	`status`
StreamChat	7.03 MB	7.03 MB	+1 KB	🟢
StreamChatUI	4.77 MB	4.77 MB	0 KB	🟢

nuno-vieira

Added a couple of comments for now 👍

nuno-vieira · 2024-12-17T12:08:46Z

Sources/StreamChat/Database/DatabaseContainer.swift

-                // Refresh the state by merging persistent state and local state for avoiding optimistic locking failure
-                for object in self.writableContext.updatedObjects {
-                    self.writableContext.refresh(object, mergeChanges: true)
-                }
-


Can you explain why was this needed before, and now it is not? 🤔 What are the chances of this producing some regressions? 🤔

Isn't there a chance that Data will be missing or the FRC won't trigger updates?

Yes, we need more details on why was this removed.

I looked into the history of this and it goes like this:

In 2021 it started as an optimisation change for reducing context changes

// If you touch ManagedObject and update one of it properties to same value // Object will be marked as `updated` even it hasn't changed. // By reseting such objects we remove updates that are not updates. for object in self.writableContext.updatedObjects { if object.changedValues().isEmpty { self.writableContext.refresh(object, mergeChanges: false) } }

refresh(_:mergeChanges:false) = "Updates the persistent properties of a managed object to use the latest values from the persistent store. If flag is false, the context discards pending changes and the managed object becomes a fault. Upon next access, the context reloads the object’s values from the persistent store or last cached state."

In early 2023 it gets changed to
self.writableContext.refresh(object, mergeChanges: true)
"If flag is true, the context reloads the object’s property values from the store or the cache. Then the context applies local changes over the newly loaded values. Merging the local values into object always succeeds, and never results in a merge conflict."
I think this was done because change 1 was making FRC to lose track of changes.
Reasoning:
We use DTOs' willSave to force FRC updates by accessing the "parent" DTO and assigning a property and setting back the same value to the DTO. For example, UserDTO is getting saved and if it happens to be CurrentUserDTO.user, then UserDTO.willSave forces the CurrentUserDTO to change as well. Then FRC observing the CurrentUserDTO picks up the change. Discarding changes in these cases breaks FRC observing which I think the change number 2 tackled. The change in number 2 made sure these updates go through.
From UserDTO:

if let currentUser = currentUser, !currentUser.hasChanges { let fakeNewUnread = currentUser.unreadChannelsCount currentUser.unreadChannelsCount = fakeNewUnread }

I removed the if check when testing if it fixes these some crashes in CoreData. It didn't.

Since the change number 2 some things have happened like we do not use viewContext anymore (except reading the state in DataStore, DB observers do not use it anymore). All the DB mutations go through the writableContext. Therefore, there should never be a case where writableContext itself would require to refresh the DTO from the persistent store (because it is the only one changing it). That is the main reason I believe we can remove it.
Interesting side of it is that refreshing an object seems to discard cached data in Core Data (cached data in other contexts) which leads to performance degradation.

Didn't we also introduce a state layer context? Will this have an impact on it, have you checked that?

For state layer, with similar test, the change is 1.60 s to 0.77 s (as expected)

nuno-vieira · 2024-12-17T12:10:59Z

Sources/StreamChat/Utils/Codable+Extensions.swift

+        self.init(
+            dateFormatter: iso8601formatter,
+            dateCache: dateCache,
+            rawJSONCache: RawJSONCache(countLimit: 500)


In the performance benchmark table, we are not checking the memory usage. Is there any significant increase in memory by caching all the extra data?

I don't expect it, but maybe we clean it up on memory warning notifications, just in case?

Internally it is NSCache which clears itself on low memory warning. I used a wrapping class because of unit-tests. Can't mock NSCache from the Swift side (it is objc generic class and brings restrictions).

There is significant decrease in memory usage used for RawJSON.
Before:

After

nuno-vieira · 2024-12-17T12:12:15Z

Sources/StreamChat/Database/DTOs/MemberModelDTO.swift

-            } catch {
-                memberExtraData = [:]
-            }
+        let memberExtraData: [String: RawJSON]


Another interesting thing that we could try to improve is that ChatMember and ChatUser are both classes. And I think we should be able to convert them to structs without API changes. Could be interesting to see if there are any performance improvements by doing so 🤔

why do you think structs would improve performance? Probably only the SDK size would increase.

ChatUser.init itself is visible in the time profiler, but it is like 1 ms which is nothing compared to other things happening. At some point it would be nice to only use structs for models (consistent).

Usually structs are lightweight to create and destroy, so it should improve things, but maybe not significantly. @laevandus We not only need to check the init, but also how much time is spend destroying existing objects 🤔

Based on time profiler I don't see this popping up really, a couple of ms for ChatUser.__deallocating_deinit and ChatChannelMember.__deallocating_deinit. We should do that for consistency, but in a separate ticket. No real performance improvement here.

Okay nice 👍

martinmitrevski

Looks good! I'm only concerned about the refresh removal, let's test this a bit.

martinmitrevski · 2024-12-17T13:25:45Z

Sources/StreamChat/Database/DTOs/MemberModelDTO.swift

-            } catch {
-                memberExtraData = [:]
-            }
+        let memberExtraData: [String: RawJSON]


why do you think structs would improve performance? Probably only the SDK size would increase.

martinmitrevski · 2024-12-17T13:26:18Z

Sources/StreamChat/Database/DatabaseContainer.swift

-                // Refresh the state by merging persistent state and local state for avoiding optimistic locking failure
-                for object in self.writableContext.updatedObjects {
-                    self.writableContext.refresh(object, mergeChanges: true)
-                }
-


Yes, we need more details on why was this removed.

martinmitrevski · 2024-12-17T13:26:54Z

Sources/StreamChat/Utils/Codable+Extensions.swift

+        self.init(
+            dateFormatter: iso8601formatter,
+            dateCache: dateCache,
+            rawJSONCache: RawJSONCache(countLimit: 500)


I don't expect it, but maybe we clean it up on memory warning notifications, just in case?

…nd not force refreshing updated objects which affects Core Data row caches

nuno-vieira

LGTM! ✅ ⚡

martinmitrevski

LGTM ✅

sonarqubecloud · 2024-12-20T07:35:17Z

Quality Gate passed

Issues
173 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

laevandus added 🔧 WIP A PR that is Work in Progress 🌐 SDK: StreamChat (LLC) Tasks related to the StreamChat LLC SDK ⚡ Performance An Issue or PR related to performance improvements labels Dec 17, 2024

laevandus requested a review from a team as a code owner December 17, 2024 10:26

laevandus force-pushed the perf/extra-data-cache-and-db-observing branch from 93d2f62 to e424ea0 Compare December 17, 2024 11:57

laevandus changed the title ~~[WIP] Improve database model conversion performance by caching extra data and keeping row cache~~ Improve database model conversion performance by caching extra data and keeping row cache Dec 17, 2024

laevandus removed the 🔧 WIP A PR that is Work in Progress label Dec 17, 2024

nuno-vieira reviewed Dec 17, 2024

View reviewed changes

martinmitrevski reviewed Dec 17, 2024

View reviewed changes

laevandus changed the title ~~Improve database model conversion performance by caching extra data and keeping row cache~~ [WIP] Improve database model conversion performance by caching extra data and keeping row cache Dec 18, 2024

laevandus added the 🔧 WIP A PR that is Work in Progress label Dec 18, 2024

laevandus force-pushed the perf/extra-data-cache-and-db-observing branch 2 times, most recently from 41c1b22 to 7c25aea Compare December 19, 2024 07:54

laevandus changed the title ~~[WIP] Improve database model conversion performance by caching extra data and keeping row cache~~ Improve database model conversion performance by caching extra data and keeping row cache Dec 19, 2024

laevandus removed the 🔧 WIP A PR that is Work in Progress label Dec 19, 2024

Improve database model conversion performance by caching extra data a…

9f33d73

…nd not force refreshing updated objects which affects Core Data row caches

laevandus force-pushed the perf/extra-data-cache-and-db-observing branch from 7c25aea to 9f33d73 Compare December 19, 2024 12:26

nuno-vieira approved these changes Dec 19, 2024

View reviewed changes

martinmitrevski approved these changes Dec 20, 2024

View reviewed changes

Merge branch 'develop' into perf/extra-data-cache-and-db-observing

2944567

laevandus enabled auto-merge (squash) December 20, 2024 07:20

laevandus merged commit c65255f into develop Dec 20, 2024
14 checks passed

laevandus deleted the perf/extra-data-cache-and-db-observing branch December 20, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve database model conversion performance by caching extra data and keeping row cache #3534

Improve database model conversion performance by caching extra data and keeping row cache #3534

laevandus commented Dec 17, 2024 •

edited

Loading

Stream-SDK-Bot commented Dec 17, 2024

Stream-SDK-Bot commented Dec 17, 2024

Stream-SDK-Bot commented Dec 17, 2024 •

edited

Loading

nuno-vieira left a comment

nuno-vieira Dec 17, 2024

nuno-vieira Dec 17, 2024

martinmitrevski Dec 17, 2024

laevandus Dec 17, 2024

martinmitrevski Dec 17, 2024

laevandus Dec 19, 2024 •

edited

Loading

nuno-vieira Dec 17, 2024

martinmitrevski Dec 17, 2024

laevandus Dec 17, 2024

laevandus Dec 19, 2024

nuno-vieira Dec 17, 2024

martinmitrevski Dec 17, 2024

laevandus Dec 17, 2024

nuno-vieira Dec 17, 2024 •

edited

Loading

laevandus Dec 19, 2024

nuno-vieira Dec 19, 2024

martinmitrevski left a comment

martinmitrevski Dec 17, 2024

martinmitrevski Dec 17, 2024

martinmitrevski Dec 17, 2024

nuno-vieira left a comment

martinmitrevski left a comment

sonarqubecloud bot commented Dec 20, 2024

Improve database model conversion performance by caching extra data and keeping row cache #3534

Improve database model conversion performance by caching extra data and keeping row cache #3534

Conversation

laevandus commented Dec 17, 2024 • edited Loading

🔗 Issue Links

🎯 Goal

📝 Summary

🛠 Implementation

Performance measurement setup

🎨 Showcase

Takeaways

🧪 Manual Testing Notes

☑️ Contributor Checklist

Stream-SDK-Bot commented Dec 17, 2024

SDK Size

Stream-SDK-Bot commented Dec 17, 2024

SDK Performance

Stream-SDK-Bot commented Dec 17, 2024 • edited Loading

SDK Size

nuno-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laevandus Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuno-vieira Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinmitrevski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuno-vieira left a comment

Choose a reason for hiding this comment

martinmitrevski left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 20, 2024

Quality Gate passed

laevandus commented Dec 17, 2024 •

edited

Loading

Stream-SDK-Bot commented Dec 17, 2024 •

edited

Loading

laevandus Dec 19, 2024 •

edited

Loading

nuno-vieira Dec 17, 2024 •

edited

Loading