413947 - Memory cache entries should be evicted on memory-pressure notification

Status: NEW

(Core :: Networking: HTTP, defect, P3)

Description

[Stuart Parmenter]

Right now there is no way to purge https images, js, etc from the memory cache.  Would be good to knock these out when memory-pressure notification fires.

Comment 1

[Reed Loden [:reed]]

Attachment: patch - v1

Comment 2

[Reed Loden [:reed]]

So, after talking to stuart, it looks like my patch clears the entire necko memory cache. stuart wants to only clear HTTPS. There doesn't seem to be a way to clear _only_ HTTPS, but I can clear HTTP(S) easily. Would that work?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Why would you only clear HTTPS instead of all the memory cache? (Wouldn't it be a really good idea especially to clear the image cache, which this patch also does?)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Note that not all HTTPS content is stored in the memory cache (some is on disk).

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we land this?  Should be a nice easy win.

Comment 6

[Jason Duell]

So the patch clears the entire necko memory cache (not just HTTPS).  That's fine (?), as I don't see the initial reasoning for why we'd only toss HTTPS entries when we're under RAM pressure.  But as comment 3 seems to be trying to imply, we probably would want to flush the image cache as well, which AFAICT this patch doesn't do.

But I'm not clear on whether we want to flush these caches every time we are suspended, and/or whether there's a better way to do it.

cjones tells me memory-pressure events are very rare on desktop (from quick source browse I see things like when malloc fails, etc), but it happens on android each time we're put in the background.   So this is basically the mobile/background case.

There seems to be talk about how if ashmem is used, the kernel can free segments when there's memory pressure.  That would be preferable to simply tossing the entire, hot HTTP RAM cache out the window every time we're backgrounded.  But Taras didn't seem to see the promised effect (see bug 622723 comment 2) and I can't find decent info on the web about it. 

blassey and mbrubeck tell me we continue running in the background, including loading pages.  It's not clear to me that dumping a hot HTTP cache is a win in that case.  

Also note that at present the RAM cache is typically quite underpopulated--we only use it for HTTPS no-store entries.  That should probably change, but for now we're gobbling up several MB w/o using much of it, which is an argument in favor of this patch, at least for now.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It seems pretty clear that we should be OK with clearing the memory cache, because the alternative is that we get killed or crash due to out-of-memory. We just need to make sure there isn't anything that relies on something being in the memory cache.

In order for this to have the positive effect we are looking for, we have to be allocating memory in such a way that, when we free it, the memory allocator will return the memory to the operating system. I think dougt told me that this only happens in jemalloc if/when we allocate memory in chunks of 2MB or 4MB or something like that. And, IIRC, or memory cache on mobile is only 1MB. So, there is a disconnect there.

> we only use it for HTTPS no-store entries.

If this is true, then we can do no-store caching in a different way in the future, to avoid needing the memory cache just for it. I have filed bug 701597 for this.

If/when we store other (non-no-store) items in the memory cache, then we should try to flush them into the disk cache. Please file a follow-up bug to do that.

> That should probably change,
> but for now we're gobbling up several MB w/o using much of it, which is
> an argument in favor of this patch, at least for now.

If we are using several MB just for no-store pages, then we should fix that in general, especially on mobile. It seems very wrong to allocate even 1MB of memory exclusively for use in caching no-store pages.

Comment 8

[Jason Duell]

> If/when we store other (non-no-store) items in the memory cache, then we should try to flush them into the disk cache. Please file a follow-up bug to do that.

Bug 602611 is the current bin for ideas on how to make the memory and disk cache relate in some sort of way (whether backing store or flush to disk, etc).

> for now we're gobbling up several MB w/o using much of it

I spoke a bit too soon.  The memory cache appears to use only as much memory as is actively needed--my desktop about:cache shows that only 1.8MB of up to 30 MB max is being used, and about:memory shows network-memory-cache as using 1.8 MB.  Given how often fennec probably gets restarted on most android devices, I suspect the RAM cache is already pretty small for most use-cases, and not "wasted" (except in the sense that since it takes a while to fill up 1 MB of no-store entries in most browsing histories, some of the entries might be fairly stale if the browser stays alive for a long time).

Comment 9

[Jason Duell]

> It seems pretty clear that we should be OK with clearing the memory cache, 
> because the alternative is that we get killed or crash due to out-of-memory.

*When* the alternative is "clear RAM cache or be killed", then yes.  But it's not clear to me what % of the time fennec's death will be avoided by freeing up the memory cache, and whether preemptively doing that every time we're backgrounded is worth having an empty memory cache when we get foregrounded after not being killed.  It will probably depend a lot on the memory capacity of the device and the user's application switching patterns.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

To add to what Jason said, we could be smarter and avoid discarding the entire memory cache. Instead, we could create a new, smaller memory cache and only add in the most recent X items that have been accessed within the last Y minutes. And/or, like Jason suggested in another bug, we could simply use a smaller memory cache.

Still, I think we should operate on the assumption that the memory pressure event means we need to free up as much RAM as we can *right* now.

We probably should wait a little bit after receiving the "pause" event from the mobile OS before sending out the memory pressure event, in order to avoid releasing a bunch of memory for very temporary things, but if we're sending the memory pressure event too soon then that's a different bug.

Comment 11

[Josh Matthews [:jdm]]

The reason behind sending memory-pressure notifications when Firefox is sent to the background has to do with the OS app-killing behaviour. Specifically, when memory becomes tight, the OS frequently will kill apps in the background using lots of memory before resorting to killing the foreground process. Therefore, the memory-pressure notifications occur when the app loses foreground status to boost the chances of staying alive until it returns to the foreground.

Comment 12

[Jason Duell]

> we could create a new, smaller memory cache and only add in the most recent 
> X items that have been accessed within the last Y minutes.

And/or just implement eviction of entries by timestamp (older than X, younger than Y), which would be useful for proper "Clear recent history" support anyway (right now we blow away the entire RAM + disk caches).

> We probably should wait a little bit after receiving the "pause" event 
> from the mobile OS before sending out the memory pressure event

I suppose this depends on how soon we tend to get killed if we don't shrink in size.

> we need to free up as much RAM as we can *right* now.

Sure--I don't want this discussion to lead to inertia.  I suggest we extend this patch to 1) also flush the image cache (which I know little about, but I suspect it's many, many times bigger than the memory cache in most cases), and 2) add some telemetry to detect how often we get backgrounded and *don't* die.  If there's a way to detect the amount of free RAM on a device, that might provide a useful heuristic about whether to flush caches on background, too.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from comment #12)
> > we need to free up as much RAM as we can *right* now.
> 
> Sure--I don't want this discussion to lead to inertia.

Sorry, I was unclear: I didn't mean we need the simplest fix we can do as fast as possible. I meant that when the cache receives the memory pressure notification, it should not delay in freeing up the memory and it should free up as much memory as it can. (This means even releasing the disk cache metadata, to be re-read from disk when we are brought to the forward again.)

> I suggest we extend this patch to 1) also flush the image cache (which I
> know little about, but I suspect it's many, many times bigger than the
> memory cache in most cases),

But, again, when we release this memory, will it actually get released back to the operating system? I think it only happens if we free a chunk of memory that was malloc'd using a size of 1MB or 2MB or more; allocations smaller than that are managed internally by the memory allocator and so freeing them wouldn't actually shrink our footprint.

> and 2) add some telemetry to detect how often we get backgrounded and
> *don't* die.  If there's a way to detect the amount of free RAM on a device,
> that might provide a useful heuristic about whether to flush caches on
> background, too.

+1.

Comment 14

[Jason Duell]

> when we release this memory, will it actually get released back to the 
> operating system? 

Sounds like an argument for using ashmem (possibly with a memory allocation library on top of it, so we can still use malloc-like calls if that's what the memory cache uses).

Comment 15

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 16

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258