Add collection view batch updates #15

JosephDuffy · 2020-12-13T13:51:37Z

The idea of this is that without breaking the API the collection view will batch updates.

Inserting a large number of sections is the main area of performance loss we are currently encountering, because the sections are inserted individually and not batched. This change alone has reduce the initial load time of one our screens (which has 100-150 sections added at once) from 30-45 seconds down to less than a second (at least it is not noticeable).

I had created composed-swift/Composed#17 to try and address this, which has the advantage that it would apply to other view types (e.g. UITableView), but I believe does not offer the same performance improvements and it is restricted to a single ComposedSectionProvider.

This is a draft to collect feedback; as you can see there are some TODOs but I think there's enough implemented to provide an overview of the changes that would be required to implement this.

This does not currently work; there are situations that cause the collection NSInternalInconsistencyException', reason: 'Invalid update error. I have some failing tests that demonstrate what the result should be.

This is only required for the tests, so I don' think the `from` needs to be updated in `Package.swift` since consumers never run tests?

shaps80

Ok so its obviously a lot more complex and likely this is necessary. I'm never a big fan of index tracking since its very easy to break.

Could we improve this with perhaps a type that maintains an index. Then include a few accessors like:

stack.push()
stack.pop()
stack.isEmpty

EDIT: I'm obviously referring to the += and -= balance calls in the mapping.

shaps80 · 2020-12-14T11:53:35Z

Sources/ComposedUI/CollectionView/CollectionCoordinator.swift

@@ -192,31 +199,40 @@ open class CollectionCoordinator: NSObject {
        delegate?.coordinatorDidUpdate(self)
    }

+    fileprivate func debugLog(_ message: String) {
+        if #available(iOS 12, *) {
+            os_log("%@", log: OSLog(subsystem: "ComposedUI", category: "CollectionCoordinator"), type: .debug, message)


shaps80 · 2020-12-14T13:44:42Z

Sources/ComposedUI/CollectionView/CollectionCoordinator.swift

-                    let cell = self.collectionView.cellForItem(at: indexPath) else {
-                        indexPathsToReload.append(indexPath)
-                        continue
+                      !section.prefersReload(forElementAt: indexPath.item),


Not sure what's going on here with the whitespace?

This is how Xcode indents when copy/pasting or using re-indent.

I've reverted it, although I think it's clearer when some extra line breaks are added and running re-indent:

guard let section = self.sectionProvider.sections[indexPath.section] as? CollectionUpdateHandler, !section.prefersReload(forElementAt: indexPath.item), let cell = self.collectionView.cellForItem(at: indexPath) else { indexPathsToReload.append(indexPath) continue }

I think that's fine.

JosephDuffy · 2020-12-14T16:53:09Z

Ok so its obviously a lot more complex and likely this is necessary. I'm never a big fan of index tracking since its very easy to break.

I'm not enjoying it and I'm worried it's fragile, but the (pretty large and growing) test suite makes me feel a little better about it.

Could we improve this with perhaps a type that maintains an index. Then include a few accessors like:
stack.push()
stack.pop()
stack.isEmpty
EDIT: I'm obviously referring to the += and -= balance calls in the mapping.

I thought that tracking some sort of index (I was using the index provided by the mapping) would be useful/required, but the issue is that we don't know the order change could occur so I don't think there's a single index that can be tracked that helps.

At first I was going to create a Change type that contains the data of the change, but always with a var section: Int property to make some of the updates easier, e.g.:

allChanges.map { change in
	guard change.section > removedSection else { return change }

	var change = change
	change.section -= 1
	return change
}

But this isn't actually that useful and could cause more confusion because of differences with how changes are handled (see how sections removes modify other section removes for example)

I have been thinking about some sort of Changeset type that contains this logic though. It would allow for reuse with UITableView and would be a single unit that can be tested, rather than the current kind of hacky testing. The current tests actually apply the changes to a UICollectionView though so they are useful for catching mistakes in the expected results because the collection view will trigger an exception.

shaps80 · 2020-12-14T17:15:09Z

Sources/ComposedUI/CollectionView/CollectionCoordinator.swift

        reset()
        prepareSections()
        collectionView.reloadData()
    }

    public func mappingWillBeginUpdating(_ mapping: SectionProviderMapping) {
-        reset()
-        defersUpdate = true
+        batchedUpdatesCount += 1


Perhaps I'm misunderstanding but for the index stack I suggested, I meant for this value? Not sure how your response relates? Maybe you can elaborate here.

Also, yeah reuse across other coordinators makes total sense and esp from a testing POV.

Ah I thought you were referencing the index changes performed in each of the update delegate methods.

Moving batchedUpdatesCount to the Changeset type mentioned is part of the plan for that

shaps80

Only one question on the review. Otherwise, looks much better. Purposeful at least and definitely more testable and reusable.

EDIT: Approved by accident hahah!

shaps80 · 2020-12-15T12:42:55Z

Tests/ComposedUITests/ChangesReducerTests.swift

+        XCTAssertTrue(changesReducer.changeset.elementsUpdated.isEmpty)
+        XCTAssertTrue(changesReducer.changeset.elementsMoved.isEmpty)
+    }
+}


I assume you plan on adding more test cases later on?

This is only the start -- the CollectionCoordinator doesn't use it yet.

More tests will be added to this file before merging, but I think it's going to lean a little on the new CollectionCoordinator tests passing to validate correctness according to UICollectionView.

JosephDuffy · 2020-12-15T14:51:26Z

Tests/ComposedUITests/ChangesReducerTests.swift

+        XCTAssertTrue(changeset!.groupsUpdated.isEmpty)
+    }
+
+    func testRemoveAMovedIndex() {


@shaps80 This test, along with testMoveElementAtSameIndexAsRemove below, are currently failing but I'm not 100% sure about the tests themselves.

Can you confirm that the test looks correct? e.g. this should produce removes and inserts, not a remove and moves?

JosephDuffy · 2021-01-18T15:51:36Z

Moving to main repo with merged libraries: composed-swift/Composed#28

Add foundation for collection view batch updates

c59ea6d

JosephDuffy requested a review from shaps80 December 13, 2020 13:51

JosephDuffy mentioned this pull request Dec 13, 2020

Add performBatchUpdates(_:) function composed-swift/Composed#17

Closed

JosephDuffy added 6 commits December 13, 2020 14:20

Add some debug logging

2c4be5d

Clear batchedSectionUpdates in reset

70664cb

Fix section removal indexes in some situations

f45d4ea

Add failing test

4c3f200

Add more failing tests

bdbd20b

Use Composed 1.1.0

6a636bb

This is only required for the tests, so I don' think the `from` needs to be updated in `Package.swift` since consumers never run tests?

JosephDuffy mentioned this pull request Dec 14, 2020

Cache sections and numberOfSection composed-swift/Composed#18

Merged

shaps80 requested changes Dec 14, 2020

View reviewed changes

JosephDuffy added 2 commits December 14, 2020 14:32

Account for more section insert/removal scenarios

909c1b5

Improve handling of batched removals

12c9802

Revert indentation changes

11f1339

shaps80 reviewed Dec 14, 2020

View reviewed changes

JosephDuffy added 2 commits December 15, 2020 11:34

Begin refactor of applying changes

0b82546

Batch updates via mapping(_:move:to:)

96f031b

shaps80 approved these changes Dec 15, 2020

View reviewed changes

JosephDuffy added 7 commits December 15, 2020 13:14

Use consistent naming in ChangesReducer

1599d66

Appends Tests to file name

4d63e83

Fix removeGroups compilation

7b26084

Add moveElements overload that accepts tuples

538610b

Gather coverage when running tests

35200b1

Use ChangesReducer in CollectionCoordinator

2c070ac

Add more ChangesReducer tests

5b0399b

JosephDuffy commented Dec 15, 2020

View reviewed changes

This was referenced Dec 16, 2020

Rewrite delegates to take perform update closures composed-swift/Composed#14

Closed

Adding Suspendible support composed-swift/Composed#10

Closed

JosephDuffy mentioned this pull request Jan 18, 2021

Batch collection view updates composed-swift/Composed#28

Open

JosephDuffy closed this Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collection view batch updates #15

Add collection view batch updates #15

JosephDuffy commented Dec 13, 2020 •

edited

Loading

shaps80 left a comment •

edited

Loading

shaps80 Dec 14, 2020

shaps80 Dec 14, 2020 •

edited

Loading

JosephDuffy Dec 14, 2020

shaps80 Dec 14, 2020

JosephDuffy commented Dec 14, 2020

shaps80 Dec 14, 2020

JosephDuffy Dec 14, 2020

shaps80 Dec 15, 2020

shaps80 left a comment •

edited

Loading

shaps80 Dec 15, 2020

JosephDuffy Dec 15, 2020

JosephDuffy Dec 15, 2020

JosephDuffy commented Jan 18, 2021

Add collection view batch updates #15

Add collection view batch updates #15

Conversation

JosephDuffy commented Dec 13, 2020 • edited Loading

shaps80 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaps80 Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephDuffy commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaps80 left a comment • 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

JosephDuffy commented Jan 18, 2021

JosephDuffy commented Dec 13, 2020 •

edited

Loading

shaps80 left a comment •

edited

Loading

shaps80 Dec 14, 2020 •

edited

Loading

shaps80 left a comment •

edited

Loading