Closed Bug 693930 Opened 13 years ago Closed 13 years ago

Fennec keeps viewport ThebesLayers alive for background tabs


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox9 fixed)

Firefox 10
Tracking Status
firefox9 --- fixed


(Reporter: romaxa, Assigned: romaxa)



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


(4 files, 2 obsolete files)

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
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?
On Chrome side I only was able to catch TabParent::Deactivate....
Is it ok to resize or deactivate/clear/destroy RemoteFrameParent from there?
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.
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.
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.
I guess on desktop we just drop retained scrollable thebes layers as soon as scrolling finished...
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...?
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
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.
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.
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?
Also it looks like ContentViewer Hide does work related to Widget destroy, which automatically kill LayerManager too.
oh, no I was wrong DocumentViewerImpl::Hide is not called when we switch between tabs.
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
Attachment #567034 - Flags: feedback?(roc)
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,

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: nobody → romaxa
Attachment #567202 - Flags: review?(roc)
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)
> 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.
without it, I see content not repainted after switching in fennec tab
Why didn't the rootFrame->InvalidateWithFlags take care of it?
Attachment #567034 - Attachment is obsolete: true
Attachment #567034 - Flags: feedback?(roc)
Attachment #567487 - Flags: review?(roc)
Attachment #567202 - Attachment is obsolete: true
Attachment #567202 - Flags: review?(roc)
Attachment #567488 - Flags: review?(roc)
Cool, tested try build
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)
Keywords: checkin-needed
Whiteboard: [MemShrink] → [MemShrink:P1]
Can someone explain at a high level what this fix does, i.e. how much memory does it save and how often?  Thanks!
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?
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
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.
On desktop this is default behavior (happen without this fix).
Also on desktop DisplayPort is not in use, and that functionality
 only called from Fennec.
Attachment #567487 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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+
Whiteboard: [MemShrink:P1] → [MemShrink:P1], QA?
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+
This doesn't look to apply cleanly to beta. Please land it yourself as soon as possible.
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.