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

Increased default maxConcurrentOperationCount, fixes #527 #897

Merged
merged 1 commit into from
Nov 2, 2014

Conversation

MrAlek
Copy link
Contributor

@MrAlek MrAlek commented Sep 14, 2014

The default maxConcurrentOperationCount is set way too low in the image downloader (see issue #527). By increasing it, table/collection view performance is drastically improved. Especially when downloading images of mixed file sizes, as in the demo project.

@bpoplauschi
Copy link
Member

I think this is a value people should set on their own. Our default value must be a middle one.

@MrAlek
Copy link
Contributor Author

MrAlek commented Nov 2, 2014

Still, a good default value is one that fits the most common use cases and loading images in cells is arguable on of the top ones.

People are having performance problems because it is difficult to draw the conclusion that the maxConcurrentOperationCount needs to be set higher. Myself, I nearly opted out of using SDWebImage when benchmarking it against my regular network library before finding out that there weren't enough concurrency.

Would you feel more comfortable with a lower value, like 6?

@rs
Copy link
Member

rs commented Nov 2, 2014

Ok for me

@bpoplauschi
Copy link
Member

I agree as well.

@bpoplauschi bpoplauschi added this to the 4.0.0 milestone Nov 2, 2014
@MrAlek
Copy link
Contributor Author

MrAlek commented Nov 2, 2014

Lowered it to 6

@bpoplauschi
Copy link
Member

Thanks for the quick turnaround. Merging

bpoplauschi added a commit that referenced this pull request Nov 2, 2014
Increased default maxConcurrentOperationCount, fixes #527
@bpoplauschi bpoplauschi merged commit 759f0b9 into SDWebImage:master Nov 2, 2014
@MrAlek MrAlek deleted the fix/max-concurrency-count branch November 2, 2014 17:34
@rromanchuk
Copy link
Contributor

Guys, did you profile this change before making it? It should really be the responsibility of the client to cancel requests that are no longer valid (cell goes off screen) or manually modifying this default if their application best fits a large number of concurrent connections...like a table view with many small images.

However, changing a very reasonable default of 2 to 6 could have drastic consequences for an application that is downloading very large images. Imagine what this could mean in terms of memory pressure for a user that has a fast connection and downloading full screen retina images.

This change may have contributed to the 15 or so tickets that are currently open due to extreme memory pressure SDWebImage is putting on devices

@MrAlek
Copy link
Contributor Author

MrAlek commented Feb 22, 2015

I would say that applications that concurrently shows many very large images is not a typical use case compared to images in table & collection views. One should optimize for the common case. If you're queuing up several full retina images, it's not wise to decompress all of them before actually showing them anyway.

The memory issues in SDWebImage have been around since long before this fix and are related to live allocations building up over time, not temporal spikes, why this change doesn't affect those issues.

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