LinkedInAttic / LayoutKit

LayoutKit is a fast view layout library for iOS, macOS, and tvOS.
http://layoutkit.org
Apache License 2.0
3.16k stars 267 forks source link

BatchUpdates reloadItems has slow performance #90

Closed stephensilber closed 7 years ago

stephensilber commented 7 years ago

I have built a horizontally scrolling control that uses a fixed number of fixed-size cells using LayoutKit. When the user taps a cell, I'd like to reload the previously selected cell and the newly selected cell in order to move the selection indicator. I attempted to do this by using BatchUpdates and setting the reloadItems property, but the performance is very slow.

Once I build BatchUpdates, I call layoutAdapter.reload() with a fixed width and height, passing in the BatchUpdates I just built, and returning a cached array of Layout items to the layoutProvider (with the 2 indexPaths already rebuilt and updated in the cache).

To work around this, I've instead just fetched each cell manually (cellForItemAtIndexPath), built the new layout, and arranged/made in the cell.contentView. This shows much faster performance. What's the intended use of BatchUpdates, and is there a way to get better performance out of it?

nicksnyder commented 7 years ago

the performance is very slow

What call is being slow? Can you please provide numbers/measurements to quantify the slowness?

If you could attach the code you are using and how to reproduce the slowness, that would be helpful.

stephensilber commented 7 years ago

Sorry for the delayed response. I'm attaching a sample project that reproduces what I'm talking about.

DatePicker is a simple date selection control that has a single highlighted date. The expected behavior is that after we tap an unselected date, the currently selected date and the newly selected date are both reloaded to update their selected state. You'll notice that this causes visual stutter. The problem gets worse when we attach this control to a UITableView where the DatePicker is updated every time the tableView scrolls to a new date.

I've also attached a screenshot of Time Profiler that consists of opening the app and selecting a few dates in over a period of a couple of seconds. DatePicker.swift:176 in the sample project should help explain what I'm talking about.

datepickerprofile DatePickerExample.zip

stephensilber commented 7 years ago

@nicksnyder hey, I was curious if you'd had a chance to look at the sample project. Still stuck on this issue.

Just to add a little more info, I've done some timing using the LayoutKit logger, and when using BatchUpdates for reloading a max of 2 indexPaths, it can take upwards of 500ms+ to reload synchronously. Something just seems wrong there.

nicksnyder commented 7 years ago

Thanks for submitting a repo project. We haven't had a chance to take a look at it yet.

staguer commented 7 years ago

@stephensilber I tried reproducing the problem and couldn't see it. I tried both on the simulator and on an iPhone 5s. In both cases the UI responded swiftly and showed a smooth animation.

stephensilber commented 7 years ago

@staguer using the sample project I provided?

staguer commented 7 years ago

@stephensilber Yes. I haven't tried uncommenting the code in DatePicker.swift lines 184-187. That commented out code is the fast workaround, if I'm understanding correctly.

Just to be clear, you're not referring to the slow fading out of the perviously-selected item, right?

stephensilber commented 7 years ago

The slow fading happens due to a slow reload of the entire collectionView from what I understand. The fading looks fine when tapping a single date, but when a scrollview is attached to the "date picker", the performance becomes extremely slow. The expected behavior would be no animation, just an instant update of the previously selected and newly selected cells

staguer commented 7 years ago

I think the slow update is an intentional animation implemented by UICollectionView, and it's triggered by the UICollectionView performBatchUpdates method. In the simulator if you select Slow Animations from the Debug menu, the fade takes a lot longer. This indicates to me that it's an intentional animation and not just laggy UI.

stephensilber commented 7 years ago

It would be nice to have BatchUpdates be built without any animations, allowing for more performant updates, in that case. Our current solution is to calculate our own LayoutArrangements and then update those through the layout adapter. This works well, but I'm confused why it's so much faster compared to providing a fixed width/height previous to the other reload: method on layout adapter

staguer commented 7 years ago

If you disable animations with UIView.setAnimationsEnabled(false), does that make the update feel more responsive?

// In order to reload indexPaths, you must modify the data sources before calling this function
func layoutDatePicker(synchronous: Bool, updates: BatchUpdates? = nil) {
    DispatchQueue.global().async {
        let sections = self.buildSections(updates: updates)
        DispatchQueue.main.async {
            UIView.setAnimationsEnabled(false)
            self.layoutAdapter.reload(
                width: self.layout.itemSize.width, height: DatePicker.pickerHeight, synchronous: synchronous, batchUpdates: updates,
                layoutProvider: {_ -> [LayoutKit.Section<[Layout]>] in
                    return sections
            }, completion: {
                UIView.setAnimationsEnabled(true)
            })
        }
    }
}

I'm not sure what you mean about using your own LayoutArrangements though the layout adapter. I'd think that you'd have to bypass the adapter if you wanted to use your own arrangement, because the adapter makes the arrangement internally and only takes a Layout.

stephensilber commented 7 years ago

Disabling the animation does remove the fade and seems a bit more responsive, yes.

Also, the exact method I'm referring to is func reload(arrangement: [Section<[LayoutArrangement]>])on ReloadableViewLayoutAdapter:165

staguer commented 7 years ago

Ah I see, yes. That one calls UICollectionView.reloadData instead of UICollectionView.performBatchUpdates. Hence no animation.

stephensilber commented 7 years ago

I'll go ahead and close this issue since we've found a reasonable workaround for our initial problem. Thanks 😃