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.

Sign up for GitHub

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

Jump to bottom

Experiment with different strategies for image downloader priority #1349

Merged
merged 5 commits into from
Mar 8, 2019
Merged

Experiment with different strategies for image downloader priority #1349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0454fd6
Experiment with different strategies for image downloader priority
nguyenhuy Feb 20, 2019
1df0049
Add imports
nguyenhuy Feb 23, 2019
3848174
Fix build failure
nguyenhuy Feb 23, 2019
6a1f368
Encapsulate common logics into methods
nguyenhuy Feb 23, 2019
d7413c8
Address comments
nguyenhuy Mar 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 16 additions & 23 deletions Podfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,32 @@
PODS:
- FBSnapshotTestCase/Core (2.1.4)
- FLAnimatedImage (1.0.12)
- JGMethodSwizzler (2.0.1)
- OCMock (3.4.1)
- PINCache (3.0.1-beta.6):
- PINCache/Arc-exception-safe (= 3.0.1-beta.6)
- PINCache/Core (= 3.0.1-beta.6)
- PINCache/Arc-exception-safe (3.0.1-beta.6):
- PINCache (3.0.1-beta.7):
- PINCache/Arc-exception-safe (= 3.0.1-beta.7)
- PINCache/Core (= 3.0.1-beta.7)
- PINCache/Arc-exception-safe (3.0.1-beta.7):
- PINCache/Core
- PINCache/Core (3.0.1-beta.6):
- PINOperation (~> 1.1.0)
- PINCache/Core (3.0.1-beta.7):
- PINOperation (~> 1.1.1)
- PINOperation (1.1.1)
- PINRemoteImage (3.0.0-beta.13):
- PINRemoteImage/FLAnimatedImage (= 3.0.0-beta.13)
- PINRemoteImage/PINCache (= 3.0.0-beta.13)
- PINRemoteImage/Core (3.0.0-beta.13):
- PINRemoteImage (3.0.0-beta.14):
- PINRemoteImage/PINCache (= 3.0.0-beta.14)
- PINRemoteImage/Core (3.0.0-beta.14):
- PINOperation
- PINRemoteImage/FLAnimatedImage (3.0.0-beta.13):
- FLAnimatedImage (>= 1.0)
- PINRemoteImage/Core
- PINRemoteImage/PINCache (3.0.0-beta.13):
- PINCache (= 3.0.1-beta.6)
- PINRemoteImage/PINCache (3.0.0-beta.14):
- PINCache (= 3.0.1-beta.7)
- PINRemoteImage/Core

DEPENDENCIES:
- FBSnapshotTestCase/Core (~> 2.1)
- JGMethodSwizzler (from `https://github.com/JonasGessner/JGMethodSwizzler`, branch `master`)
- OCMock (= 3.4.1)
- PINRemoteImage (= 3.0.0-beta.13)
- PINRemoteImage (= 3.0.0-beta.14)

SPEC REPOS:
https://github.com/cocoapods/specs.git:
- FBSnapshotTestCase
- FLAnimatedImage
- OCMock
- PINCache
- PINOperation
Expand All @@ -50,13 +44,12 @@ CHECKOUT OPTIONS:

SPEC CHECKSUMS:
FBSnapshotTestCase: 094f9f314decbabe373b87cc339bea235a63e07a
FLAnimatedImage: 4a0b56255d9b05f18b6dd7ee06871be5d3b89e31
JGMethodSwizzler: 7328146117fffa8a4038c42eb7cd3d4c75006f97
OCMock: 2cd0716969bab32a2283ff3a46fd26a8c8b4c5e3
PINCache: d195fdba255283f7e9900a55e3cced377f431f9b
PINCache: 7cb9ae068c8f655717f7c644ef1dff9fd573e979
PINOperation: a6219e6fc9db9c269eb7a7b871ac193bcf400aac
PINRemoteImage: d6d51c5d2adda55f1ce30c96e850b6c4ebd2856a
PINRemoteImage: 81bbff853acc71c6de9e106e9e489a791b8bbb08

PODFILE CHECKSUM: 42715d61f73cc22cc116bf80d7b268cb1f9e4742
PODFILE CHECKSUM: 445046ac151568c694ff286684322273f0b597d6

COCOAPODS: 1.5.3
COCOAPODS: 1.6.0
3 changes: 2 additions & 1 deletion Schemas/configuration.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
"exp_disable_a11y_cache",
"exp_skip_a11y_wait",
"exp_new_default_cell_layout_mode",
"exp_dispatch_apply"
"exp_dispatch_apply",
"exp_image_downloader_priority"
]
}
}
Expand Down
1 change: 1 addition & 0 deletions Source/ASExperimentalFeatures.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
ASExperimentalSkipAccessibilityWait = 1 << 10, // exp_skip_a11y_wait
ASExperimentalNewDefaultCellLayoutMode = 1 << 11, // exp_new_default_cell_layout_mode
ASExperimentalDispatchApply = 1 << 12, // exp_dispatch_apply
ASExperimentalImageDownloaderPriority = 1 << 13, // exp_image_downloader_priority
ASExperimentalFeatureAll = 0xFFFFFFFF
};

Expand Down
5 changes: 3 additions & 2 deletions Source/ASExperimentalFeatures.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@
@"exp_disable_a11y_cache",
@"exp_skip_a11y_wait",
@"exp_new_default_cell_layout_mode",
@"exp_dispatch_apply"]));

@"exp_dispatch_apply",
@"exp_image_downloader_priority"]));

if (flags == ASExperimentalFeatureAll) {
return allNames;
}
Expand Down
164 changes: 104 additions & 60 deletions Source/ASMultiplexImageNode.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,13 @@ @interface ASMultiplexImageNode ()
@private
// Core.
id<ASImageCacheProtocol> _cache;

id<ASImageDownloaderProtocol> _downloader;
struct {
unsigned int downloaderImplementsSetProgress:1;
unsigned int downloaderImplementsSetPriority:1;
unsigned int downloaderImplementsDownloadWithPriority:1;
} _downloaderFlags;

__weak id<ASMultiplexImageNodeDelegate> _delegate;
struct {
Expand Down Expand Up @@ -89,8 +95,6 @@ @interface ASMultiplexImageNode ()
BOOL _shouldRenderProgressImages;

//set on init only
BOOL _downloaderImplementsSetProgress;
BOOL _downloaderImplementsSetPriority;
BOOL _cacheSupportsClearing;
}

Expand Down Expand Up @@ -171,13 +175,14 @@ - (instancetype)initWithCache:(id<ASImageCacheProtocol>)cache downloader:(id<ASI
_cache = (id<ASImageCacheProtocol>)cache;
_downloader = (id<ASImageDownloaderProtocol>)downloader;

_downloaderImplementsSetProgress = [downloader respondsToSelector:@selector(setProgressImageBlock:callbackQueue:withDownloadIdentifier:)];
_downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
_downloaderFlags.downloaderImplementsSetProgress = [downloader respondsToSelector:@selector(setProgressImageBlock:callbackQueue:withDownloadIdentifier:)];
_downloaderFlags.downloaderImplementsSetPriority = [downloader respondsToSelector:@selector(setPriority:withDownloadIdentifier:)];
_downloaderFlags.downloaderImplementsDownloadWithPriority = [downloader respondsToSelector:@selector(downloadImageWithURL:priority:callbackQueue:downloadProgress:completion:)];

_cacheSupportsClearing = [cache respondsToSelector:@selector(clearFetchedImageFromCacheWithURL:)];

_shouldRenderProgressImages = YES;

self.shouldBypassEnsureDisplay = YES;

return self;
Expand Down Expand Up @@ -271,49 +276,34 @@ - (BOOL)placeholderShouldPersist
- (void)displayWillStartAsynchronously:(BOOL)asynchronously
{
[super displayWillStartAsynchronously:asynchronously];

[self didEnterPreloadState];

if (_downloaderImplementsSetPriority) {
{
ASDN::MutexLocker l(_downloadIdentifierLock);
if (_downloadIdentifier != nil) {
[_downloader setPriority:ASImageDownloaderPriorityImminent withDownloadIdentifier:_downloadIdentifier];
}
}
}
[self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityImminent];
}

/* didEnterVisibleState / didExitVisibleState in ASNetworkImageNode has a very similar implementation. Changes here are likely necessary
in ASNetworkImageNode as well. */
- (void)didEnterVisibleState
{
[super didEnterVisibleState];

if (_downloaderImplementsSetPriority) {
ASDN::MutexLocker l(_downloadIdentifierLock);
if (_downloadIdentifier != nil) {
[_downloader setPriority:ASImageDownloaderPriorityVisible withDownloadIdentifier:_downloadIdentifier];
}
}

[self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityVisible];
[self _updateProgressImageBlockOnDownloaderIfNeeded];
}

- (void)didExitVisibleState
{
[super didExitVisibleState];

if (_downloaderImplementsSetPriority) {
ASDN::MutexLocker l(_downloadIdentifierLock);
if (_downloadIdentifier != nil) {
[_downloader setPriority:ASImageDownloaderPriorityPreload withDownloadIdentifier:_downloadIdentifier];
}
}

[self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityPreload];
Copy link
Contributor

Choose a reason for hiding this comment

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

Just reminder that in thrash case (without coalescing) a view may exit and enter immediately. It seems ok in this case though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I think the right fix is coalescing in such case to avoid jumping between interface states unnecessarily.

[self _updateProgressImageBlockOnDownloaderIfNeeded];
}

- (void)didExitDisplayState
{
[super didExitDisplayState];
if (ASActivateExperimentalFeature(ASExperimentalImageDownloaderPriority)) {
[self _updatePriorityOnDownloaderIfNeededWithDefaultPriority:ASImageDownloaderPriorityPreload];
}
}

#pragma mark - Core

- (void)setImage:(UIImage *)image
Expand Down Expand Up @@ -449,7 +439,6 @@ - (void)_setDownloadIdentifier:(id)downloadIdentifier
_downloadIdentifier = downloadIdentifier;
}


#pragma mark - Image Loading Machinery

- (void)_loadImageIdentifiers
Expand Down Expand Up @@ -493,19 +482,37 @@ - (UIImage *)_bestImmediatelyAvailableImageFromDataSource:(id *)imageIdentifierO

#pragma mark -

/**
@note: This should be called without _downloadIdentifierLock held. We will lock
super to read our interface state and it's best to avoid acquiring both locks.
*/
- (void)_updatePriorityOnDownloaderIfNeededWithDefaultPriority:(ASImageDownloaderPriority)defaultPriority
{
ASAssertUnlocked(_downloadIdentifierLock);

if (_downloaderFlags.downloaderImplementsSetPriority) {
// Read our interface state before locking so that we don't lock super while holding our lock.
ASInterfaceState interfaceState = self.interfaceState;
ASDN::MutexLocker l(_downloadIdentifierLock);

if (_downloadIdentifier != nil) {
ASImageDownloaderPriority priority = defaultPriority;
if (ASActivateExperimentalFeature(ASExperimentalImageDownloaderPriority)) {
priority = ASImageDownloaderPriorityWithInterfaceState(interfaceState);
}

[_downloader setPriority:priority withDownloadIdentifier:_downloadIdentifier];
}
}
}

- (void)_updateProgressImageBlockOnDownloaderIfNeeded
{
ASAssertUnlocked(_downloadIdentifierLock);

BOOL shouldRenderProgressImages = self.shouldRenderProgressImages;

// Read our interface state before locking so that we don't lock super while holding our lock.
ASInterfaceState interfaceState = self.interfaceState;
ASDN::MutexLocker l(_downloadIdentifierLock);

if (!_downloaderImplementsSetProgress || _downloadIdentifier == nil) {
if (!_downloaderFlags.downloaderImplementsSetProgress || _downloadIdentifier == nil) {
return;
}

Expand Down Expand Up @@ -825,7 +832,7 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
[_delegate multiplexImageNode:self didStartDownloadOfImageWithIdentifier:imageIdentifier];

__weak __typeof__(self) weakSelf = self;
void (^downloadProgressBlock)(CGFloat) = nil;
ASImageDownloaderProgress downloadProgressBlock = NULL;
if (_delegateFlags.downloadProgress) {
downloadProgressBlock = ^(CGFloat progress) {
__typeof__(self) strongSelf = weakSelf;
Expand All @@ -835,30 +842,67 @@ - (void)_downloadImageWithIdentifier:(id)imageIdentifier URL:(NSURL *)imageURL c
};
}

ASImageDownloaderCompletion completion = ^(id <ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) {
// We dereference iVars directly, so we can't have weakSelf going nil on us.
__typeof__(self) strongSelf = weakSelf;
if (!strongSelf)
return;

ASDN::MutexLocker l(strongSelf->_downloadIdentifierLock);
//Getting a result back for a different download identifier, download must not have been successfully canceled
if (ASObjectIsEqual(strongSelf->_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
return;
}

completionBlock([imageContainer asdk_image], error);

// Delegateify.
if (strongSelf->_delegateFlags.downloadFinish)
[strongSelf->_delegate multiplexImageNode:weakSelf didFinishDownloadingImageWithIdentifier:imageIdentifier error:error];
};

// Download!
ASPerformBlockOnBackgroundThread(^{
[self _setDownloadIdentifier:[_downloader downloadImageWithURL:imageURL
callbackQueue:dispatch_get_main_queue()
downloadProgress:downloadProgressBlock
completion:^(id <ASImageContainerProtocol> imageContainer, NSError *error, id downloadIdentifier, id userInfo) {
// We dereference iVars directly, so we can't have weakSelf going nil on us.
__typeof__(self) strongSelf = weakSelf;
if (!strongSelf)
return;

ASDN::MutexLocker l(_downloadIdentifierLock);
//Getting a result back for a different download identifier, download must not have been successfully canceled
if (ASObjectIsEqual(_downloadIdentifier, downloadIdentifier) == NO && downloadIdentifier != nil) {
return;
}

completionBlock([imageContainer asdk_image], error);

// Delegateify.
if (strongSelf->_delegateFlags.downloadFinish)
[strongSelf->_delegate multiplexImageNode:weakSelf didFinishDownloadingImageWithIdentifier:imageIdentifier error:error];
}]];
[self _updateProgressImageBlockOnDownloaderIfNeeded];
__typeof__(self) strongSelf = weakSelf;
if (!strongSelf)
return;

dispatch_queue_t callbackQueue = dispatch_get_main_queue();

id downloadIdentifier;
if (strongSelf->_downloaderFlags.downloaderImplementsDownloadWithPriority
&& ASActivateExperimentalFeature(ASExperimentalImageDownloaderPriority)) {

/*
Decide a priority based on the current interface state of this node.
It can happen that this method was called when the node entered preload state
but the interface state, at this point, tells us that the node is (going to be) visible,
If that's the case, we jump to a higher priority directly.
*/
ASImageDownloaderPriority priority = ASImageDownloaderPriorityWithInterfaceState(strongSelf.interfaceState);
downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
priority:priority
callbackQueue:callbackQueue
downloadProgress:downloadProgressBlock
completion:completion];
} else {
/*
Kick off a download with default priority.
The actual "default" value is decided by the downloader.
ASBasicImageDownloader and ASPINRemoteImageDownloader both use ASImageDownloaderPriorityImminent
which is mapped to NSURLSessionTaskPriorityDefault.

This means that preload and display nodes use the same priority
and their requests are put into the same pool.
*/
downloadIdentifier = [strongSelf->_downloader downloadImageWithURL:imageURL
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call this we a priority also so that it do not need to be called differently in if-else?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to let the concrete downloader to decide what the default priority is. Right now both ASBasicImageDownloader and ASPINRemoteImageDownloader have the same default value, but other custom ones may have a different one.

callbackQueue:callbackQueue
downloadProgress:downloadProgressBlock
completion:completion];
}

[strongSelf _setDownloadIdentifier:downloadIdentifier];
[strongSelf _updateProgressImageBlockOnDownloaderIfNeeded];
});
}

Expand Down
Loading