Closed
Bug 1039883
Opened 10 years ago
Closed 10 years ago
TiledThebesLayer does not clean all gralloc buffers when app is in background
Categories
(Core :: Graphics: Layers, defect, P1)
Tracking
()
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
(Keywords: memory-footprint, perf, Whiteboard: [c=memory p= s=2014.08.01.t u=2.0] [MemShrink])
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1038958 +++ ClientTiledThebesLayer::ClearCachedResources() does not clean all buffers. It clean only back buffers. Front buffers are kept. If some applications use tiled layers and they are in background states, all of them consume gralloc buffers even when they are in background. But fixing this problem causes performance regression on the following use case. - Transition from app to homescreen This regression should be handled by Bug 1002430.
Assignee | ||
Comment 1•10 years ago
|
||
Nominate to v1.4+. This problem should happens also on b2g-v1.4.
blocking-b2g: --- → 1.4?
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 2•10 years ago
|
||
The patch clears all gralloc buffers when app go to background. The patch is created for b2g-v2.0. I confirmed that the patch works on v2.0 flame.
Assignee | ||
Comment 3•10 years ago
|
||
attachment 8457715 [details] [diff] [review] did 2 things - [1] Free all GrallocTextureClientOGL when an application go to background - [2] un-map gralloc buffer Without, DropGrallocBuffer() gralloc buffers are still mapped. It says that Host sides are not destroyed in this use case. The host sides also have to be destroyed in this use case. Need to investigate about it more.
Blocks: CAF-v2.0-FC-metabug
status-b2g-v2.0:
--- → affected
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #3) > attachment 8457715 [details] [diff] [review] did 2 things > - [1] Free all GrallocTextureClientOGL when an application go to background > - [2] un-map gralloc buffer > > Without, DropGrallocBuffer() gralloc buffers are still mapped. It says that > Host sides are not destroyed in this use case. The host sides also have to > be destroyed in this use case. Need to investigate about it more. Another problem is - When LayerManagerComposite::ClearCachedResources() is called in Compositor side, the function call does not clean up the gralloc buffers. ThebesLayerComposite::CleanupResources() does not clean up TiledLayerBufferComposite's gralloc buffers. Detach() does not clean up the buffers. And TiledLayerBufferComposite is not destroyed, because CompositableParent hold a pointer to the TiledLayerBufferComposite. TiledContentClient is still exist, then it keeps CompositableParent/CompositableChild valid. ----------------------------------------------------- void ThebesLayerComposite::CleanupResources() { if (mBuffer) { mBuffer->Detach(this); } mBuffer = nullptr; } http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ThebesLayerComposite.cpp#175
Assignee | ||
Comment 5•10 years ago
|
||
Nical, On host side ClearCachedResources() seems do nothing about freeing TextureHost except forgetting reference to CompositableHost. Can you give us a comment about it?
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Updated•10 years ago
|
Summary: ClientTiledThebesLayer::ClearCachedResources() does not clean all gralloc buffers → TiledThebesLayer does not clean all gralloc buffers when app is in background
Assignee | ||
Updated•10 years ago
|
Keywords: footprint
Whiteboard: [MemShrink]
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #1) > Nominate to v1.4+. This problem should happens also on b2g-v1.4. And this bug blocks b2g-v2.0+ bug.
Assignee | ||
Comment 8•10 years ago
|
||
non-tiled thebes layer case, ContentClient just releases ContentClient. http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientThebesLayer.h#74 http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientThebesLayer.h#110
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8457715 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
attachment 8458120 [details] [diff] [review] releases all gralloc buffers when an application is in background. But it regress the performance of application transition from an application to the homescreen. Before applying the patch, homescreen's tiled layer's front buffers are not released. But after applying the patch, any application's(including homescreen) gralloc buffers are released when the application go to background. Threfore, homescreen need to be redrawn from scratch during transition to the homescreen. The cost of redrawing homescreen seems very expensive.
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8458120 [details] [diff] [review] patch for b2g v2.0 - release Tiled layer's gralloc when an application is background Can you reiview the patch soon?
Attachment #8458120 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 12•10 years ago
|
||
About the homescreen's performance problem in Comment 10, I asked a hep to BenWa. Thanks!
Assignee | ||
Updated•10 years ago
|
Attachment #8458120 -
Attachment description: patch - release Tiled layer's gralloc when an application is background → patch for b2g v2.0 - release Tiled layer's gralloc when an application is background
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=314c991923f4
Updated•10 years ago
|
Comment 15•10 years ago
|
||
Confirmed that the patch fixes the problem on master.
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #12) > About the homescreen's performance problem in Comment 10, I asked a hep to > BenWa. Thanks! It was my misunderstanding. I compared the default ROM and a ROM that the patch applied. Performance seems same.
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #11) > Comment on attachment 8458120 [details] [diff] [review] > patch for b2g v2.0 - release Tiled layer's gralloc when an application is > background > > Can you reiview the patch soon? I am running automation with this fix . I will update soon.
This patch seems to be working fine in my local testing. Can we land it if you are ok?
Comment 19•10 years ago
|
||
(In reply to Tapas Kumar Kundu from comment #18) > This patch seems to be working fine in my local testing. Can we land it if > you are ok? I am trying to get this r+'ed but it may not happen today(for 5pm) due to timezone isssues. hence given comment #18 you should be ok to pick this up.
Comment 20•10 years ago
|
||
According to comment 0 a series of other patches would also need to land together if we take this on 1.4, which might be too much for now. Pushing to 2.0 as requested in bug 1038958
blocking-b2g: 1.4? → 2.0?
Comment 21•10 years ago
|
||
Comment on attachment 8458120 [details] [diff] [review] patch for b2g v2.0 - release Tiled layer's gralloc when an application is background Review of attachment 8458120 [details] [diff] [review]: ----------------------------------------------------------------- My understanding is that we intentionally did not discard front buffers when switching between apps to have smooth app transitions and only discarded them on memory pressure events. Doesn't that do the job (even if we are using more memory than if we discarded all buffers right away, it's memory that is reclaimed when we need it)? I am okay with change, as long as the user experience is good and you guys did some testing. I wonder why reclaiming memory on memory pressure events isn't preferable, though.
Attachment #8458120 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #21) > I am okay with change, as long as the user experience is good and you guys > did some testing. I wonder why reclaiming memory on memory pressure events > isn't preferable, though. In my understanding, by current gecko's implementation, even when there are memory pressure events happens, the buffers are not released except applications are killed. And gralloc buffer allocation in graphic buffer also means gralloc buffer allocation in b2g process. Therefore, many application's gralloc buffer allocation causes b2g process's too much gralloc buffers allocation. It could cause b2g process could be target of low memory killer. And consuming too much gralloc buffers is not a good for a platform. It could cause another problems.
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8458148 [details] [diff] [review] patch for master - release Tiled layer's gralloc when an application is background Carry "r=nical".
Attachment #8458148 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d65a1bbb01
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #22) > (In reply to Nicolas Silva [:nical] from comment #21) > > I am okay with change, as long as the user experience is good and you guys > > did some testing. I wonder why reclaiming memory on memory pressure events > > isn't preferable, though. > > In my understanding, by current gecko's implementation, even when there are > memory pressure events happens, the buffers are not released except > applications are killed. And gralloc buffer allocation in graphic buffer > also means gralloc buffer allocation in b2g process. Therefore, many > application's gralloc buffer allocation causes b2g process's too much > gralloc buffers allocation. It could cause b2g process could be target of > low memory killer. And consuming too much gralloc buffers is not a good for > a platform. It could cause another problems. When application go to background, "low-memory" event is sent to the application. In homescreen case, all applicaiton's icons are freed. when the applications are back to foreground, these resources are re-loaded again. Therefore it take time already. This change might not cause regression because of this. http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#1109
Comment 26•10 years ago
|
||
sorry had to back this out for test failures/assertion failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=44109385&tree=Mozilla-Inbound
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #26) > sorry had to back this out for test failures/assertion failures like > https://tbpl.mozilla.org/php/getParsedLog.php?id=44109385&tree=Mozilla- > Inbound Hmm, it seems Bug 1017781.
Assignee | ||
Comment 28•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d4122cbf657f
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #26) > sorry had to back this out for test failures/assertion failures like > https://tbpl.mozilla.org/php/getParsedLog.php?id=44109385&tree=Mozilla- > Inbound It seems that the problem just hit same sanity checks during shutdown.
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #29) > (In reply to Carsten Book [:Tomcat] from comment #26) > > sorry had to back this out for test failures/assertion failures like > > https://tbpl.mozilla.org/php/getParsedLog.php?id=44109385&tree=Mozilla- > > Inbound > > It seems that the problem just hit same sanity checks during shutdown. I think that I is a sanity check's problem and not leak buffer actually.
Comment 31•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #30) > (In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #29) > > (In reply to Carsten Book [:Tomcat] from comment #26) > > > sorry had to back this out for test failures/assertion failures like > > > https://tbpl.mozilla.org/php/getParsedLog.php?id=44109385&tree=Mozilla- > > > Inbound > > > > It seems that the problem just hit same sanity checks during shutdown. > > I think that I is a sanity check's problem and not leak buffer actually. Let's clear out the sanity check then, if it's making things insane...
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Comment 32•10 years ago
|
||
Make it graphics, not sure who's tracking "General" bugs.
Component: General → Graphics: Layers
Assignee | ||
Comment 33•10 years ago
|
||
By irc with nical, I received the following information. - Shmem is used by gfxShmSharedReadLock's to store read lock counter. - Shemm might be cleaned correctly in Client side. But the cleaning up might be not completed yet by host side. The above thing is not related to the patch change. The patch seems to trigger the crash just by changing the timing...
Assignee | ||
Comment 34•10 years ago
|
||
Fix shmem leak. Updated TiledContentHost::Detach().
Attachment #8458148 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1ab0e9f97526
Assignee | ||
Comment 36•10 years ago
|
||
Fix shmem leak.
Attachment #8458120 -
Attachment is obsolete: true
Assignee | ||
Comment 37•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b764e12796d
Assignee | ||
Comment 38•10 years ago
|
||
Comment on attachment 8458861 [details] [diff] [review] patch for master - release Tiled layer's gralloc when an application is background Carry "r=nical".
Attachment #8458861 -
Flags: review+
Assignee | ||
Comment 39•10 years ago
|
||
Comment on attachment 8458874 [details] [diff] [review] patch for b2g v2.0 - release Tiled layer's gralloc when an application is background Carry "r=nical".
Attachment #8458874 -
Flags: review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f38a75b454 for b2g mochitest failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=44138526&tree=Mozilla-Inbound
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Wes Kocher (:KWierso) from comment #40) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f38a75b454 for b2g > mochitest failures: > https://tbpl.mozilla.org/php/getParsedLog.php?id=44138526&tree=Mozilla- > Inbound We will wait for final fixes. We are also running some internal stability testing with patch from #comment 39
Assignee | ||
Comment 42•10 years ago
|
||
Add clearing TiledContentClient. Carry "nical+".
Attachment #8458861 -
Attachment is obsolete: true
Attachment #8459091 -
Flags: review+
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 43•10 years ago
|
||
Add Clearing TiledContentClient. Carry "nical+".
Attachment #8458874 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8459092 -
Flags: review+
Assignee | ||
Comment 44•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=fc36d780a79e
Assignee | ||
Comment 45•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #44) > https://tbpl.mozilla.org/?tree=Try&rev=fc36d780a79e Test failures are fixed :-)
Assignee | ||
Comment 46•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c483baa100
Comment 47•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a9c483baa100
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 48•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0bb536942e2
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
status-firefox33:
--- → fixed
Updated•10 years ago
|
Severity: normal → blocker
Priority: P2 → P1
Whiteboard: [MemShrink][c=memory p= s= u=] → [c=memory p= s=2014.08.01.t u=2.0] [MemShrink]
(In reply to Preeti Raghunath(:Preeti) from comment #49) > Tapas > > Please test and provide your results here This patch looks fine to me but we are hitting another memleak in bug 1041751
Flags: needinfo?(tkundu)
Blocks: 1058366
No longer blocks: 1058366
Comment 51•10 years ago
|
||
Unable to verify because this is a backend issue.
QA Whiteboard: [QAnalyst-verify-][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-verify-][QAnalyst-Triage?] → [QAnalyst-verify-][QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in
before you can comment on or make changes to this bug.
Description
•