Closed
Bug 693930
Opened 13 years ago
Closed 13 years ago
Fennec keeps viewport ThebesLayers alive for background tabs
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox9 fixed)
RESOLVED
FIXED
Firefox 10
Tracking | Status | |
---|---|---|
firefox9 | --- | fixed |
People
(Reporter: romaxa, Assigned: romaxa)
References
Details
(Whiteboard: [MemShrink:P1], QA?)
Attachments
(4 files, 2 obsolete files)
11.16 KB,
text/plain
|
Details | |
2.82 KB,
text/plain
|
Details | |
2.53 KB,
patch
|
roc
:
review+
blassey
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
roc
:
review+
blassey
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
I guess on desktop we just drop retained scrollable thebes layers as soon as scrolling finished...
Assignee | ||
Comment 9•13 years ago
|
||
Assignee | ||
Comment 10•13 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•13 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•13 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•13 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•13 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•13 years ago
|
||
Also it looks like ContentViewer Hide does work related to Widget destroy, which automatically kill LayerManager too.
Assignee | ||
Comment 16•13 years ago
|
||
oh, no I was wrong DocumentViewerImpl::Hide is not called when we switch between tabs.
Assignee | ||
Comment 17•13 years ago
|
||
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•13 years ago
|
||
Attachment #567034 -
Flags: feedback?(roc)
Assignee | ||
Comment 19•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #567034 -
Attachment is obsolete: true
Attachment #567034 -
Flags: feedback?(roc)
Attachment #567487 -
Flags: review?(roc)
Assignee | ||
Comment 27•13 years ago
|
||
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•13 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•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Comment 29•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dd7339bedb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fce0afe2141
Keywords: checkin-needed
![]() |
||
Comment 30•13 years ago
|
||
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•13 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 32•13 years ago
|
||
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 33•13 years ago
|
||
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?
Comment 34•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d2dd7339bedb
https://hg.mozilla.org/mozilla-central/rev/2fce0afe2141
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Comment 35•13 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•13 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•13 years ago
|
Attachment #567487 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 37•13 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•13 years ago
|
Whiteboard: [MemShrink:P1] → [MemShrink:P1], QA?
Assignee | ||
Comment 38•13 years ago
|
||
Should I land it to aurora? or who should do that?
Comment 39•13 years ago
|
||
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•13 years ago
|
||
[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•13 years ago
|
||
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 42•13 years ago
|
||
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?
Updated•13 years ago
|
Attachment #567487 -
Flags: approval-mozilla-beta?
Attachment #567487 -
Flags: approval-mozilla-beta+
Attachment #567487 -
Flags: approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #567488 -
Flags: approval-mozilla-beta?
Attachment #567488 -
Flags: approval-mozilla-beta+
Attachment #567488 -
Flags: approval-mozilla-aurora+
Comment 43•13 years ago
|
||
This doesn't look to apply cleanly to beta. Please land it yourself as soon as possible.
Comment 44•13 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•