Improve channel list performance by enabling reconfigure cells

[laevandus]

🔗 Issue Links

Resolves ios-issues-tracking/790

🎯 Goal

Enable cell reconfiguring and guard against reentrancy which led to an assertion in Apple's code/

📝 Summary

• Enable cell reconfiguring in channel list which makes it much faster
• Guard against reentrancy which can happen if reconfigure cells call ends up triggering Core Data store change which is then picked up by database observers (and leads to calling reloadChannels again)

🧪 Manual Testing Notes

Exploratory testing:
Open channel list and trigger channel list changes (e.g. new messages arriving)

☑️ Contributor Checklist

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

[laevandus]

The issue was when reload called reconfigure cell and that in turn triggered Core Data store change which led to calling reloadChannels again. Then Apple throws an assert.
Pretty hard to unit-test this case.
I am thinking about using a boolean here which just reschedules reload channels if re-entrancy is detected.

[nuno-vieira]

Probably not worth to to test it 🤔

[Stream-iOS-Bot]

StreamChat XCMetrics

target	metric	benchmark	branch	performance	status
MessageList	Hitches total duration	10 ms	10.01 ms	-0.1% 🔽	🟡
	Duration	2.6 s	2.55 s	1.92% 🔼	🟢
	Hitch time ratio	4 ms per s	3.92 ms per s	2.0% 🔼	🟢
	Frame rate	79 fps	78.13 fps	1.1% 🔼	🟢
	Number of hitches	1	0.8	20.0% 🔼	🟢
ChannelList	Hitches total duration	12.5 ms	5.01 ms	59.92% 🔼	🟢
	Duration	2.6 s	2.12 s	18.46% 🔼	🟢
	Hitch time ratio	5 ms per s	2.43 ms per s	51.4% 🔼	🟢
	Frame rate	76 fps	79.67 fps	-4.83% 🔽	🟡
	Number of hitches	1.2	0.6	50.0% 🔼	🟢

[nuno-vieira]

Shouldn't this be set inside the reload completion block? 🤔

[martinmitrevski]

I also think that

[laevandus]

This is actually not a completion but it is the setData closure which is called as a first thing in performBatchUpdates (too early). This can be called multiple times although for us it seems to be called once (haven't dived into when exactly multiple changesets are generated).

 func reload<C>(
        using stagedChangeset: StagedChangeset<C>,
        reconfigure: ((IndexPath) -> Bool)? = nil,
        interrupt: ((Changeset<C>) -> Bool)? = nil,
        setData: (C) -> Void
    ) {

where setData is called after each changeset

for changeset in stagedChangeset {
            if let interrupt = interrupt, interrupt(changeset), let data = stagedChangeset.last?.data {
                setData(data)
                return reloadData()
            }
            
            performBatchUpdates({
                setData(changeset.data)
                
                if !changeset.sectionDeleted.isEmpty {
                    deleteSections(IndexSet(changeset.sectionDeleted))
                }
                // later we call reconfigureItems which can trigger re-entrancy

In summary, it is in a correct places. Note that performBatchUpdates's updates closure is not escaping and is called synchronously. Therefore, resetting the boolean is indeed at a correct place.

Flow:

1. Set boolean to true
2. Call reload
3. Loop changesets
4. Call setData
5. Call reconfigureItems <-- boolean must be set false after calling reconfigure
6. exit the reload function
7. Set the boolean to false

[sonarcloud]

Quality Gate passed

Issues
68 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud