693930 - Fennec keeps viewport ThebesLayers alive for background tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Oleg Romashin (:romaxa)]

I've created patch for dropping ShadowableThebesLayer backBuffers, but found that fennec still consume huge amount of memory, because we seems don't drop Front Thebes Layers (remote viewport) for inactive tabs.

IIRC we where dropping viewport before, not sure what happened recently.
As simple example, is open ~15 /usr/share tabs, will cause very big memory usage in fennec

Comment 1

[Oleg Romashin (:romaxa)]

Ok, checked it, we do call :Deactivate and nsDOMWindowUtils::SetDisplayPortForElement(0,0,0,0) on remote frame...
but nsDOMWindowUtils::SetDisplayPortForElement does not do anything with underlying layers and it's buffers... 
Should not we drop layer tree if dimensions empty?

Comment 2

[Oleg Romashin (:romaxa)]

On Chrome side I only was able to catch TabParent::Deactivate....
Is it ok to resize or deactivate/clear/destroy RemoteFrameParent from there?

Comment 3

[Oleg Romashin (:romaxa)]

Do we have some API which I can simply call from TabParent and that will drop ShadowLayers tree, and restore it back as soon new paint happen? or do I need to add some API to shadowLayerManager and provide some function like (MarkTree[In]active) and that supposed to go through shadowLayers subtree and drop all allocated bufers?

Comment 4

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

Can you bisect to find what regressed this?  I wonder if it was something like bfcache, and we're missing the clearing of an active bit somewhere.

Comment 5

[Oleg Romashin (:romaxa)]

checked old versions (7.0)... and we never did any layer tree drop... only setUpdateDimensions to 0,0, which just block paint activity for remote tabs, but layer tree and all buffers still there for all background tabs.

Comment 6

[Oleg Romashin (:romaxa)]

Ok, I was able to get desktop behavior, by setting Retained flag to False...
Retained layer manager make whole child layer tree alive forever.

So, one option (not sure yet what exactly needed) is to disable retained mode in runtime when tab goes to background, or just provide some ShadowLayerManager API which would allow us just clear all internal ShadowTree buffers...

Comment 7

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

So what happens on desktop is that we (usually) have one layer manager per top-level window.  Whenever a tab is unfocused, its layers are discarded because the tab is invisible.  The content tab is part of the same document tree as the top-level XUL window.

In content processes, each TabChild has its own nsIWebBrowser, which owns its own "widget" (PuppetWidget).  When a tab is put into the background, the same mechanism on desktop won't work as-is, because there won't be a "next paint" when the new background tab is invisible.  We need to get the same effect some other way.

If you minimize a window on desktop, is the layer tree thrown out?  If so, what causes that?  That might be a place to look.

Comment 8

[Oleg Romashin (:romaxa)]

I guess on desktop we just drop retained scrollable thebes layers as soon as scrolling finished...

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Last creatures that keeps layers alive after switching to another tab

Comment 10

[Oleg Romashin (:romaxa)]

Tried to remove layerManager and do some other tricks, but that endup with weird behavior later...
wondering could not we just call LayerManager->ClearCached resources and reuse that for clearing all cached front buffers...?

Comment 11

[Oleg Romashin (:romaxa)]

ok, I tried to make last 0 size paint event but it does not help at all.
last two references only released when tab is closed

Comment 12

[Oleg Romashin (:romaxa)]

Alsi even if I disable all checks for empty region, then this check
frame->GetStateBits() & NS_FRAME_UPDATE_LAYER_TREE
prevents entering into BuildContainer blabla...
I disabled even that check and still no luck.

Comment 13

[Oleg Romashin (:romaxa)]

I got layers destroyed by:
1) removing reference to LayerManager from PuppetWidget
2) I called PropertyTable()->DeleteAll(); when dirty region was empty in Shell::Paint... that crashed later, but at the same time released reference to LayerManger and released all layers.

Comment 14

[Oleg Romashin (:romaxa)]

Also I checked how desktop FF clearing that property table, and found that it is calling simply ContentViewer::Hide which destroy presshell with all properties...

IIUC we should try to destroy LayerManager in PuppetWidget, but what should I do with presShell? destroy it too?

Comment 15

[Oleg Romashin (:romaxa)]

Also it looks like ContentViewer Hide does work related to Widget destroy, which automatically kill LayerManager too.

Comment 16

[Oleg Romashin (:romaxa)]

oh, no I was wrong DocumentViewerImpl::Hide is not called when we switch between tabs.

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Build display list with empty region and empty display port

Ok, I forced layer re-build for inactive tab, and found that even with empty port and region we still create children frame:
SolidColor 0xabe37400(Viewport(-1)) (0,0,58800,27540)(0,0,0,0) opaque uniform
which cause container be non empty, and hold thebes layer...

I've intersected canvasArea with visible region and disabled AddCanvasBackgroundColorItem in case if canvasArea empty after that intersection, and I got empty displayList after that, and thebesLayer dropped from PuppetWidget layerManager

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Don't add Background item if visibleRegion is empty

Comment 19

[Oleg Romashin (:romaxa)]

Also now I need to figure out what to do with hacks related to passing PAINT event with empty region...
right now there are 2 places in viewManager that prevents to send empty paint event.. also I'm blocked here, http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5359.

So what I should do about this? add some special flags and allow that to endup in PAintFrame? or should I just make that fake paint from PresShell::SetIsActive?

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Send empty paint transaction when display port is empty

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 567034 [details] [diff] [review]
Don't add Background item if visibleRegion is empty

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

I'd put the IsEmpty() check in PresShell::AddCanvasBackgroundColorItem. Other than that, this is good.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Oleg Romashin (:romaxa) from comment #19)
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.
> cpp#5359.

I'm not sure what line exactly you meant.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 567202 [details] [diff] [review]
Send empty paint transaction when display port is empty

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +355,5 @@
> +          }
> +        } else if (lastDisplayPort.IsEmpty()) {
> +          nsIntRect rect;
> +          widget->GetBounds(rect);
> +          widget->Invalidate(rect, true);

this invalidate should not be necessary.

Comment 24

[Oleg Romashin (:romaxa)]

without it, I see content not repainted after switching in fennec tab

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why didn't the rootFrame->InvalidateWithFlags take care of it?

Comment 26

[Oleg Romashin (:romaxa)]

Attachment: Don't add Background item if bounds are empty

Comment 27

[Oleg Romashin (:romaxa)]

Attachment: Send empty paint transaction when display port is empty

Comment 28

[Oleg Romashin (:romaxa)]

Cool, tested try build https://tbpl.mozilla.org/?tree=Try&rev=e81581d85185
And now I can open more than 50 tabs with (file:///proc) and my GTab does not get how, and trying to kick me out... (before it was happening after 15-20 same tabs opened)

Comment 29

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dd7339bedb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fce0afe2141

Comment 30

[Nicholas Nethercote [inactive]]

Can someone explain at a high level what this fix does, i.e. how much memory does it save and how often?  Thanks!

Comment 31

[Oleg Romashin (:romaxa)]

Practically this fix releasing whole layer tree for inactive tabs, or when active tab going to background together with fennec (bg application).

Layer tree for each tab usually is:
1) ThebesLayer displayport  - 1500x2000px  ~ 12*2Mb for ALPHA display port, and ~6*2Mb for opaque display port (with bug 693938 it will be always opaque 6*2mb)
2) Depends on page structure we might have more thebes, Image, Canvas layers, depends on size and content type calculation is simple (width*height*(opaque ? 2 : 4) - bytes of allocated memory per layer ) * 2 - most of IPC layers are double buffered in SW mode, and in GL mode they have Front OGL texture memory + shmem backbuffer memory.

With current fennec all these layer stay in memory for each opened tab. And most likely most android phones just killing fennec because it too fat.

With this patch all that memory released for inactive tabs, and released for last active tab if app goes to Background/Taskswitcher.

Make sense?

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 567487 [details] [diff] [review]
Don't add Background item if bounds are empty

Small, low risk patch that saves lots of memory in fennec.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 567488 [details] [diff] [review]
Send empty paint transaction when display port is empty

Small, low risk patch that saves lots of memory in fennec.

Comment 34

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/d2dd7339bedb
https://hg.mozilla.org/mozilla-central/rev/2fce0afe2141

Comment 35

[Asa Dotzler [:asa]]

Roc, Finkle, do you feel like this is really safe for Desktop? Looks like a big win for Fennec but it also looks like it's shared code and so I want to feel better about the potential risk to desktop before approving.

Comment 36

[Oleg Romashin (:romaxa)]

On desktop this is default behavior (happen without this fix).
Also on desktop DisplayPort is not in use, and that functionality 
http://mxr.mozilla.org/mozilla-central/search?string=SetDisplayPortForElement
 only called from Fennec.

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 567488 [details] [diff] [review]
Send empty paint transaction when display port is empty

Thanks for the explanation. Approving these for Aurora.

Comment 38

[Oleg Romashin (:romaxa)]

Should I land it to aurora? or who should do that?

Comment 39

[Matt Brubeck (:mbrubeck)]

Without bug 694706, this will cause a flash of checkerboard every time you switch tabs.  Is that a regression that we want to ship, or should we request approval for bug 694706 before landing this?

Comment 40

[Alex Keybl [:akeybl]]

[Triage Comment]
This bug was approved for Aurora while Firefox 9 was on that channel. If there's reason to now take this for Firefox 9 on Beta, please set approval-mozilla-beta?

Comment 41

[Matt Brubeck (:mbrubeck)]

Comment on attachment 567488 [details] [diff] [review]
Send empty paint transaction when display port is empty

Requesting approval for Beta 9.  Previously this was approved for Aurora 9 but did not land in time.  See comment 33 and comment 36 for the Aurora nomination and risk analysis.  This patch is mobile-only has has baked in the nightly channel for three weeks.

Comment 42

[Matt Brubeck (:mbrubeck)]

Comment on attachment 567487 [details] [diff] [review]
Don't add Background item if bounds are empty

Requesting approval for Beta 9; see comment 41.

Also, I should clarify that the effects of the patches are mobile-only, but the patches do touch code that is part of desktop Firefox.

Comment 43

[LegNeato]

This doesn't look to apply cleanly to beta. Please land it yourself as soon as possible.

Comment 44

[Matt Brubeck (:mbrubeck)]

This had already landed on beta but wasn't marked as fixed:
http://hg.mozilla.org/releases/mozilla-beta/rev/f8b25775b12b
http://hg.mozilla.org/releases/mozilla-beta/rev/304ce011f834