Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create an experiment to remove extra collection teardown step #975

Merged
merged 3 commits into from
Jun 19, 2018

Conversation

Adlai-Holler
Copy link
Member

Currently we have an issue where the background deallocation of most cell nodes is not happening, because of the following stack:

thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 8.1
  * frame #0: 0x000000010ac960ff PinterestDevelopment`::-[ASDisplayNode dealloc](self=0x00007fc71d959a00, _cmd="dealloc") at ASDisplayNode.mm:415
    frame #1: 0x000000010bd1c6aa PinterestDevelopment`-[PIPinCloseupGalleryCellV1 dealloc](self=0x00007fc71d959a00, _cmd="dealloc") at PIPinCloseupGalleryCell.m:189
    frame #2: 0x0000000118268a6e libobjc.A.dylib`objc_object::sidetable_release(bool) + 202
    frame #3: 0x000000010ac0b1f8 PinterestDevelopment`::-[ASCollectionElement .cxx_destruct](self=0x00006080003be300, _cmd=".cxx_destruct") at ASCollectionElement.mm:29
    frame #4: 0x0000000118252920 libobjc.A.dylib`object_cxxDestructFromClass(objc_object*, objc_class*) + 127
    frame #5: 0x000000011825e502 libobjc.A.dylib`objc_destructInstance + 124
    frame #6: 0x000000011825e539 libobjc.A.dylib`object_dispose + 22
    frame #7: 0x0000000118268a6e libobjc.A.dylib`objc_object::sidetable_release(bool) + 202
    frame #8: 0x000000011054a239 Foundation`empty + 58
    frame #9: 0x0000000110481f39 Foundation`-[NSConcreteMapTable dealloc] + 50
    frame #10: 0x0000000118268a6e libobjc.A.dylib`objc_object::sidetable_release(bool) + 202
    frame #11: 0x000000010aebbf98 PinterestDevelopment`-[ASElementMap .cxx_destruct](self=0x0000608000851310, _cmd=".cxx_destruct") at ASElementMap.m:41
    frame #12: 0x0000000118252920 libobjc.A.dylib`object_cxxDestructFromClass(objc_object*, objc_class*) + 127
    frame #13: 0x000000011825e502 libobjc.A.dylib`objc_destructInstance + 124
    frame #14: 0x000000011825e539 libobjc.A.dylib`object_dispose + 22
    frame #15: 0x0000000118268a6e libobjc.A.dylib`objc_object::sidetable_release(bool) + 202
    frame #16: 0x000000010ac611c7 PinterestDevelopment`::-[ASDataController setVisibleMap:](self=0x0000608000128fc0, _cmd="setVisibleMap:", visibleMap=0x000060400084e7f0) at ASDataController.mm:92
    frame #17: 0x000000010ac60ebe PinterestDevelopment`::-[ASDataController clearData](self=0x0000608000128fc0, _cmd="clearData") at ASDataController.mm:933
    frame #18: 0x000000010ac2f61c PinterestDevelopment`::-[ASCollectionView _asyncDelegateOrDataSourceDidChange](self=0x00007fc71c955c00, _cmd="_asyncDelegateOrDataSourceDidChange") at ASCollectionView.mm:579
    frame #19: 0x000000010ac2f3c0 PinterestDevelopment`::-[ASCollectionView setAsyncDelegate:](self=0x00007fc71c955c00, _cmd="setAsyncDelegate:", asyncDelegate=0x0000000000000000) at ASCollectionView.mm:572
    frame #20: 0x000000010ac2ac55 PinterestDevelopment`::-[ASCollectionView dealloc](self=0x00007fc71c955c00, _cmd="dealloc") at ASCollectionView.mm:327

This subtle issue was introduced in #797.

Having dug through the history, the [setAsyncDataSource:nil] in dealloc was migrated from a prior super.delegate = nil which was there because at the time UICollectionView's delegate was assign and so there were crashes when the collection view would outlive the delegate, and try to call out to a dangling pointer. Since iOS 9, collection view delegate is weak and so this should no longer be a problem.

I've removed these set:nil calls from the dealloc methods, behind a new experiment called exp_collection_teardown. As always, the default behavior is unaffected.

I'd like for this to work as-is, so that it becomes easier to reason about collection view teardown and it gets us back our background deallocation. If we find issues, we'll work through them.

As a small ridealong, I removed repeated access to weak variables in the delegate proxies. Should be a little faster and more reliable.

Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

LGTM overall, although it would be great if we have a test case to prevent regressions like this in the future. Not a blocking issue though.

@@ -930,6 +930,8 @@ - (void)clearData
ASDisplayNodeAssertMainThread();
if (_initialReloadDataHasBeenCalled) {
[self waitUntilAllUpdatesAreProcessed];
auto vis = self.visibleMap;
auto pending = self.pendingMap;
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes indeed!

@TextureGroup TextureGroup deleted a comment Jun 19, 2018
@appleguy
Copy link
Member

This is good, definitely running as an experiment is important as we've been encountering a range of hard-to-anticipate crashes from background deallocation (deeper instance variables like an NSMutableDictionary holding UIGestureRecognizers are not handled by default).

@appleguy
Copy link
Member

Will try to review later, just passing by now - as usual, you can merge without me if others have looked at the code.

@Adlai-Holler Adlai-Holler merged commit a0e5f4c into master Jun 19, 2018
@Adlai-Holler Adlai-Holler deleted the AHTeardown branch June 19, 2018 15:32
Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

LGTM and agree with @appleguy around unknown crashes - would be interesting if we would could find a way to monitor that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants