Last Comment Bug 806029 - Pmem buffers are not being dropped when an application is sent to the background
: Pmem buffers are not being dropped when an application is sent to the background
Status: RESOLVED FIXED
[MemShrink][soft-blocker]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla19
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 808227 808231 808266
Blocks: slim-fast 806032
  Show dependency treegraph
 
Reported: 2012-10-26 18:41 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-11-08 23:50 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
pmem monitor script (1.87 KB, text/plain)
2012-10-26 18:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details
Drop layer trees when we go hidden (2.12 KB, patch)
2012-10-30 15:55 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
cjones.bugs: feedback-
cjones.bugs: feedback-
Details | Diff | Splinter Review
ClearCachedResources() when we go hidden (16.80 KB, patch)
2012-10-31 14:41 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
In-gecko gralloc monitor (3.61 KB, patch)
2012-11-02 16:05 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Have remote content drop their buffers when they're hidden (17.93 KB, patch)
2012-11-02 16:11 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Have remote content drop their buffers when they're hidden, v2 (18.44 KB, patch)
2012-11-06 15:54 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
matt.woodrow: review+
roc: superreview+
Details | Diff | Splinter Review
Followup: ensure that compositing-only LayerManagerOGL's don't try to composite invalid trees (3.51 KB, patch)
2012-11-06 19:43 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
matt.woodrow: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-26 18:41:20 PDT
Created attachment 675775 [details]
pmem monitor script

STR
 (1) adb reboot
 (2) Download pmemmon script and adb logcat | grep pmemmon
 (3) Note "current" pmem value
 (4) Launch a few applications in a row like Clock, Music, ...

The "current" pmem allocations increase monotonically (until background processes start being gunned down).  The allocations should shrink every time a process goes into the background.

Inactivating a window is supposed to make it think it's not visible, and repaint itself to an empty layer tree, freeing all the pmem buffers.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-30 00:30:42 PDT
PuppetWidget::Show(false) is definitely not being called when apps are sent to background.  I don't think that's sufficient to nuke the layer tree, but that's not expected.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-30 15:55:30 PDT
Created attachment 676823 [details] [diff] [review]
Drop layer trees when we go hidden

So the general problem here is, in "desktop" when "tabs" go hidden, they simply don't get included in the display list so their layers are dropped.  With remote content, there's no parent document to not be included in; each "tab" is toplevel and has its own widget.  I thought we had solved this problem for xul-fennec, but apparently not :(.  (Or maybe that fix regressed.)

Anyway, in the mozbrowser code, we have a setVisible() API call that ends up setting docshell.isActive on the remote tab.  This patch hooks into that notification for presshells that have a TabChild*.

I think the code to take things from hidden->visible is correct, but obviously the visible->hidden code is horrific and broken hack.  How should I do that?  Is this patch the right approach?
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-30 16:48:22 PDT
Comment on attachment 676823 [details] [diff] [review]
Drop layer trees when we go hidden

The creature I wanted here is BasicLayerManager::ClearCachedResources().

Tentative plan
 - presshell gets "inactive" notification
 - presshell pokes tabchild, if it can find one
 - tabchild pokes its puppetwidget
 - puppetwidget ClearCachedResources()s and makes sure NeedsPaint() is false until "activate" notification
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-10-31 14:41:12 PDT
Created attachment 677170 [details] [diff] [review]
ClearCachedResources() when we go hidden

Works but needs some more testing.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 13:54:42 PDT
Comment on attachment 677170 [details] [diff] [review]
ClearCachedResources() when we go hidden

Not working.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 14:39:49 PDT
On further investigation, it's working, it's just missing the dadburned subtrees it's intended to clear.  Frak.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 16:05:19 PDT
Created attachment 677919 [details] [diff] [review]
In-gecko gralloc monitor

I wasn't sure how much I trusted the numbers coming out of the monitor script, so I added some light instrumentation to cross-check.  They mostly agree.  The in-gecko instrumentation actually computed a higher max value by a few buffers or so.  I can't really explain that.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 16:11:54 PDT
Created attachment 677922 [details] [diff] [review]
Have remote content drop their buffers when they're hidden

This uncovered a couple of bad assumptions in the gaia window manager, but those are just polish bugs, don't block 5MB RAM win.

So, with this improved impl we're doing even better than I thought.  In heavy "normal" usage, I wasn't able to get pmem usage above 4.22MB.  Loading our old friend the gallery single-photo view and panning through quickly (with some very large images) spiked us up to 8.9MB according to pmemmon, and 9.5MB according to the in-gecko instrumentation.  I think I can live with that, and we may be able to make some fixes in the gallery app.

Camera isn't feeling well so I wasn't able to test that.  However, I realized that this approach isn't complete yet: we won't drop buffers that are allocated by ImageBridge.  That should be fixable, though a bit trickier, so I'll shoot for that in a followup.
Comment 9 Justin Lebar (not reading bugmail) 2012-11-02 16:48:15 PDT
Comment on attachment 677919 [details] [diff] [review]
In-gecko gralloc monitor

Let's add this to about:memory?  You don't need to write the patch, Chris, if you can sign off on the approach here.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 17:03:44 PDT
Fine by me, with the caveats that this can undercount by not rounding to page and GPU-required alignment the same way the gralloc library would.  It can also overcount because it doesn't see the real allocations done by the driver.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-11-02 22:02:11 PDT
Comment on attachment 677922 [details] [diff] [review]
Have remote content drop their buffers when they're hidden

Review of attachment 677922 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +441,5 @@
> +   *
> +   * Destruction begins from |aSubtree| or |mRoot| if |aSubtree| is
> +   * null.  |aSubtree|'s manager must be this.
> +   */
> +  virtual void ClearCachedResources(Layer* aSubtree = nullptr) {}

I don't understand why you would call this with anything but null --- since you have to complete a layer transaction anyway; what's the benefit of passing something non-null?
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 22:21:57 PDT
The client is

>diff --git a/gfx/layers/ipc/ShadowLayersParent.cpp 
>+bool
>+ShadowLayersParent::RecvClearCachedResources()
>+{
>+  if (mRoot) {
>+    mLayerManager->ClearCachedResources(mRoot);
>+  }
>+  return true;
>+}

|mLayerManager| here is the toplevel widget's layer manager.  It can manage layers for any number of thread contexts.  In b2g for example, if we just called mLayerManager->ClearCachedResources(null) here, we'd blow away all visible subprocess layer trees, in addition to all the "system UI".  And if the subtree for that particular ShadowLayersParent isn't actually reachable from the toplevel widget's layer manager, we can fail to clear it all.

The first version of my patch did that, and it's not what we want ;).
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-02 22:23:00 PDT
(That's what I was grousing about in comment 6.)
Comment 14 Matt Woodrow (:mattwoodrow) 2012-11-04 14:45:33 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #12)
> The client is
> 
> >diff --git a/gfx/layers/ipc/ShadowLayersParent.cpp 
> >+bool
> >+ShadowLayersParent::RecvClearCachedResources()
> >+{
> >+  if (mRoot) {
> >+    mLayerManager->ClearCachedResources(mRoot);
> >+  }
> >+  return true;
> >+}
> 
> |mLayerManager| here is the toplevel widget's layer manager.  It can manage
> layers for any number of thread contexts.  In b2g for example, if we just
> called mLayerManager->ClearCachedResources(null) here, we'd blow away all
> visible subprocess layer trees, in addition to all the "system UI".  And if
> the subtree for that particular ShadowLayersParent isn't actually reachable
> from the toplevel widget's layer manager, we can fail to clear it all.
> 
> The first version of my patch did that, and it's not what we want ;).

Maybe a comment here noting that mRoot isn't necessarily the root layer of mLayerManager, and is instead the root of the process' layer subtree would make this more obvious.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-11-04 19:59:23 PST
Comment on attachment 677922 [details] [diff] [review]
Have remote content drop their buffers when they're hidden

Review of attachment 677922 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/Layers.h
@@ +436,5 @@
> +   * This attempts to free up any free-able resources like retained
> +   * buffers, but may do nothing at all.  After this call, the layer
> +   * tree is left in an undefined state.  A painting or forwarding
> +   * transaction must complete on the tree before it returns to a
> +   * valid state.

This comment makes it sound like aSubtree is just a hint about which resources to destroy --- the value of aSubtree does not affect the requirements placed on the caller. Is that correct?
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-05 14:02:35 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Comment on attachment 677922 [details] [diff] [review]
> Have remote content drop their buffers when they're hidden
> 
> Review of attachment 677922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.h
> @@ +436,5 @@
> > +   * This attempts to free up any free-able resources like retained
> > +   * buffers, but may do nothing at all.  After this call, the layer
> > +   * tree is left in an undefined state.  A painting or forwarding
> > +   * transaction must complete on the tree before it returns to a
> > +   * valid state.
> 
> This comment makes it sound like aSubtree is just a hint about which
> resources to destroy --- the value of aSubtree does not affect the
> requirements placed on the caller. Is that correct?

It's not just a hint; specifies the subtree root from which to clear cached resources.  I'm not sure I understand the question about requirements placed on the caller.  The requirements are always
  !aSubtree || aSubtree->Manager == this

The value of aSubtree changes the *effects* of the call, if that's what you mean.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-11-06 01:04:34 PST
I mean:
> A painting or forwarding transaction must complete on the tree before it returns
> to a valid state.

Can you be more precise about what exactly is required here? Does this requirement depend on what value(s) of aSubtree were passed in? Does the caller have to be able to repaint or otherwise fully update *any* layer in the tree, or just layers which have aSubtree as an ancestor, or what?
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 08:21:58 PST
OK, I think I see.  The usage of "tree" "subtree" "layer tree" is not precise.  Sorry, remnant of an older patch.  Will fix asap.
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 15:54:19 PST
Created attachment 678959 [details] [diff] [review]
Have remote content drop their buffers when they're hidden, v2

Made requested comment fixes.
Comment 20 Matt Woodrow (:mattwoodrow) 2012-11-06 16:31:14 PST
Comment on attachment 678959 [details] [diff] [review]
Have remote content drop their buffers when they're hidden, v2

Review of attachment 678959 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Can we add a debug only 'layer tree is in an invalid state' flag that gets set during ClearCachedResources, and cleared during a transaction? Then we can assert that we never attempt to composite an 'invalid' shadow layer tree.
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 17:33:53 PST
Sure.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 19:43:42 PST
Created attachment 679030 [details] [diff] [review]
Followup: ensure that compositing-only LayerManagerOGL's don't try to composite invalid trees
Comment 23 Matt Woodrow (:mattwoodrow) 2012-11-06 19:52:35 PST
Comment on attachment 679030 [details] [diff] [review]
Followup: ensure that compositing-only LayerManagerOGL's don't try to composite invalid trees

Review of attachment 679030 [details] [diff] [review]:
-----------------------------------------------------------------

I think we could use DebugOnly<bool> here and avoid some of the #ifdef's
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 19:55:05 PST
DebugOnly<T> is only spec'd for stack storage; it has undefined behavior as a member variable.  It may change object alignment or size.  Not a big deal here, but not a good habit to be in.
Comment 25 Matt Woodrow (:mattwoodrow) 2012-11-06 19:59:00 PST
Interesting, that isn't mentioned in the header.
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 20:10:50 PST
Sorry, this class was supposed to be MOZ/NS_STACK_CLASS, but mfbt didn't have the capability at the time.
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-07 19:25:18 PST
Blocks a blocker.
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-07 19:53:46 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea85f4e3fc2d
Comment 29 Ed Morley [:emorley] 2012-11-08 02:36:49 PST
https://hg.mozilla.org/mozilla-central/rev/ea85f4e3fc2d
Comment 30 Ryan VanderMeulen [:RyanVM] 2012-11-08 12:41:57 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d97db15dcc1

Note You need to log in before you can comment on or make changes to this bug.