Multithreading Issue

[ghost]

Hi Dave - I seem to have come across an odd issue dealing with multithreading that results in Image View hanging.

Whenever there are multiple threads active in the app performing other tasks (eg running multiple network threads) it seems the Image View will hang and not load an image until previous threads complete or free up.

What I believe may be happening (pl. feel free to correct) is that the Image View internally uses AsyncTask - which has limited number of concurrent threads per app domain that can run depending on SDK version. I believe its around 5 threads, subsequent threads will be queued.

This could be a bit problematic for apps with lots of processing. What makes it worse is that the problem will not be evident on some SDKs/devices. There's also very little gained by going with AsyncTask VS. using Threads directly, other than syntactic convenience I suppose :)

Would it be possible to replace AsyncTask in the next library update ?

Thanks!

[davemorrissey]

I'm reluctant to do this unless I have to. This is the first problem like this that's been reported, and up to now it's been rock solid with AsyncTasks for a lot of users, so a move to threads would be a big step into the unknown. Besides the convenience, they are the standard method of doing work off the UI thread and they provide the concurrency control this view needs - I can safely queue up requests to load tiles even while the user is rapidly panning and zooming, and not worry if the activity is paused or stopped. It may actually be desirable that image loading is paused while the app is doing a lot of work in the background.

One option may be to execute AsyncTasks on the provided THREAD_POOL_EXECUTOR. This seems to work, but I don't know if BitmapRegionDecoder is guaranteed to provide thread safe decoding of images from all sources.

[ghost]

Actually AsyncTask is known to have many pitfalls and problems. You will find lots of material on this.

The issue I described can easily be reproduced by starting 4-5 async tasks, and then trying to use the Image View. You will see it will hang and not load any images.

[davemorrissey]

The worst issues with AsyncTasks either don't apply to the way I'm using them here, or I've already worked around using static inner classes, weak references and state checks. Implementing my own threading would require solving these problems and more, and I'm not keen on spawning threads within a view. Sharing the threads Android has already created for the app makes more sense. I'll consider making the parallel executor available as an option, which should solve this problem.

[ghost]

Just to update on the issue.

Now I am able to reproduce the issue even by spawning a single network thread (even without AsyncTask) and then trying to load the image file. As long as the thread is running the image will not load up.

So with this new bit of detail I'm not sure if a parallel executor would solve this. Ripping out AsyncTask and replacing with Threads and Handlers typically isn't that trivial. The only trick is not leaking memory but that's easily handled by making handlers static (if inner classes) and using WeakReferences for private members of the handler. Of course you know your logic better and what it would require but I did it for my whole app in about 2 days, I originally used async tasks but ran into issues.

Additionally I think this actually effects more people than we first assumed earlier in this post. Having a network thread run in the background is very common for modern apps now, syncing stuff and all. Likely reason why this was not reported much is because the issue just manifests itself as a slow load in most cases as the network thread will eventually will timeout, and the image will load right after.

Thanks for looking into this.

[ghost]

FYI for now I'm only seeing this on some test devices. I'll do another test targeting more devices tomorrow and post findings.

[davemorrissey]

Why wouldn't a parallel executor solve this? It would allow the view to load images on the parallel executor even while other AsyncTasks are using the single threaded executor. There would only be a delay if the parallel executor is busy running several other tasks, and in that scenario it might be desirable to avoid spawning more threads - besides the CPU load it increases the chances of getting an OOME while loading.

[davemorrissey]

I've pushed a new release (3.3.0) with support for AsyncTask.THREAD_POOL_EXECUTOR on SDK 11+. Give this a try and let me know if it helps. Call this before setting the image source:

imageView.setParallelLoadingEnabled(true);

[davemorrissey]

Any luck with this, can I close the bug?

[franciscofranco]

Sorry for bringing this from the dead, but instead of using reflection to access the POOL_EXECUTOR you can use https://github.com/android/platform_frameworks_support/blob/master/v4/java/android/support/v4/os/AsyncTaskCompat.java probably saving some overhead from the reflection.

[davemorrissey]

There is no dependency on support-v4 at the moment and I don't want to add one for this. The overhead of reflection is tiny.

[AlexVPerl]

Hi Dave - revisiting this after a long pause.

AsyncTask still remains an issue blocking images from loading if other AsyncTasks are running. The limitation is that only max of 5 tasks can run simultaneously.

AsyncTask.THREAD_POOL_EXECUTOR does not resolve this issue. All it does is execute tasks in parallel instead of serial order. It does nothing to address the limit of 5 tasks, rest will be queued.

I'm still constantly running into issues where images cannot be loaded for a while because other AsyncTasks are running.

Would it be possible to address this in near future by replacing AsyncTasks with regular Threads?

Thanks in advance!

[davemorrissey]

I think I'd have concerns about using an unbounded number of threads to load tiles, but it would be possible to send the AsyncTasks to a custom executor with more threads available, rather than rewriting the tasks as threads. I'd consider a PR that allows an Executor instance to be supplied, but would rather not have the view spawning and managing threads, especially because some apps have several on screen at once.

Are the executor's threads being kept busy by other apps? Simultaneously loading images on more threads than there are CPU cores is unlikely to make the view run faster. In my experience, with my phone probably not doing any significant background work, enabling parallel loading does nothing for performance.

[franciscofranco]

For the past couple months I've been using a dedicated Executor for AsyncTasks instead of the POOL_EXECUTOR for this exact reason. I use Executor.newCachedThreadPool() because it spawns whatever threads it needs, re-uses previously cached Threads and if they aren't used for 60 seconds they are destroyed.

Regarding your N5X, it's an odd case because it's very likely these jobs aren't queued into the A57 cores, but to the A53 cores which are ridiculously underpowered. I'll guess that the task scheduler will actively migrate threads to the A57 if there are a lot of them being created since the scheduler doesn't want to starve cores, so that might improve performance.

[davemorrissey]

I guess the sample app isn't a great real world example because it has no network loading tasks or any other background work going on, but I've always thought the tile loading performance is reasonable, with new tiles typically loaded in a second or so.

Without buying a large collection of phones I have nothing to compare the N5X to, but if anyone wants to submit screen captures of the sample app loading/panning/zooming the test card image, that would be interesting to see.

I won't spawn threads in the view but I would accept a PR that allowed a developer to supply one that can be shared and have a sensible lifecycle.

[franciscofranco]

I can do that sometime soon, I have like 20 different phones. I think the current tile loading performance is good but I also wonder if it can be better with a different Executor. The PR code is trivial, I'll play with it.

[franciscofranco]

A quick and dirty test (without any numbers to back it up) using a different Executor on the card.jpg of the sample app doesn't seem to make any real world perceptible difference on an One Plus 5, but then again this chipset is a monster.

[davemorrissey]

Yep I just tried the same, using Executors.newFixedThreadPool(10) made little to no difference. Tiles still seem to load at regular intervals, which makes me wonder whether BitmapRegionDecoder or Skia even support multiple threads.

What does make a noticeable difference is changing the code to load tiles during pan and zoom gestures, where currently it waits until the end of the gesture. In 2014 when I wrote this that made a big difference to performance, resulting in choppy movement, but on today's phones it may not.

[franciscofranco]

Without checking the code I wouldn't be surprised if those "interfaces" were single-threaded.
Yep, I always thought why you did that in the first place. If you change that behavior it'll be nothing short of amazing.

[davemorrissey]

Yeah, but I'm scared of changing anything now, every time I do the sample app works great on my phones but lots of developer's apps break :-)

[franciscofranco]

I make custom Kernels for Android devices, so I know your pain. I'll be more than happy to test that change, I have devices ranging from the Galaxy Nexus/N4 to the monstruous S8/Pixel/OP5.

[davemorrissey]

The get out of jail free card here is to make loading during gestures one of the configurable options, but I should add that while this does help a bit, gestures aren't usually long enough for it to make a big difference, and it will hurt performance on old/cheap devices.

The trouble with these options there's no one configuration that works well for all images on all phones, so it's hard to document recommended settings. The defaults of RGB_565, 320dpi, single threaded loading, and no loading during gestures apparently work for pretty much everyone without problems - complaints about performance are very rare considering this has 3.5K stars - but changing those defaults will cause crashes on older devices.

Keeping the current defaults and labelling the options with a "here be dragons" warning seems the best way to have a quiet life.

[davemorrissey]

Aha - see line 189: https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/graphics/java/android/graphics/BitmapRegionDecoder.java

There's nothing to gain with multiple threads, but there might be something to gain by providing a single threaded executor that isn't busy with other jobs.

[franciscofranco]

Interesting, I kind of expected it after seeing the results with other Executors. Yeah a dedicated single threaded Executor will be definitely be marginally better on devices with lots of AsyncTasks running in the background using the default serial executor.

[davemorrissey]

I've just hacked together what's effectively an object pool of 4 BitmapRegionDecoders, using a dumb round robin implementation. It would be complex to make this production ready and might cause all sorts of problems, but it does work beautifully.

[davemorrissey]

I'm coding up a solution now.

• Replace synchronized blocks with ReadWriteLocks so decoders can be used by multiple threads but locked for recycling by one. Currently there are synchronized blocks in TileLoadTask and SkiaImageRegionDecoder so single thread loading was enforced, but only to allow safe recycling. This made no difference to performance with BitmapRegionDecoder because it's also synchronized, but the view couldn't take advantage of a custom decoder that supports multi-threaded decoding. This is a breaking change for custom decoders that aren't expecting decodeRegion to be called from multiple threads.
• Replace setParallelLoadingEnabled(boolean) with setTileLoadingExecutor(Executor).
• New class SkiaPooledImageRegionDecoder which will create 4 decoders.

I'll push this to a branch and release it as a beta.

[franciscofranco]

Theoretically that sounds lovely! Why 4 decoders and not N where N = number of CPUs available?

[davemorrissey]

There's little benefit from having more than 4 because it's rare that more than 4 tiles become visible at the same moment. Capping it lower if N < 4 might be sensible.

However there's a problem with this plan. Each BitmapRegionDecoder uses native memory equivalent to the file size of the image being displayed. Loading the data once as a byte[] and creating multiple instances with isShareable=true doesn't help because the native code ignores the flag and copies the data.

My N5X was okay with 10 instances and a 5Mb image, so I guess 4 should be okay for most devices and most images. The pool will need to be smart about memory usage to preserve enough space for tiles so this needs more research.

[davemorrissey]

I've pushed a new release 3.9.0.ALPHA to maven (probably, sonatype is not working well today). This has the following options:

Load tiles during gestures and animations. Can be wasteful and reduce the frame rate on older devices, especially with ARGB_8888 images.

imageView.setEagerLoadingEnabled(true);

Use a custom executor for all loading of bitmaps. The executor should be shared by all instances and kept for the life of the app. The view will only effectively use one thread unless the ImageRegionDecoder class supports parallel loading, which the default one doesn't.

imageView.setExecutor(AsyncTask.THREAD_POOL_EXECUTOR);

A new and highly experimental pooling tile decoder. Caps estimated memory usage at 20Mb and four decoder instances, and delays creation of extra decoders until it looks like the user is interacting with the view. Works well for PNGs, but isn't a huge improvement for JPGs, at least on my phone.

imageView.setRegionDecoderClass(SkiaPooledImageRegionDecoder.class);

Default behaviour hasn't changed so it would be fairly safe to release this, but I have changed to ReadWriteLocks for internal synchronization so it would be good to get some feedback from real world apps before committing to these changes.

[franciscofranco]

Outstanding. Let me get those changes on my app and test a bit on different devices and I'll give you some feedback.

[franciscofranco]

My preliminary very unscientific test shows a near 2x faster decoding of tiles when zooming in, and I don't notice tiles catching up anymore when panning, they're just there, ready and high quality. This was tested on the Pixel. Will try on some half-assed devices like the Nexus 4, Mi A1, etc. But I'm really enjoying seeing these improvements right now.

[davemorrissey]

With a bit of hacking to restore SDK 10 support I can probably try my HTC Desire from 2010. That'll be a good acid test.

[franciscofranco]

That single core QSD8250 CPU won't be happy :)

[franciscofranco]

These changes are AWESOME!

[davemorrissey]

HTC Desire (2010, SDK 10): A valiant effort with images 40x its screen area, but occasional OOMEs and frequent corrupt bitmaps with default config, so I didn't try eager loading. SDK 10 only supports serial execution of AsyncTasks.

HTC One S (2012, SDK 16): With PNGs, eager loading and the pooled decoder both work well but do reduce the frame rate slightly. With JPGs, using a parallel executor causes segfaults. BitmapRegionDecoder did not have internal synchronization until Lollipop, and I've just stripped out the view's synchronization and exposed this problem. I'll restore it for SDK < 21. This was my original test device in 2013, which explains some design decisions.

Nexus 7 (2012, SDK 22): Eager loading and decoder pooling work reliably but do affect the frame rate some of the time, and combined with ARGB_8888 it's often quite unresponsive.

For each of eager loading, parallel loading and ARGB_8888 there's a decision to be made about whether the default should change. I think that eager loading and AsyncTask.THREAD_POOL_EXECUTOR (purely to reduce contention) could be safe defaults for SDK >= 21, but parallel loading and ARGB_8888 would cause poor performance and crashes for some devices, and should be options available for developers who want them.

[franciscofranco]

I quickly abandoned 8888 as the defaults on my app and leave it to the users to choose if they want to, but I lower the tile dpi to 240 and 160 if ActivityManager.isLowRamDevice(). I only use SDK >= 21 so I'm fine with these new features being default on that api level, and it works beautifully.

[franciscofranco]

Here try the apk I've been working on. It's a simple gallery with a proper UI just with the features I need and like with a small size to be usable on emerging markets with garbage devices with low storage and lame internet caps. Just pasting this here for you to see your 3.9.0-ALPHA changes in action in a real app https://www.dropbox.com/s/jmzvgwc60dus4dz/FocusGo-1.0-Beta1.apk

[davemorrissey]

That seems to work very well. Does it have the pooling decoder enabled?

BTW I've added a less ugly method of globally setting the bitmap config: SubsamplingScaleImageView.setPreferredBitmapConfig(Bitmap.Config.ARGB_8888);

@AlexVPerl have you had a chance to try this alpha to see if you're able to fix the contention problem you raised by using a custom executor?

[franciscofranco]

Thanks for that method helper! Yeah, it's using the Pooled decoder and using the default 320 dpi with 565 unless the user chooses to use 8888. Can you push these changes to maven? Not using the source project on my project by default.

[davemorrissey]

I'd like a bit more feedback before pushing these changes into the wild, so I've made eager loading and THREAD_POOL_EXECUTOR the new defaults and pushed them as 3.9.0.BETA.

[franciscofranco]

Yup, that's what I was asking. I currently have ~7k beta testers with a wide range of devices and not a single report of bad performance or strange events with picture viewing, just to let you know.

[franciscofranco]

Just to let you know, I already have 2 apps on production with this patchset, no issues reported so far.

[davemorrissey]

Changes released in v3.9.0