Last Comment Bug 693930 - Fennec keeps viewport ThebesLayers alive for background tabs
: Fennec keeps viewport ThebesLayers alive for background tabs
Status: RESOLVED FIXED
[MemShrink:P1], QA?
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: Firefox 10
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on: 694706
Blocks: 679923
  Show dependency treegraph
 
Reported: 2011-10-12 00:43 PDT by Oleg Romashin (:romaxa)
Modified: 2013-12-27 14:19 PST (History)
15 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Last creatures that keeps layers alive after switching to another tab (11.16 KB, text/plain)
2011-10-13 00:07 PDT, Oleg Romashin (:romaxa)
no flags Details
Build display list with empty region and empty display port (2.82 KB, text/plain)
2011-10-14 02:18 PDT, Oleg Romashin (:romaxa)
no flags Details
Don't add Background item if visibleRegion is empty (1.40 KB, patch)
2011-10-14 02:26 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Send empty paint transaction when display port is empty (2.12 KB, patch)
2011-10-14 15:45 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Don't add Background item if bounds are empty (2.53 KB, patch)
2011-10-17 10:40 PDT, Oleg Romashin (:romaxa)
roc: review+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Send empty paint transaction when display port is empty (1.97 KB, patch)
2011-10-17 10:40 PDT, Oleg Romashin (:romaxa)
roc: review+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2011-10-12 00:43:02 PDT
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) 2011-10-12 13:25:27 PDT
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) 2011-10-12 15:05:16 PDT
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) 2011-10-12 15:47:12 PDT
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 2011-10-12 15:50:53 PDT
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) 2011-10-12 16:31:55 PDT
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) 2011-10-12 18:19:11 PDT
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 2011-10-12 18:22:39 PDT
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) 2011-10-12 19:21:30 PDT
I guess on desktop we just drop retained scrollable thebes layers as soon as scrolling finished...
Comment 9 Oleg Romashin (:romaxa) 2011-10-13 00:07:18 PDT
Created attachment 566753 [details]
Last creatures that keeps layers alive after switching to another tab
Comment 10 Oleg Romashin (:romaxa) 2011-10-13 01:14:04 PDT
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) 2011-10-13 21:17:31 PDT
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) 2011-10-13 21:45:48 PDT
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) 2011-10-13 21:55:18 PDT
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) 2011-10-13 21:58:44 PDT
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) 2011-10-13 22:01:00 PDT
Also it looks like ContentViewer Hide does work related to Widget destroy, which automatically kill LayerManager too.
Comment 16 Oleg Romashin (:romaxa) 2011-10-14 00:09:26 PDT
oh, no I was wrong DocumentViewerImpl::Hide is not called when we switch between tabs.
Comment 17 Oleg Romashin (:romaxa) 2011-10-14 02:18:11 PDT
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
Comment 18 Oleg Romashin (:romaxa) 2011-10-14 02:26:34 PDT
Created attachment 567034 [details] [diff] [review]
Don't add Background item if visibleRegion is empty
Comment 19 Oleg Romashin (:romaxa) 2011-10-14 02:32:48 PDT
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) 2011-10-14 15:45:26 PDT
Created attachment 567202 [details] [diff] [review]
Send empty paint transaction when display port is empty
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-16 21:57:14 PDT
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) 2011-10-16 21:59:00 PDT
(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) 2011-10-16 22:19:35 PDT
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) 2011-10-16 22:25:34 PDT
without it, I see content not repainted after switching in fennec tab
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-17 05:35:54 PDT
Why didn't the rootFrame->InvalidateWithFlags take care of it?
Comment 26 Oleg Romashin (:romaxa) 2011-10-17 10:40:16 PDT
Created attachment 567487 [details] [diff] [review]
Don't add Background item if bounds are empty
Comment 27 Oleg Romashin (:romaxa) 2011-10-17 10:40:58 PDT
Created attachment 567488 [details] [diff] [review]
Send empty paint transaction when display port is empty
Comment 28 Oleg Romashin (:romaxa) 2011-10-18 09:05:07 PDT
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 30 Nicholas Nethercote [:njn] 2011-10-18 15:11:33 PDT
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) 2011-10-18 15:32:52 PDT
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?) 2011-10-18 19:22:48 PDT
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?) 2011-10-18 19:23:07 PDT
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 35 Asa Dotzler [:asa] 2011-10-20 14:40:19 PDT
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) 2011-10-20 14:56:18 PDT
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] 2011-10-20 15:11:26 PDT
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) 2011-11-07 16:51:37 PST
Should I land it to aurora? or who should do that?
Comment 39 Matt Brubeck (:mbrubeck) 2011-11-07 16:54:36 PST
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] 2011-11-08 11:58:53 PST
[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) 2011-11-08 16:21:22 PST
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) 2011-11-08 16:22:55 PST
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 christian 2011-11-15 17:15:06 PST
This doesn't look to apply cleanly to beta. Please land it yourself as soon as possible.
Comment 44 Matt Brubeck (:mbrubeck) 2011-11-15 17:23:20 PST
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

Note You need to log in before you can comment on or make changes to this bug.