The default bug view has changed. See this FAQ.

Fennec keeps viewport ThebesLayers alive for background tabs

RESOLVED FIXED in Firefox 10

Status

Fennec Graveyard
General
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
Firefox 10
x86
Linux
Dependency tree / graph

Details

(Whiteboard: [MemShrink:P1], QA?)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 2

6 years ago
On Chrome side I only was able to catch TabParent::Deactivate....
Is it ok to resize or deactivate/clear/destroy RemoteFrameParent from there?
Whiteboard: [MemShrink]
(Assignee)

Comment 3

6 years ago
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?
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.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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...
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.
(Assignee)

Comment 8

6 years ago
I guess on desktop we just drop retained scrollable thebes layers as soon as scrolling finished...
(Assignee)

Comment 9

6 years ago
Created attachment 566753 [details]
Last creatures that keeps layers alive after switching to another tab
(Assignee)

Comment 10

6 years ago
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...?
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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?
(Assignee)

Comment 15

6 years ago
Also it looks like ContentViewer Hide does work related to Widget destroy, which automatically kill LayerManager too.
(Assignee)

Comment 16

6 years ago
oh, no I was wrong DocumentViewerImpl::Hide is not called when we switch between tabs.
(Assignee)

Comment 17

6 years ago
Created attachment 567032 [details]
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
(Assignee)

Comment 18

6 years ago
Created attachment 567034 [details] [diff] [review]
Don't add Background item if visibleRegion is empty
Attachment #567034 - Flags: feedback?(roc)
(Assignee)

Comment 19

6 years ago
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?
(Assignee)

Comment 20

6 years ago
Created attachment 567202 [details] [diff] [review]
Send empty paint transaction when display port is empty
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #567202 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Depends on: 694706
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.
(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 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.
(Assignee)

Comment 24

6 years ago
without it, I see content not repainted after switching in fennec tab
Why didn't the rootFrame->InvalidateWithFlags take care of it?
(Assignee)

Comment 26

6 years ago
Created attachment 567487 [details] [diff] [review]
Don't add Background item if bounds are empty
Attachment #567034 - Attachment is obsolete: true
Attachment #567034 - Flags: feedback?(roc)
Attachment #567487 - Flags: review?(roc)
(Assignee)

Comment 27

6 years ago
Created attachment 567488 [details] [diff] [review]
Send empty paint transaction when display port is empty
Attachment #567202 - Attachment is obsolete: true
Attachment #567202 - Flags: review?(roc)
Attachment #567488 - Flags: review?(roc)
Attachment #567487 - Flags: review?(roc) → review+
Attachment #567488 - Flags: review?(roc) → review+
(Assignee)

Comment 28

6 years ago
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)
(Assignee)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink:P1]
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dd7339bedb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fce0afe2141
Keywords: checkin-needed
Can someone explain at a high level what this fix does, i.e. how much memory does it save and how often?  Thanks!
(Assignee)

Comment 31

6 years ago
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 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.
Attachment #567487 - Flags: approval-mozilla-aurora?
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.
Attachment #567488 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d2dd7339bedb
https://hg.mozilla.org/mozilla-central/rev/2fce0afe2141
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10

Comment 35

6 years ago
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.
(Assignee)

Comment 36

6 years ago
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.

Updated

6 years ago
Attachment #567487 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 37

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

Thanks for the explanation. Approving these for Aurora.
Attachment #567488 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Whiteboard: [MemShrink:P1] → [MemShrink:P1], QA?
(Assignee)

Comment 38

6 years ago
Should I land it to aurora? or who should do that?
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?
[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 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.
Attachment #567488 - Flags: approval-mozilla-beta?
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.
Attachment #567487 - Flags: approval-mozilla-beta?
Attachment #567487 - Flags: approval-mozilla-beta?
Attachment #567487 - Flags: approval-mozilla-beta+
Attachment #567487 - Flags: approval-mozilla-aurora+
Attachment #567488 - Flags: approval-mozilla-beta?
Attachment #567488 - Flags: approval-mozilla-beta+
Attachment #567488 - Flags: approval-mozilla-aurora+

Comment 43

5 years ago
This doesn't look to apply cleanly to beta. Please land it yourself as soon as possible.
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
status-firefox9: --- → fixed
Blocks: 679923
Depends on: 712517
No longer depends on: 712517
You need to log in before you can comment on or make changes to this bug.