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

Move initialization of PHPhotoLibrary and PHCachingImageManager to background #565

Closed
wants to merge 3 commits into from

Conversation

leonspok
Copy link
Contributor

@leonspok leonspok commented Apr 2, 2019

We have freezes (not deadlocks) inside PHCachingImageManager initialization. According to stach trace, it might be related to PHPhotoLibrary singleton initialization. So it was moved to background thread to avoid blocking UI.

Thread 0:
0   libsystem_kernel.dylib               0x00000001e61db428 __semwait_signal + 8
1   libsystem_c.dylib                    0x00000001e61503cc sleep + 44
2   CoreData                             0x00000001e9181634 -[NSXPCStore sendMessage:fromContext:interrupts:error:] + 868
3   CoreData                             0x00000001e91e6c98 -[NSXPCStore loadMetadata:] + 344
4   CoreData                             0x00000001e92ca290 __91-[NSPersistentStoreCoordinator addPersistentStoreWithType:configuration:URL:options:error:]_block_invoke + 1544
5   CoreData                             0x00000001e92d6be4 gutsOfBlockToNSPersistentStoreCoordinatorPerform + 212
6   libdispatch.dylib                    0x00000001e607e484 _dispatch_client_callout + 16
7   libdispatch.dylib                    0x00000001e605e744 _dispatch_lane_barrier_sync_invoke_and_complete + 56
8   CoreData                             0x00000001e92c79d4 _perform + 200
9   CoreData                             0x00000001e9161ec8 -[NSPersistentStoreCoordinator addPersistentStoreWithType:configuration:URL:options:error:] + 384
10  PhotoLibraryServices                 0x00000001f44405e0 +[PLManagedObjectContext _configureXPCPersistentStoreCoordinator:] + 616
11  AssetsLibraryServices                0x00000001f402778c __pl_dispatch_sync_block_invoke + 36
12  libdispatch.dylib                    0x00000001e607e484 _dispatch_client_callout + 16
13  libdispatch.dylib                    0x00000001e605e744 _dispatch_lane_barrier_sync_invoke_and_complete + 56
14  AssetsLibraryServices                0x00000001f402775c pl_dispatch_sync + 68
15  PhotoLibraryServices                 0x00000001f44408bc +[PLManagedObjectContext sharedPersistentStoreCoordinator] + 152
16  PhotoLibraryServices                 0x00000001f443b560 -[PLManagedObjectContext initWithConcurrencyType:useSharedPersistentStoreCoordinator:] + 124
17  PhotoLibraryServices                 0x00000001f443b3d8 +[PLManagedObjectContext contextForPhotoLibrary:name:] + 200
18  PhotoLibraryServices                 0x00000001f435e528 -[PLPhotoLibrary _loadDatabase:] + 416
19  PhotoLibraryServices                 0x00000001f4352168 -[PLPhotoLibrary initWithTransientContext:name:pathManager:] + 580
20  Photos                               0x00000001f53b0fa4 -[PHPhotoLibrary photoLibrary] + 108
21  Photos                               0x00000001f53b1f80 -[PHPhotoLibrary registerChangeObserver:] + 168
22  Photos                               0x00000001f5391630 -[PHImageManager init] + 420
23  Photos                               0x00000001f53957bc -[PHCachingImageManager init] + 40
24  ChattoAdditions                      0x000000010360ed84 PhotosInputDataProvider.init() + 159108 (PhotosInputDataProvider.swift:63)

@leonspok leonspok marked this pull request as ready for review April 2, 2019 15:57

DispatchQueue.main.async(execute: { [weak self] in
guard let self = self else {
DispatchQueue.main.async(execute: completion)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's already executing on the main queue, right?

@@ -113,7 +133,7 @@ final class PhotosInputDataProvider: NSObject, PhotosInputDataProviderProtocol,
request.handleCompletion(with: result)
}
request.cancelBlock = { [weak self] in
self?.imageManager.cancelImageRequest(requestId)
self?.imageManager?.cancelImageRequest(requestId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the image manager be captured by this closure instead of self?

@codecov-io
Copy link

codecov-io commented Apr 2, 2019

Codecov Report

Merging #565 into master will decrease coverage by 0.36%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
- Coverage   59.97%   59.61%   -0.37%     
==========================================
  Files          87       87              
  Lines        3945     3969      +24     
==========================================
  Hits         2366     2366              
- Misses       1579     1603      +24
Impacted Files Coverage Δ
...e/Input/Photos/Photo/PhotosInputDataProvider.swift 0% <0%> (ø) ⬆️
...dditions/Source/Input/Photos/PhotosInputView.swift 1.69% <0%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5995970...8813401. Read the comment docs.

sSelf.collectionView.reloadData()
DispatchQueue.main.async(execute: completion)
sSelf.collectionViewQueue.addTask { [weak self] (completion) in
guard let sSelf = self else { return }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should a completion be called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't before, I decided not to change it

@leonspok
Copy link
Contributor Author

leonspok commented Apr 3, 2019

Closed as duplicate of #566

@leonspok leonspok closed this Apr 3, 2019
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.

3 participants