As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image 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 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image Robert O'Callahan (:roc) (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 User image 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 User image 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 User image 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 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-06 17:33:53 PST
Sure.
Comment 22 User image 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 User image 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 User image 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 User image Matt Woodrow (:mattwoodrow) 2012-11-06 19:59:00 PST
Interesting, that isn't mentioned in the header.
Comment 26 User image 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 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-11-07 19:25:18 PST
Blocks a blocker.
Comment 28 User image 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 User image Ed Morley [:emorley] 2012-11-08 02:36:49 PST
https://hg.mozilla.org/mozilla-central/rev/ea85f4e3fc2d
Comment 30 User image 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.