Skip to content

Conversation

@JosephDuffy
Copy link
Member

When adding a lot of sections (we have a screen which inserts 100~175 on initial load) the performance is quite poor.

Along with the improve-composed-section-provider-performance branch (which I want to fully validate before making a PR) performing these changes in batch helps a lot.

@JosephDuffy JosephDuffy requested a review from shaps80 December 7, 2020 13:31
isPerformingBatchUpdates = true
updateDelegate?.willBeginUpdating(self)

updates(self)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if passing the section provider here is really useful? It might be more confusing than helpful.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I don't think its a good idea tbh. Also definitely not required (for now anyway) so the minimum API would be better IMHO.

Copy link
Collaborator

@shaps80 shaps80 left a comment

Choose a reason for hiding this comment

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

so I'm not at all against this, but the current will/did calls were supposed to handle this. Just not via a closure. In fact your imp obvs just wraps those calls.

I picked the minimum implementation and thought its more helpful to not use closures.

Thoughts?

@JosephDuffy
Copy link
Member Author

JosephDuffy commented Dec 7, 2020

so I'm not at all against this, but the current will/did calls were supposed to handle this. Just not via a closure. In fact your imp obvs just wraps those calls.

I picked the minimum implementation and thought its more helpful to not use closures.

Thoughts?

I have tried to move this "up the chain" to the top-level consumer (e.g. the CollectionCoordinator) by making the calls balanced (so nothing is applied until the number of end calls matches the number of begin) but it caused issues I couldn't immediately debug.

My guess is that the indexes for changes get out of sync (since they're created in the update closures but don't account for further changes e.g. deletes). I think this might be a problem here, and appears to be the reason that willBeginUpdating(_:)/(didEndUpdating(_:) calls from child providers can't be bypassed while isPerformingBatchUpdates.

If you can see a better way (that's not a large refactor to account for these changing index paths) to insert large numbers of section in a single batch (from the perspective of the collection view) then I'll work that alternative approach.

@shaps80
Copy link
Collaborator

shaps80 commented Dec 8, 2020

You seem to have a better view of it currently. I'll trust your judgement.

@JosephDuffy
Copy link
Member Author

Closing this in favour of composed-swift/ComposedUI#15. improve-composed-section-provider-performance is probably worth it and will be looked at as a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants