Fix perf regression in DiffableSnapshot init (#138)

Reverts some changes from #136.

When attempting to run the latest `main` in our app, I noticed that a
large list would take several seconds to appear.

Instruments shows that `DiffableSnapshot.init(viewModel:)` is the
culprit:

<img width="1047" alt="Screenshot 2024-10-14 at 15 07 25"
src="https://github.com/user-attachments/assets/e729033c-cc63-467c-80be-2678985c3050">

It turns out that appending items one by one is much slower than
appending them all at once.

This reverts the change to this method introduced in #136.

I also added a simple performance regression test, which generates 10
sections of 10,000 items each and runs some trivial assertion.

On the current `main`, this takes a very long time to complete. With the
reverted change on this branch it takes about ~200-300ms on my laptop.
(This still seems pretty slow to me, but I can't see any other obvious
ways of optimizing this.)

While running the test, I noticed some flaky failures, and the following
error being logged:

> Diffable data source detected an attempt to insert or append 1 item
identifier that already exists in the snapshot. The existing item
identifier will be moved into place instead, but this operation will be
more expensive. For best performance, inserted item identifiers should
always be unique. Set a symbolic breakpoint on
BUG_IN_CLIENT_OF_DIFFABLE_DATA_SOURCE__IDENTIFIER_ALREADY_EXISTS to
catch this in the debugger. Item identifier that already exists:
289A50E9

This is caused by the random ID generated for each cell by default – I
guess I got pretty (un)lucky here!

I feel that it makes more sense generally to just use the section/item
indexes as ids instead. I think this would also make it easier to debug
any off-by-one errors or similar issues in tests.

I have separated the fix and test changes into separate commits so you
can have a look yourself. (Or if you don't like the changes to the test,
it would make sense to just merge the fix itself.)

Nice one!

---------

Co-authored-by: Jesse Squires <[email protected]>

Author: lachenmayer

CHANGELOG.md

@@ -12,7 +12,7 @@ NEXT
 
 - Allow setting a `UICollectionViewDelegateFlowLayout` object to receive flow layout events from the collection view. ([@jessesquires](https://github.com/jessesquires), [#134](https://github.com/jessesquires/ReactiveCollectionsKit/pull/134))
 - Swift Concurrency improvements: `@MainActor` annotations have been removed from most top-level types and protocols, instead opting to apply `@MainActor` to individual members only where necessary. The goal is to impose fewer restrictions/burdens on clients. ([@jessesquires](https://github.com/jessesquires), [#135](https://github.com/jessesquires/ReactiveCollectionsKit/pull/135))
-- Various performance improvements. ([@jessesquires](https://github.com/jessesquires), [#136](https://github.com/jessesquires/ReactiveCollectionsKit/pull/136))
+- Various performance improvements. ([@jessesquires](https://github.com/jessesquires), [#136](https://github.com/jessesquires/ReactiveCollectionsKit/pull/136), [@lachenmayer](https://github.com/lachenmayer), [#138](https://github.com/jessesquires/ReactiveCollectionsKit/pull/138))
 
 0.1.7
 -----

Sources/DiffableSnapshot.swift

@@ -21,12 +21,12 @@ extension DiffableSnapshot {
     init(viewModel: CollectionViewModel) {
         self.init()
 
-        for section in viewModel.sections {
-            self.appendSections([section.id])
+        let allSectionIdentifiers = viewModel.sections.map(\.id)
+        self.appendSections(allSectionIdentifiers)
 
-            for cell in section {
-                self.appendItems([cell.id], toSection: section.id)
-            }
+        viewModel.sections.forEach {
+            let allCellIdentifiers = $0.cells.map(\.id)
+            self.appendItems(allCellIdentifiers, toSection: $0.id)
         }
     }
 }

Tests/Fakes/FakeCollectionViewModel.swift

@@ -20,9 +20,9 @@ extension XCTestCase {
     @MainActor
     func fakeCollectionViewModel(
         id: String = .random,
-        sectionId: (Int) -> String = { _ in .random },
-        cellId: (Int, Int) -> String = { _, _ in .random },
-        supplementaryViewId: (Int, Int) -> String = { _, _ in .random },
+        sectionId: (Int) -> String = defaultSectionId,
+        cellId: (Int, Int) -> String = defaultCellId,
+        supplementaryViewId: (Int, Int) -> String = defaultCellId,
         numSections: Int = Int.random(in: 2...15),
         numCells: Int? = nil,
         useCellNibs: Bool = false,
@@ -53,8 +53,8 @@ extension XCTestCase {
     @MainActor
     func fakeSectionViewModel(
         id: String = .random,
-        cellId: (Int, Int) -> String = { _, _ in .random },
-        supplementaryViewId: (Int, Int) -> String = { _, _ in .random },
+        cellId: (Int, Int) -> String = defaultCellId,
+        supplementaryViewId: (Int, Int) -> String = defaultCellId,
         sectionIndex: Int = 0,
         numCells: Int = Int.random(in: 1...20),
         useCellNibs: Bool = false,
@@ -108,7 +108,7 @@ extension XCTestCase {
 
     @MainActor
     func fakeCellViewModels(
-        id: (Int, Int) -> String = { _, _ in .random },
+        id: (Int, Int) -> String = defaultCellId,
         sectionIndex: Int = 0,
         count: Int = Int.random(in: 3...20),
         useNibs: Bool = false,
@@ -182,3 +182,11 @@ extension XCTestCase {
         fields.contains(target) ? self.expectation(field: target, id: id, function: function) : nil
     }
 }
+
+private func defaultSectionId(_ index: Int) -> String {
+    "\(index)"
+}
+
+private func defaultCellId(_ sectionIndex: Int, _ itemIndex: Int) -> String {
+    "\(sectionIndex)-\(itemIndex)"
+}

Tests/TestDiffableSnapshot.swift

@@ -44,4 +44,13 @@ final class TestDiffableSnapshot: UnitTestCase, @unchecked Sendable {
         XCTAssertTrue(snapshot.sectionIdentifiers.isEmpty)
         XCTAssertTrue(snapshot.itemIdentifiers.isEmpty)
     }
+
+    @MainActor
+    func test_init_perf() {
+        let model = self.fakeCollectionViewModel(numSections: 10, numCells: 10_000)
+        measure {
+            let snapshot = DiffableSnapshot(viewModel: model)
+            XCTAssertEqual(snapshot.numberOfItems, 100_000)
+        }
+    }
 }