glide icon indicating copy to clipboard operation
glide copied to clipboard

Glide's `trimMemory()` implementation doesn't trim when foregrounded

Open ygnessin opened this issue 7 years ago • 6 comments

I am profiling memory on my app usage and was looking at the built-in tremMemory(int level) methods in LruResourceCache and LruBitmapPool to see what their behavior is when I invoke them from my application's onTrimMemory method. The trim memory levels are documented here.

Here is Glide's implementation of this, for both of the aforementioned classes (and probably others):

    public void trimMemory(int level) {
        if (level >= android.content.ComponentCallbacks2.TRIM_MEMORY_MODERATE) {
            // Nearing middle of list of cached background apps
            // Evict our entire bitmap cache
            clearMemory();
        } else if (level >= android.content.ComponentCallbacks2.TRIM_MEMORY_BACKGROUND) {
            // Entering list of cached background apps
            // Evict oldest half of our bitmap cache
            trimToSize(getMaxSize() / 2);
        }
    }

The problem with this is that Glide is treating the level as a sequential number. It assumes that higher is worse. But that is not actually the case, if you look at the constant value of the levels in the linked documentation.

In particular, notice that unless level is at least 40, Glide won't trim any memory. And all levels 40 and above are only triggered if the app is backgrounded. But if the device is running critically low on memory while the app is foregrounded, Glide will never trim memory. Take, for instance, the most severe level, TRIM_MEMORY_RUNNING_CRITICAL which gets called when the app is foregrounded and the device has no remaining memory. This level's constant value is 15, which won't trigger a memory trim. And unless the app is backgrounded, none of the levels before this will trigger a trim either, because all of them are under 40.

I'd like to propose modifying this default trimMemory() behavior to take foreground events into account. If the community agrees this is a good idea, I'd also be happy to open a pull request myself when I have the time, but wanted to propose the idea first. Thanks for taking the time to read this.

Glide Version: 3.8.0 (but checked the source and this is still in issue on 4.5.0)

ygnessin avatar Jan 13 '18 02:01 ygnessin

At the moment this is intended. If there's a good reason why we should expect those error messages in the foreground and why clearing the memory cache or bitmap pool will help I'd love to hear it.

At the moment I'd expect the the low memory callbacks to occur either due to other activity on the device outside of the applications control or excessive allocations inside the application. Neither of those are fixed by temporarily clearing the caches. In addition, normal usage of most apps will pretty quickly fill up the memory cache and bitmap pool. Clearing those caches isn't much of a solution to anything.

In the background trimming the cache and pool makes more sense because it reduces the size of the backstack and the caches won't be re-filled unless the app is opened again (for the most part).

sjudd avatar Jan 16 '18 15:01 sjudd

@sjudd you make some good points. My original thought was that clearing the cache when memory is critical may prevent the app from being killed or encountering an OOM. I can understand the argument that clearing the cache won't make much of a difference, as it'll simply fill up quickly again. But what if the trim memory message is received while the user is on an activity with few or no images, and the bitmap pool and memory cache contain images from paused or stopped activities? That's the sort of scenario I was envisioning when I proposed this.

Thanks again for taking the time to consider this idea

ygnessin avatar Jan 16 '18 23:01 ygnessin

I don't have super strong feelings one way or another. You can implement this in an application by calling Glide.clearMemory, but you can't clear only a percentage of the caches.

If you're interested in sending a pull request, I'd be ok with handling the foreground cases.

sjudd avatar Jan 19 '18 15:01 sjudd

@sjudd I think this issue should be closed. Since #2889 was already merged.

dalinaum avatar Nov 27 '19 12:11 dalinaum

I also mistakenly thought that clearing the cache could be done but it is true that it is not possible to clear it even one percent, for some sites when having some issues related to crashes or picos school crashes. unresponsive caches can be cleared, but for some sites with large storage it may not be possible.

picopa88 avatar Jun 21 '21 04:06 picopa88

can u assign me this

ANUKULJHA1506 avatar May 16 '22 08:05 ANUKULJHA1506

I, too, was under the impression that it was possible to erase the cache, but it has now been brought to my attention that this is not even feasible for one percent, do you have any knowledge about rolling ball 3d I need assistance with this as well.

Junlasy avatar Nov 08 '22 05:11 Junlasy