Race condition with memory cache? #2560
Comments
|
I'm not totally sure this is the issue you're describing, but, if you start two requests simultaneously for an image that's in the disk cache, but not in the memory cache, I'd expect both of those requests to complete with RESOURCE_DISK_CACHE as their source. Glide will start the first request asynchronously, then attach the second request as a listener to the first. When the load completes, both the listener for the first request and the second request are notified with the source where the image was loaded from. |
|
@sjudd The images are in memory or should be. The result is consistent for every calls. I found that when changing to support Android Auto and lacked a check, this resulted in updating both notification and MediaSession at the same time every second with those 2 nearly simultaneous calls. This leaded to load from disk cache every seconds for the 2 calls. I then tried some tests to have more data (The test with same size, then test with delay). I can understand the first calls are from disk cache, but all future should not be. That's why I suspect a race condition. |
|
Are you sure the items were in the memory cache? It's a little hard to measure, but custom logging or patience with the debugger might eventually work. The memory cache is managed entirely on the main thread, so there shouldn't be race conditions. It's possible there's some place where that condition doesn't hold, but that would be easy to verify by creating a subclass of the LruCache an adding a main thread assertion to every call. |
|
Well race condition are complicated to debug without understanding all the library internals :( Logs does not tell if the memory cache is tested at all and why it fails. And adding breakpoints would create the delay and remove the issue. And the issue could be reading or writing to it after. Maybe I'm not clear but the problem is that At T=0 I do the 2 calls, both are returned from disk (could be OK) With same size in request at T=1 or 2 or 3, 90% both are from memory, 10% 1 or 2 are from disk. When adding a delay between the calls at T=1, 2, 3 ... 100% are from memory. Since with a delay all are in memory I can assume that putting to memory cache works correctly for the model and query types so that it should work in the other cases too. From the engine logs the query are the exact same and all parameters in the logs confirms that including hashcode. |
|
I made an attempt to write a test case for what you described, let me know what else is missing from there: sjudd@6849da3. I don't think there are any logs, but you could try to debug evictions from the memory cache: |
|
I do not use override but into(x,y) and I use a custom model that implement hashcode/equals and the Model interface don't know if any of those have impacts. I'll see if I can get more logs today. |
|
@sjudd I tried to log the cache eviction but it seems it does not work. In custom GlideModule: And the listener is never called (But I do see ressources properly switching from loaded from memory to disk when scrolling in heavy image recyclerview so there is correct eviction.) Trying to debug it seems the engine will override the listener :( So I went to a child class and overriding and then I can see that there's no evict. |
|
Try logging If we know the cache its self isn't involved, the next thing to look at is active resources. Active resources uses weak values, so it's possible that the values are being gced. That could happen if the Target returned by Looking at the code again, I'd bet that's it. You can test this by just retaining a reference to the Future returned by |
|
@sjudd ok so after logging remove I see the calls and they always return null for the bitmaps (All works as you explained for the Drawable targets) So it seems asBitmap().into(x,y).get(10,SECONDS) never put back the images in memory cache and it only works when get from active ressources? But that does not explain why it only occurs with simultaneous calls :( Not sure how I can keep a ref to the future as those calls are made from static helper functions. |
|
For testing you could probably just use a random static collection. I wasn't able to reproduce the scenario I described in an emulator, but I think that's more due to WeakReference/gc issues. I was able to reproduce the scenario in a unit test: cdb21f6. If I'm right, it's basically a matter of getting the system to collect the weak references. That may be more frequent under more memory pressure, or it may be more or less random. |
|
Adding ref to FutureTarget makes the query always return from MEMORY with "Loaded resource from active resources" But then I'm not sure to fully understand the issue and the possible solution. Not keeping ref, does not keep image in active but do not put it in memory cache either. (This is the part I do not understand if there's no ref then the image should return to memory cache no?) Solution seems to keep a ref to last futuretarget but does calling GlideApp.clear on that futuretarget when I suppose no more used works and properly release everything ? |
|
Yes the long term solution in Glide is to make sure that when active resources is cleared because a reference is removed, the image is still added to the memory cache. There is a caveat that without an explicit clear it won't be safe for us to re-use the Bitmap associated with the resource, even if we can add it to the memory cache. Yes you should be able to solve this for now by retaining a reference to the FutureTarget. Yes you can clear the FutureTarget whenever you're done with the Bitmap/Drawable to allow Glide to re-use the image as expected. In fact it's the recommended way of dealing with Glide's Futures on background threads, especially for cases where you're loading lots of images in a row. Does that help? |
|
Well still unsure how to deal with that correctly :( Currently for MediaSession meta data I do get the bitmap each time to avoid caching it myself. So every time there's a change I call Glide to get the bitmap as I assumed it would always come from memory cache if the same image is request multiple times in a row. (All that in a static method that returns a MediaMetadataCompat but it's not important) But since it's a future I'd need to keep previous + new and only clear previous when new is actually sent to MediaSession. But doing so the clear happens too late and new will still load from disk. If I clear too early I guess it could lead to problem in MediaSession as glide could reuse / recycle. And if I reuse the same future target, there's still chance GC remove the image before new call actually ends. So not really sure what's the correct approach except caching the bitmap myself. |
|
Can you clarify what you mean by:
If the clear happens late that's ok from the caching perspective. Clear isn't required for the memory cache to be used, it's only required to allow Glide to re-use the resources (Bitmap in this case) for a subsequent load. You don't actually have to clear at all, it's just a performance optimization. The only work around you need to get memory caching working is to retain a reference to the FutureTarget until you're done with the corresponding Bitmap. |
|
I was thinking about future fix and using memory cache, was not thinking about your workaround that is to use the active ressources. I'll try with the workaround for now thanks. |
Previously resources that were dereferenced but not released vanished into the ether. As a result you could load a Bitmap and then immediately after load the same Bitmap again and get a memory cache miss on the second load. This pattern is particularly common with the submit() APIs that load images on background threads where the Target is neither referenced by a view nor referenced directly. Fixes bumptech#2560
Previously resources that were dereferenced but not released vanished into the ether. As a result you could load a Bitmap and then immediately after load the same Bitmap again and get a memory cache miss on the second load. This pattern is particularly common with the submit() APIs that load images on background threads where the Target is neither referenced by a view nor referenced directly. Fixes bumptech#2560
Previously resources that were dereferenced but not released vanished into the ether. As a result you could load a Bitmap and then immediately after load the same Bitmap again and get a memory cache miss on the second load. This pattern is particularly common with the submit() APIs that load images on background threads where the Target is neither referenced by a view nor referenced directly. Fixes bumptech#2560
|
So I have a fix for this, the tradeoff is that Glide will retain some extra memory. We'll keep a soft reference to the resource we pass out to consumers, but a hard reference to the actual data. If the resource we pass out is GCed, we release the data back into the memory cache. We won't be able to re-use any resources returned to Glide like this because we can't know if the caller is still using the actual Bitmap. The hard reference Glide will retain may transiently increase memory usage because there's not a defined time between when the resource we pass out is GCed and when our weak reference is cleared. It's always going to be good practice to retain a reference to the |
|
Thanks can you still confirm when to use the clear call? When I know that I will not need the image called again or when I know that the bitmap is no more used anywhere (This one is kinda impossible) |
|
In particular, call Glide uses reference counting, so each load increments the reference count and each If you have many places across your application using the resource with different load calls, each one can call |
|
Also I should note that Glide is permissive. We won't re-use or recycle the Bitmap if you never call |
Glide Version:
4.2, 4.3
Integration libraries:
Device/Android Version:
Issue details / Repro steps / Use case background:
When starting at the same time the same request to the same model (With correct hashCode/Model implementation) with different submit(x,y) values, you most of the time get
Finished loading Bitmap from RESOURCE_DISK_CACHE
and not
Finished loading Bitmap from MEMORY_CACHE
Meaning image is loaded from disk and not memory cache.
When targeting the same size 90% of the time requests are from memory.
When adding a delay between the calls, 100% of the time the image is correctly returned from memory cache.
It is important to note that it's not a problem with purging of the memory cache, as all other images are still correctly returned from memory cache.
Glide load line /
GlideModule(if any) / list Adapter code (if any):Stack trace / LogCat:
One of the request is sometimes from disk when using same size
When using different target size all requests are from disk.
The text was updated successfully, but these errors were encountered: