[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

appleguy · 2017-07-09T23:29:15Z

See #430 for details.

Besides the unit tests, which do pass, is there any dedicated ASCollectionView thrash tester I should run? I see the ASTableViewStressTest in examples_extra/.

…nds changes. See #430 for details.

Adlai-Holler

This is a great improvement for flow layouts! Pinterest's collection view layout relies on calculatedSize rather than layoutThatFits: and so we will need to modify that code before landing this patch.

@nguyenhuy Would you be willing to spec out that change? I can get to it in a couple of days but I've got a lot of non-Texture work on my plate this week.

appleguy · 2017-07-11T01:53:54Z

@Adlai-Holler @nguyenhuy thanks for looking!

The most important question here: what caused this to start crashing? It could be something happening in my app, but it also might be one of the other refactors that has happened across the collection stack.

Perhaps there is a different way to fix the crash that would not create an issue for any layout implementations using -calculatedSize.

nguyenhuy · 2017-07-13T16:58:12Z

@appleguy To answer your question, looking at the crash log, I believe there were some blocks scheduled on the main queue before the relayout, and one of them is the completion block of a change set update (-dataController:didUpdateWithChangeSet:updates:). And since -[ASMainSerialQueue performBlockOnMainThread:] flushes all scheduled blocks at once, the completion block might be executed at an unexpected time.

nguyenhuy · 2017-07-13T17:04:13Z

Source/ASCollectionView.mm

+    // Because -invalidateLayout doesn't trigger any operations by itself, and we answer queries from UICollectionView using layoutThatFits:,
+    // we invalidate the layout before we have updated all of the cells. Any cells that the collection needs the size of immediately will get
+    // -layoutThatFits: with a new constraint, on the main thread, and synchronously calculate them. Meanwhile, relayoutAllNodes will update
+    // the layout of any remaining nodes on background threads (and fast-return for any nodes that the UICV got to first).
    [self.collectionViewLayout invalidateLayout];


I think invalidating the collection layout should be done "in the future", inside -[ASDataController _relayoutAllNodes] here. The reason is that there might be new layouts calculated before the size change, but still stuck on the main serial queue and not yet applied. And those layouts are invalid. But calling invalidating here is too soon and doesn't invalidate them.

So it looks like we need to change -[ASDataController relayotuAllNodes] to accept a block that get called on the main serial queue.

@nguyenhuy Interesting; that sounds about right. This seems like a very important issue to fix. Do you have any ideas on how we would test it properly?

I'm a bit worried because I don't have time to dig into this right now. In fact, I haven't synced with upstream for almost 2 months, and I have to focus on some deadlines for Q4 and 2018 planning.

If you would be able to take over this diff, I'd certainly appreciate it. I bet this change would greatly help the Pinterest app, especially on iPad rotation.

@appleguy Yeah, I can take over this diff, but not anytime soon though I'm afraid.

nguyenhuy · 2017-07-13T17:12:53Z

Source/ASCollectionView.mm

+    // Because -invalidateLayout doesn't trigger any operations by itself, and we answer queries from UICollectionView using layoutThatFits:,
+    // we invalidate the layout before we have updated all of the cells. Any cells that the collection needs the size of immediately will get
+    // -layoutThatFits: with a new constraint, on the main thread, and synchronously calculate them. Meanwhile, relayoutAllNodes will update
+    // the layout of any remaining nodes on background threads (and fast-return for any nodes that the UICV got to first).


Meanwhile, relayoutAllNodes will update the layout of any remaining nodes on background threads (and fast-return for any nodes that the UICV got to first).

I'm actually not aware of this behavior in the current codebase. The current implementation of -relayoutAllNodes processes all nodes in the UIKit index space on the main thread. That being said, I think this is the right approach that we should pursue. Are you planning to implement it shortly?

Heh, you're right. I suppose it is necessary for it to be synchronous in the current codebase without some kind of virtualization support (exposing only a subset of the datasource to UIKit).

nguyenhuy · 2017-07-13T17:23:48Z

@Adlai-Holler Sure, I'll prepare a change in Pinterest once this PR is merged.

ghost · 2017-10-08T06:42:25Z

🚫 CI failed with log

… to flush the ASMainSerialQueue before -invalidateLayout is called.

appleguy · 2017-10-08T07:31:38Z

Hmm, this erroneous test failure is striking again.

ASCollectionViewTests
testThatMultipleBatchFetchesDontHappenUnnecessarily, failed - Too many batch fetches!
/Users/Shared/buildkite/builds/iosf-garrett-VMrmmvuqKsO4.dyn.pinadmin.com-1/pinterest/texture/Tests/ASCollectionViewTests.mm:894

  if (batchFetchCount > 1) {
    XCTFail(@"Too many batch fetches!");

ghost · 2017-10-08T08:04:43Z

🚫 CI failed with log

ghost · 2017-10-09T17:17:57Z

🚫 CI failed with log

@djblake

Good catch by @djblake, if you scroll fast enough, you leave images set on the image node because didExitPreloadRange (which would have cleared it) was already called.

* Updating to support animated WebP * Fix a deadlock with display link * Fix playhead issue. * Fix up timing on iOS 10 and above * Don't redraw the same frame over and over * Clear out layer contents if we're an animated GIF on exit range * Clear out cover image on exit of visible range * Don't set cover image if we're no longer in display range. * Don't clear out image if we're not an animated image * Only set image if we're not already animating * Get rid of changes to podfile * Add CHANGELOG entry * Update license * Update PINRemoteImage * Remove commented out lines in example

…layout of nodes.

appleguy · 2017-10-13T23:30:51Z

@nguyenhuy we need to either land this one now, and then I can fix merge conflicts for #616 — or we can land that one first and I'll fix this one.

Adlai-Holler

Nice, and I really appreciate the detailed comments explaining your rationale. Let's go with this 💯

nguyenhuy

Looks like the right approach to me. Sorry for the late review!

ghost · 2017-10-24T11:18:38Z

🚫 CI failed with log

nguyenhuy · 2017-10-24T12:13:08Z

CI passed.

@djblake

…nds changes. (TextureGroup#431) * [ASCollectionView] Improve performance and behavior of rotation / bounds changes. See TextureGroup#430 for details. * Edit CHANGELOG.md * [ASDataController] Implement -relayoutAllNodesWithInvalidationBlock:, to flush the ASMainSerialQueue before -invalidateLayout is called. * Don't set download results if no longer in preload range. (TextureGroup#606) Good catch by @djblake, if you scroll fast enough, you leave images set on the image node because didExitPreloadRange (which would have cleared it) was already called. * Animated WebP support (TextureGroup#605) * Updating to support animated WebP * Fix a deadlock with display link * Fix playhead issue. * Fix up timing on iOS 10 and above * Don't redraw the same frame over and over * Clear out layer contents if we're an animated GIF on exit range * Clear out cover image on exit of visible range * Don't set cover image if we're no longer in display range. * Don't clear out image if we're not an animated image * Only set image if we're not already animating * Get rid of changes to podfile * Add CHANGELOG entry * Update license * Update PINRemoteImage * Remove commented out lines in example * [ASDataController] Add nullable specifier to invalidationBlock for relayout of nodes.

[ASCollectionView] Improve performance and behavior of rotation / bou…

46743ad

…nds changes. See #430 for details.

appleguy self-assigned this Jul 9, 2017

appleguy requested review from maicki, nguyenhuy and Adlai-Holler July 9, 2017 23:29

Adlai-Holler reviewed Jul 10, 2017

View reviewed changes

Edit CHANGELOG.md

818b6a8

nguyenhuy reviewed Jul 13, 2017

View reviewed changes

nguyenhuy self-assigned this Sep 18, 2017

Merge branch 'master' into BoundsInvalidate

ef69615

[ASDataController] Implement -relayoutAllNodesWithInvalidationBlock:,…

86b4214

… to flush the ASMainSerialQueue before -invalidateLayout is called.

Merge branch 'master' into BoundsInvalidate

78a5e43

garrettmoon and others added 3 commits October 9, 2017 23:10

Don't set download results if no longer in preload range. (#606)

1d2bc7f

Good catch by @djblake, if you scroll fast enough, you leave images set on the image node because didExitPreloadRange (which would have cleared it) was already called.

[ASDataController] Add nullable specifier to invalidationBlock for re…

9aa4fd2

…layout of nodes.

appleguy mentioned this pull request Oct 13, 2017

[ASCollectionView] Call -invalidateFlowLayoutDelegateMetrics when rotating. #trivial #616

Merged

Merge branch 'master' into BoundsInvalidate

bbda3c8

Adlai-Holler approved these changes Oct 23, 2017

View reviewed changes

nguyenhuy approved these changes Oct 24, 2017

View reviewed changes

nguyenhuy merged commit 68f3468 into master Oct 24, 2017

Adlai-Holler deleted the BoundsInvalidate branch November 20, 2017 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

appleguy commented Jul 9, 2017 •

edited

Loading

Adlai-Holler left a comment

appleguy commented Jul 11, 2017

nguyenhuy commented Jul 13, 2017 •

edited

Loading

nguyenhuy Jul 13, 2017 •

edited

Loading

appleguy Sep 18, 2017

nguyenhuy Sep 18, 2017

nguyenhuy Jul 13, 2017

appleguy Oct 8, 2017

nguyenhuy commented Jul 13, 2017

ghost commented Oct 8, 2017

appleguy commented Oct 8, 2017

ghost commented Oct 8, 2017

ghost commented Oct 9, 2017

appleguy commented Oct 13, 2017

Adlai-Holler left a comment

nguyenhuy left a comment

ghost commented Oct 24, 2017

nguyenhuy commented Oct 24, 2017

[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

[ASCollectionView] Improve performance and behavior of rotation / bounds changes. #431

Conversation

appleguy commented Jul 9, 2017 • edited Loading

Adlai-Holler left a comment

Choose a reason for hiding this comment

appleguy commented Jul 11, 2017

nguyenhuy commented Jul 13, 2017 • edited Loading

nguyenhuy Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

appleguy Sep 18, 2017

Choose a reason for hiding this comment

nguyenhuy Sep 18, 2017

Choose a reason for hiding this comment

nguyenhuy Jul 13, 2017

Choose a reason for hiding this comment

appleguy Oct 8, 2017

Choose a reason for hiding this comment

nguyenhuy commented Jul 13, 2017

ghost commented Oct 8, 2017

appleguy commented Oct 8, 2017

ghost commented Oct 8, 2017

ghost commented Oct 9, 2017

appleguy commented Oct 13, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

nguyenhuy left a comment

Choose a reason for hiding this comment

ghost commented Oct 24, 2017

nguyenhuy commented Oct 24, 2017

appleguy commented Jul 9, 2017 •

edited

Loading

nguyenhuy commented Jul 13, 2017 •

edited

Loading

nguyenhuy Jul 13, 2017 •

edited

Loading