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

A couple performance tweaks for animated images #trivial #634

Merged
merged 5 commits into from
Dec 4, 2017
Merged

A couple performance tweaks for animated images #trivial #634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ebfa855
A couple performance tweaks for animated images
garrettmoon Oct 23, 2017
1fcd529
@nguyenhuy's comments
garrettmoon Nov 17, 2017
4d020db
Avoid calling animatedImageData twice. Thanks @maicki.
garrettmoon Dec 4, 2017
1f300ad
Fix call to background deallocation
garrettmoon Dec 4, 2017
bdf684c
Good catch by @Adlai-Holler
garrettmoon Dec 4, 2017
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
Next Next commit
A couple performance tweaks for animated images
  • Loading branch information
@garrettmoon
garrettmoon committed Dec 4, 2017
commit ebfa855262169cfd6946cd9d6dfdf341bf9476b9
8 changes: 8 additions & 0 deletions Source/ASImageNode+AnimatedImage.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ - (void)_locked_setAnimatedImage:(id <ASAnimatedImageProtocol>)animatedImage
if (ASObjectIsEqual(_animatedImage, animatedImage)) {
return;
}

if (animatedImage == nil) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this check. Shouldn't we do this whenever _animatedImage != nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeeeeees, good catch.

// Animated image can take while to dealloc, do it off the main queue
__block id <ASAnimatedImageProtocol>deallocImage = _animatedImage;
ASPerformBlockOnBackgroundThread(^{
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, let's use ASPerformBackgroundDeallocation(). Also, let's use previousAnimatedImage declared a few lines below.

Copy link
Member

Choose a reason for hiding this comment

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

Lastly, I think it's easier to reason if we do this at the end of this method.

deallocImage = nil;
});
}

id <ASAnimatedImageProtocol> previousAnimatedImage = _animatedImage;
_animatedImage = animatedImage;
Expand Down
103 changes: 57 additions & 46 deletions Source/ASNetworkImageNode.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,55 +713,66 @@ - (void)_lazilyLoadImageIfNecessary
} else {
__weak __typeof__(self) weakSelf = self;
auto finished = ^(id <ASImageContainerProtocol>imageContainer, NSError *error, id downloadIdentifier, ASNetworkImageSource imageSource) {

__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}

as_log_verbose(ASImageLoadingLog(), "Downloaded image for %@ img: %@ urls: %@", self, [imageContainer asdk_image], URLs);

// Grab the lock for the rest of the block
ASDN::MutexLocker l(strongSelf->__instanceLock__);

//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;
}
ASPerformBlockOnBackgroundThread(^{
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, the only change here is that the (previous) finished block is now executed on a background thread? (it's a bit hard to read)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly. Everything before was happening on main. Now it's being dispatched to the background and once it's done the work, the delegate callbacks are dispatched to main.

__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}

//No longer in preload range, no point in setting the results (they won't be cleared in exit preload range)
if (ASInterfaceStateIncludesPreload(self->_interfaceState) == NO) {
return;
}

if (imageContainer != nil) {
[strongSelf _locked_setCurrentImageQuality:1.0];
if ([imageContainer asdk_animatedImageData] && strongSelf->_downloaderFlags.downloaderImplementsAnimatedImage) {
id animatedImage = [strongSelf->_downloader animatedImageWithData:[imageContainer asdk_animatedImageData]];
[strongSelf _locked_setAnimatedImage:animatedImage];
} else {
[strongSelf _locked__setImage:[imageContainer asdk_image]];
as_log_verbose(ASImageLoadingLog(), "Downloaded image for %@ img: %@ urls: %@", self, [imageContainer asdk_image], URLs);

// Grab the lock for the rest of the block
ASDN::MutexLocker l(strongSelf->__instanceLock__);

//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;
}
strongSelf->_imageLoaded = YES;
}

strongSelf->_downloadIdentifier = nil;
strongSelf->_cacheUUID = nil;

if (imageContainer != nil) {
if (strongSelf->_delegateFlags.delegateDidLoadImageWithInfo) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
ASNetworkImageNodeDidLoadInfo info = {};
info.imageSource = imageSource;
[delegate imageNode:strongSelf didLoadImage:strongSelf.image info:info];
} else if (strongSelf->_delegateFlags.delegateDidLoadImage) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didLoadImage:strongSelf.image];

//No longer in preload range, no point in setting the results (they won't be cleared in exit preload range)
if (ASInterfaceStateIncludesPreload(self->_interfaceState) == NO) {
return;
}
} else if (error && strongSelf->_delegateFlags.delegateDidFailWithError) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didFailWithError:error];
}

if (imageContainer != nil) {
[strongSelf _locked_setCurrentImageQuality:1.0];
if ([imageContainer asdk_animatedImageData] && strongSelf->_downloaderFlags.downloaderImplementsAnimatedImage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could move up calling asdk_animatedImageData and assign it to a variable so you don't have to call it twice if this evaluates to true. Don't know how heavy asdk_animatedImageData is though.

id animatedImage = [strongSelf->_downloader animatedImageWithData:[imageContainer asdk_animatedImageData]];
[strongSelf _locked_setAnimatedImage:animatedImage];
} else {
[strongSelf _locked__setImage:[imageContainer asdk_image]];
}
strongSelf->_imageLoaded = YES;
}

strongSelf->_downloadIdentifier = nil;
strongSelf->_cacheUUID = nil;

ASPerformBlockOnMainThread(^{
__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
return;
}

// Grab the lock for the rest of the block
ASDN::MutexLocker l(strongSelf->__instanceLock__);

if (imageContainer != nil) {
if (strongSelf->_delegateFlags.delegateDidLoadImageWithInfo) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
ASNetworkImageNodeDidLoadInfo info = {};
info.imageSource = imageSource;
[delegate imageNode:strongSelf didLoadImage:strongSelf.image info:info];
} else if (strongSelf->_delegateFlags.delegateDidLoadImage) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didLoadImage:strongSelf.image];
}
} else if (error && strongSelf->_delegateFlags.delegateDidFailWithError) {
ASDN::MutexUnlocker u(strongSelf->__instanceLock__);
[delegate imageNode:strongSelf didFailWithError:error];
}
});
});
};

// As the _cache and _downloader is only set once in the intializer we don't have to use a
Expand Down