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

Implement a Bitmap pool for Picasso #672

Closed
lucasr opened this issue Sep 25, 2014 · 19 comments
Closed

Implement a Bitmap pool for Picasso #672

lucasr opened this issue Sep 25, 2014 · 19 comments

Comments

@lucasr
Copy link
Contributor

lucasr commented Sep 25, 2014

I haven't looked very deep into this yet but it seems Glide does a good job reusing Bitmaps and reducing GC pauses on image-heavy UIs. The new APIs in #665 will definitely help with reducing GC but using a Bitmap pool seems like a good idea anyway.

The Bitmap pool implementation could actually be a standalone library. I wonder if @sjudd (Glide's creator) would be interested in collaborating? Or maybe Picasso should have its own Bitmap pool implementation. Dunno.

Just filing this ticket to get some discussion going. Thoughts?

@JakeWharton
Copy link
Member

We have talked extensively on this and I have very strong opinions on it. I haven't looked Glide's implementation.

Basically, I want it, but I have very, very strict requirements for its behavior (especially how it relates to the memory cache, Target, and request joining).

@lucasr
Copy link
Contributor Author

lucasr commented Sep 25, 2014

Yeah, using a Bitmap pool would definitely have implications on the internal and public APIs.

My first (completely random) thought was that RequestHandler.load(Request) would become RequestHandler.load(Request, Bitmap) where the Bitmap argument would be a sort of "convertBitmap" argument. Similar to how Adapter.getView(int,View,ViewGroup) works.

Or maybe the recycling magic would be encapsulated in a BitmapFactory wrapper that does the right BitmapFactory.Options incantation under the hood.

Just thinking out loud here.

@JakeWharton
Copy link
Member

Hope this isn't long... here's my thoughts:

Without leaky APIs (more on this in a second) this can be done 100% behind the scenes. A Bitmap is loaded in a reference counting container. It's handed to a PicassoDrawable and the reference is incremented. It's cached in memory and the reference is incremented. If it's every handed to other image views the reference is also incremented.

Detecting a PicassoDrawable being garbage collected in a reference queue or being recycled in an adapter view would decrement the reference count. Being evicted from the memory cache would decrement the reference count.

Through this simple mechanism you would know when to re-use the Bitmap object.

However, even in this perfect system, there are problems. If a bitmap becomes available for re-use how long do we hold on to it? What if no requests for images are made for 2 seconds. Or 10. Or 60.

Additionally, bitmap memory can only be arbitrarily re-used on KitKat. On pre-KitKat we have to match the destination resolution and density exactly in order to be re-used. This is ideal for something like a ListView. Or is it? Aggressively re-using bitmap memory of images which are of the same size is nice on the GC but bad on the CPU because it means we need to re-decode the image if you scroll back up. We can balance these two, but where is the threshold?

Moreover, I haven't even talked about leaky APIs!

Because Bitmap is final we can't reference count it when exposed in the API. So this means that any users of Target automatically destroy our chance at re-using a Bitmap since we hand it to the caller. We can't know when they're done with it, and we can't force them to always tell us (because they won't).

There's a few more nuances, but that should give you the gist. Of course this becomes easier if we push the burden on the caller. But Picasso has never been about that. In fact, I don't even know how many people are capable of correctly doing that.

I don't want to discourage us from trying. We should try. We should lie, cheat, and steal from Glide (assuming it's Apache 2). Or any other library.

There's another option which has been the one which I'm more interesting in exploring because I think it could solve the re-use problem from a different angle: inPurgeable. I talked to two companies using Picasso that are interested in exploring it here as well. Because we own the bytes and the Bitmap becomes a facade on them we might be able to do the same magic things as pure recycling without compromising the API or forcing unnecessary burden onto the caller.

@sjudd
Copy link

sjudd commented Sep 26, 2014

I'd be delighted to help work on this in Picasso, I'm all for fewer janky Android apps.

For what it's worth, I agree with Jake about leaky APIs. Because Bitmap is final you cannot guarantee that any system you use is safe. If you wrap a Bitmap your caller can always unwrap it and use it a second time without telling you, either by failing to increment the reference count or by failing to keep a reference to the wrapper.

That being said, I don't think you can get around that risk and still have a performant system. I tested using purely WeakReferences to track wrappers for Bitmaps and found that the delay between when an object is no longer referenced and when it's WeakReference is added to a ReferenceQueue is sufficiently large to degrade performance while scrolling in lists. Using inPurgable you'd still have this problem, plus you'd either have to re-use a bunch of byte arrays or use a single large byte array and write your own memory allocator...

Glide uses roughly the strategy Jake outlined initially. Glide is relatively permissive in that it's ok to fail to return resources to the pool. Glide only re-uses resources when the user re-uses Targets. Since Bitmap re-use is most important in lists and almost everyone re-uses Views in lists, this works quite well. The caveat of course is that as Jake described, a user can always decide to pull a Bitmap out of a view without telling your framework which will break in all sorts of unpleasant ways.

My opinion is that it's acceptable to say that certain types of actions by your users will result in undefined behavior as long as doing the right thing is easy and intuitive and doing the wrong thing is hard (and preferably well documented). I can totally see not agreeing with that point of view though.

Happy to expand more on how Glide works if you're interested.

@lucasr
Copy link
Contributor Author

lucasr commented Sep 26, 2014

Just as an extra reference: @chrisbanes's Android-BitmapCache (https://github.com/chrisbanes/Android-BitmapCache) has some code to manage ref counts on Bitmaps as well. It provides its own ImageView and Drawable implementations to take care of updating Bitmap ref counts under the hood.

@JakeWharton
Copy link
Member

We definitely cannot force use of a custom ImageView.

@dnkoutso dnkoutso added this to the Picasso Next milestone Oct 7, 2014
@eboudrant
Copy link

May be Ymagine (https://github.com/yahoo/ygloo-ymagine) could help for bitmap pooling, it is an alternative to the android.graphics.BitmapFactory and is very performant with complex rescaling (different size of images in the pool).

https://github.com/yahoo/ygloo-ymagine/blob/master/src/com/yahoo/mobile/client/android/ymagine/BitmapFactory.java

@JakeWharton
Copy link
Member

Yep. At the very least, it would be cool to have the option to use it.
On Feb 20, 2015 11:01 PM, "Emmanuel Boudrant" notifications@github.com
wrote:

May be Ymagine (https://github.com/yahoo/ygloo-ymagine) could help for
bitmap pooling, it is an alternative to the android.graphics.BitmapFactory
and is very performant with complex rescaling (different size of images in
the pool).

https://github.com/yahoo/ygloo-ymagine/blob/master/src/com/yahoo/mobile/client/android/ymagine/BitmapFactory.java


Reply to this email directly or view it on GitHub
#672 (comment).

@mullender
Copy link

Sharing some data about the possible improvement in performance this feature good provide.

I did a quick and dirty experiment with bitmap pooling in Picasso.
Using the sample app on an API19 emulator, scrolling all the way down, then all the way up.
the garbage collection times without bitmap pooling add to 1044ms
with bitmap pooling, the GC takes 462ms.
About a 55% percent reduction in GC pauses.

Keep in mind this data is from a quick experiment.

@mullender
Copy link

I just posted a PR (the second commit is the relevant one, the first adds gradle) that implements bitmap pooling.
The API is very explicit:

  • you must enable bitmap pooling:
        Picasso.setSingletonInstance(new Picasso.Builder(this).bitmapPool(new BitmapPoolImpl()).build());
  • your view code must release the reference explicitly:
    Picasso.with(context).recycle(view);

For the sample application it reduces the time spent in GC and the frequency of GC by half.

mullender pushed a commit to mullender/picasso that referenced this issue Oct 6, 2015
mullender pushed a commit to mullender/picasso that referenced this issue Oct 6, 2015
@mullender
Copy link

@JakeWharton @lucasr Have you had a chance to look at this? And could you provide some feedback

@Macarse
Copy link
Contributor

Macarse commented Oct 13, 2015

Any update on this?

@mullender
Copy link

Can you please give an indication of when someone will take a look?

@mullender
Copy link

ping

mullender pushed a commit to mullender/picasso that referenced this issue Oct 22, 2015
@mullender
Copy link

#1150

@sxsx0723
Copy link

any update on this?

@JakeWharton JakeWharton removed this from the Picasso Next milestone Mar 3, 2018
@Miha-x64
Copy link

We definitely cannot force use of a custom ImageView.

Yes, but you can force use of a custom Drawable!
There's PicassoDrawable already. I can imagine the following scenario:

  • exposing a Bitmap (e.g. via Target) makes it unrecycleable; new Target2 interface accepts a Drawable
  • all Picasso Drawables are managed and tracked by reuse facility
  • a Bitmap can be stolen from a PicassoDrawable if it !isVisible() both for reuse and for memory trim
  • if our Drawable becomes visible again, it requests Bitmap from Picasso and draws placeholder while Bitmap gets read and decoded.

P. S. Many people say Glide is good at image reuse. I hate it for being binary incompatible and bloated but currently it is very popular particularly for careful memory usage. While this issue is lying down here since 2014, users are getting away :( they are forced to use crappy Glide to avoid OOMs.

@JakeWharton
Copy link
Member

No plans to add this. You can't even do it with ImageDecoder, anyway.

@Miha-x64
Copy link

Thanks, this explains everything.
For further diggers: ImageDecoder#decodeBitmapImpl.

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 a pull request may close this issue.

9 participants