806029 - Pmem buffers are not being dropped when an application is sent to the background

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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]

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]

Attachment: 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]

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]

Attachment: ClearCachedResources() when we go hidden

Works but needs some more testing.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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]

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]

Attachment: 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]

Attachment: 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)]

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]

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) (email my personal email if necessary)]

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]

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]

(That's what I was grousing about in comment 6.)

Comment 14

[Matt Woodrow (:mattwoodrow)]

(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) (email my personal email if necessary)]

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]

(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) (email my personal email if necessary)]

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]

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]

Attachment: Have remote content drop their buffers when they're hidden, v2

Made requested comment fixes.

Comment 20

[Matt Woodrow (:mattwoodrow)]

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]

Sure.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Followup: ensure that compositing-only LayerManagerOGL's don't try to composite invalid trees

Comment 23

[Matt Woodrow (:mattwoodrow)]

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]

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)]

Interesting, that isn't mentioned in the header.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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]

Blocks a blocker.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea85f4e3fc2d

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea85f4e3fc2d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d97db15dcc1