CODEWITHSUNDEEP
objective-ciosuitableviewconcurrencygrand-central-dispatch
Sandeep
Sandeep

Reputation: 21154

Grand central dispatch and concurrency issue

I have a table view with an image that is being loaded lazily using grand central dispatch. I have used an async queue with two serial queues inside, the first one to download the image and the second queue to set the image to the cell. This method seems to have a laggin behavior in scrolling.

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

        __block UIImage *image = nil;
        dispatch_sync(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
            image = [UIImage imageWithData:[NSData dataWithContentsOfURL:[NSURL URLWithString:artist.imImage]]];
        });
        dispatch_sync(dispatch_get_main_queue(), ^{
            cell.artistImage.image = image;

        });

    });

And then I tried with a single asynchronous queue to download image and then get the main queue inside and set image. Even I dont feel this method being much suitable. I think I am missing something here. 

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
        UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:[NSURL URLWithString:artist.imImage]]];
        dispatch_async(dispatch_get_main_queue(), ^{
            cell.artistImage.image = image;
        });

    });

Is it like I am missing something here or is it some other problem ?

Upvotes: 1

Views: 1271

Answers (7)

EarlyRiser
EarlyRiser

Reputation: 736

I think what you should do is use the SDWebImage library: https://github.com/rs/SDWebImage

It will do all this for you and cache the resulting images locally as well as taking care of rejecting LRU image from the cache...

It's pretty fantastic.

Upvotes: 0

Christopher Pickslay
Christopher Pickslay

Reputation: 17772

I suspect the performance issue you're seeing is because you're spawning unnecessary network activity.

I would use a dictionary to cache the images. And instead of loading images whenever cells are dequeued, I'd do on viewDidLoad (or whenever your data model is initialized), and on scrollViewDidEndDecelerating:. Otherwise, you're queueing up image requests for all those cells the user just scrolled past.

-(void)viewDidLoad {
    [super viewDidLoad];
    [self initializeData];
    self.images = [NSMutableDictionary dictionary];
    [self loadVisibleImages];
}

-(void)scrollViewDidEndDecelerating:(UIScrollView *)scrollView {
    if (scrollView == self.tableView) {
        [self loadVisibleImages];
    }
}

-(void)loadVisibleImages {
    for (ArtistCell *cell in [self.tableview visibleCells]) {
        NSURL *artistURL = [NSURL URLWithString:cell.artist.imImage];
        UIImage *artistImage = [self.images objectForKey:artistURL];
        if (artistImage) {
            cell.artistImage.image = artistImage;
        } else {
            cell.artistImage.image = self.loadingImage;
            dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
                UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:[NSURL URLWithString:artist.imImage]]];
                [self.images setObject:image forKey:artistURL];
                dispatch_sync(dispatch_get_main_queue(), ^{
                    [cell setNeedsLayout];
                });
            });
        }
    }
}

-(UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    ... dequeue the cell, do all your other setup
    NSURL *artistURL = [NSURL URLWithString:cell.artist.imImage];
    UIImage *artistImage = [self.images objectForKey:artistURL];
    if (artistImage) {
        cell.artistImage.image = artistImage;
    } else {
        cell.artistImage.image = self.loadingImage;
    }
}

Upvotes: 1

deleted_user
deleted_user

Reputation: 3805

Unfortunately, you should not be using grand central to update the UI in this fashion. Grand central dispatch blocks the message queue for the main thread, even though its not supposed to.

Try this to prove my point, take your asynch code, and rather than use dispatch_async, do a simple message dispatch, such as 

[self performSelector:@selector(blabla:) withObject:nil afterDelay:0]

You will find that your code is performing fine, but GCD is actually stalling the message loop to the GUI.

Had this happen using GCD on background processes sometimes it works sometimes not.

Mostly when you are already in a GUI thread (as in user presses button action) GCD will severely delay GUI updates until its done.

Learning how runloops work in iOS is much more productive than throwing everything to GCD.

Pitch it.

Upvotes: 1

David Hoerl
David Hoerl

Reputation: 41652

Your second code snippet is the way to go. I believe Stephen Darlington gave you the reasons for your problems, and I want to suggest a different way to architect your code (as its the way I do it and its working very well right now).

So when a cell needs an image, it asks some data manager for it. If the image is there it gets returned. If its not there then a fetch operation is initiated. When images arrive and get created, you should not try to set a cell, but post a notification on the main thread when the image is saved in your cache (a NSDictionary is one way to save them, with the URL or name as key).

The object controlling your tableView listens for the notifications. When one arrives, it looks at all the visible cells, and if one is missing an image and it just arrived, the image gets updated. Otherwise it does nothing.

When the table scrolls, and the tableView asks for a new cell, you always see if the image is in the cache, and if so, use it. Otherwise you make a request for the image.

To avoid multiple requests for an image (web fetches), when you make one you need to note that fact. A way to do this is put the imageName or the URL into a NSMutableSet when you start the fetch, and remove it when you get the image.

This is a bit more work that what you are doing but the result is a very smooth and responsive GUI.

Upvotes: 1

justin
justin

Reputation: 104708

I did this, but I used NSOperation and NSOperationQueue in my implementation. When the cell went offscreen, it no longer needed the image, so it cancelled the async download+expand NSOperation. The queue had a pretty low maximum (restricts memory demands as well). It was very smooth once tuned (and after the images on the server were the right sizes).

Consider also caching what you can, if that does not consume a terrible amount of space.

Upvotes: 1

Stephen Darlington
Stephen Darlington

Reputation: 52575

The second solution you have is the way to go -- there's no point in creating more tasks than is necessary (where the value of "necessary" varies depending on what you want to do of course).

However, one problem you might see:

  1. The cell appears
  2. You request the image in your GCD task
  3. The user scrolls up
  4. The cell gets re-used
  5. You request the new image in your GCD task
  6. The original request (from step 2) finishes and updates the cell with the "old" image

You need to check the cell is still "current" before you update with the image.

Upvotes: 1

sergio
sergio

Reputation: 69047

UIKit is not thread-safe, so you should queue all UIKit operations to the main queue. It is not clear what kind of problem you have from the use of your second solution:

    UIImage *image = [UIImage imageWithData:[NSData dataWithContentsOfURL:[NSURL URLWithString:artist.imImage]]];
    dispatch_async(dispatch_get_main_queue(), ^{
        cell.artistImage.image = image;
    });

but I would suggest to instantiate the UIImage in the main thread:

    NSData* data = [NSData dataWithContentsOfURL:[NSURL URLWithString:artist.imImage]];
    dispatch_async(dispatch_get_main_queue(), ^{
        UIImage *image = [UIImage imageWithData:data];
        cell.artistImage.image = image;
    });

Upvotes: 2

Related Questions