Cached tiles are already freed when an application goes to background. It seems better to release cached tile even when an application is in foreground on memory-pressure.

https://tbpl.mozilla.org/?tree=Try&rev=f43dff55f60d

[Blocking Requested - why for this release]: This bug blocks bug 1049251.

Comment on attachment 8483610 [details] [diff] [review] patch - Add memory-pressure handling to ClientLayerManager Review of attachment 8483610 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ClientLayerManager.cpp @@ +48,5 @@ > + > + ClientLayerManagerObserver(ClientLayerManager* aClientLayerManager) > + : mClientLayerManager(aClientLayerManager) > + { > + RegisterMemoryPressureEvent(); Probably not worth registering if mClientLayerManager is nullptr? Although, to be perfectly honest, since this is always going to get "this" and thus not be a nullptr, I'd make mClientLayerManager a ClientLayerManager& instead, and not bother checking if it's nullptr at all, but that's a question of style... @@ +61,5 @@ > + NS_IMETHODIMP Observe(nsISupports* aSubject, > + const char* aTopic, > + const char16_t* aSomeData) > + { > + if (!mClientLayerManager || strcmp(aTopic, "memory-pressure")) { "memory-pressure" string shows up three times in this class. Probably worth const char* or something like that to avoid possible typos?

Comment on attachment 8483610 [details] [diff] [review] patch - Add memory-pressure handling to ClientLayerManager Review of attachment 8483610 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ClientLayerManager.cpp @@ +40,5 @@ > > using namespace mozilla::gfx; > > +// Listen memory-pressure event for ClientLayerManager > +class ClientLayerManagerObserver MOZ_FINAL : public nsIObserver nit: not very important because the class is simple enough that it is easy to find out but I'd have used MemoryObserver in the name rather than just observer to make it more informative. Your call. @@ +61,5 @@ > + NS_IMETHODIMP Observe(nsISupports* aSubject, > + const char* aTopic, > + const char16_t* aSomeData) > + { > + if (!mClientLayerManager || strcmp(aTopic, "memory-pressure")) { To follow up on Milan's comment about "memory-pressure". It seems like a common pattern in this case is to use a #define like: #define MEMORY_PRESSURE_OBSERVER_TOPIC "memory-presure" Not everybody do it, but I think it's a good practice. @@ +115,5 @@ > , mPaintSequenceNumber(0) > , mForwarder(new ShadowLayerForwarder) > { > MOZ_COUNT_CTOR(ClientLayerManager); > + mClientLayerManagerObserver = new ClientLayerManagerObserver(this); nit: mMemoryObserver would be a more informative name.

(In reply to Milan Sreckovic [:milan] from comment #4) > Comment on attachment 8483610 [details] [diff] [review] > patch - Add memory-pressure handling to ClientLayerManager > > Review of attachment 8483610 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/client/ClientLayerManager.cpp > @@ +48,5 @@ > > + > > + ClientLayerManagerObserver(ClientLayerManager* aClientLayerManager) > > + : mClientLayerManager(aClientLayerManager) > > + { > > + RegisterMemoryPressureEvent(); > > Probably not worth registering if mClientLayerManager is nullptr? Although, > to be perfectly honest, since this is always going to get "this" and thus > not be a nullptr, I'd make mClientLayerManager a ClientLayerManager& > instead, and not bother checking if it's nullptr at all, but that's a > question of style... This class assume that mClientLayerManager is not null at the construction. ASSERT check might help this. And mClientLayerManager is set to nullptr at Destroy(), therefore I used pointer here.

(In reply to Nicolas Silva [:nical] from comment #5) > Comment on attachment 8483610 [details] [diff] [review] > patch - Add memory-pressure handling to ClientLayerManager > > Review of attachment 8483610 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/client/ClientLayerManager.cpp > @@ +40,5 @@ > > > > using namespace mozilla::gfx; > > > > +// Listen memory-pressure event for ClientLayerManager > > +class ClientLayerManagerObserver MOZ_FINAL : public nsIObserver > > nit: not very important because the class is simple enough that it is easy > to find out but I'd have used MemoryObserver in the name rather than just > observer to make it more informative. Your call. If we want to use MemoryObserver, it becomes very simple name and could easily conflict with others. Then, the class should be under the name of ClientLayerManager lile ClientLayerManager::MemoryObserver.

(In reply to Sotaro Ikeda [:sotaro] from comment #3) > [Blocking Requested - why for this release]: This bug blocks bug 1049251. I agree, we should 2.0+ this.

(In reply to Nicolas Silva [:nical] from comment #5) > > To follow up on Milan's comment about "memory-pressure". It seems like a > common pattern in this case is to use a #define like: > #define MEMORY_PRESSURE_OBSERVER_TOPIC "memory-presure" I put off this from this bug. It is not clear yet that how to do it safely everywhere with/without unified build.

Applied comments without Comment 9.

Due to recent policy changes, all patches need approval for uplift regardless of blocking status. Please request b2g32 and aurora approval on these when you get a chance. Sorry for the inconvenience :(

Comment on attachment 8484199 [details] [diff] [review] patch ver2 - Add memory-pressure handling to ClientLayerManager NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): None User impact if declined: User might face that more applications get killed. Testing completed: Locally tested. Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch:

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bc34d12c095 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d1a5bf375ddd