Closed Bug 1039883 Opened 5 years ago Closed 5 years ago

TiledThebesLayer does not clean all gralloc buffers when app is in background

Categories

(Core :: Graphics: Layers, defect, P1, blocker)

32 Branch
ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

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)

+++ 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.
Nominate to v1.4+. This problem should happens also on b2g-v1.4.
blocking-b2g: --- → 1.4?
See Also: → 1002430
Assignee: nobody → sotaro.ikeda.g
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.
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.
Status: NEW → ASSIGNED
(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
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)
Summary: ClientTiledThebesLayer::ClearCachedResources() does not clean all gralloc buffers → TiledThebesLayer does not clean all gralloc buffers when app is in background
See Also: → 1037154
Duplicate of this bug: 1037154
(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.
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.
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)
About the homescreen's performance problem in Comment 10, I asked a hep to BenWa. Thanks!
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
Flags: needinfo?(nical.bugzilla)
Keywords: perf
Priority: -- → P2
Whiteboard: [MemShrink] → [MemShrink][c=memory p= s= u=]
Confirmed that the patch fixes the problem on master.
(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?
(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.
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 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+
(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.
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+
(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
sorry had to back this out for test failures/assertion failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=44109385&tree=Mozilla-Inbound
(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.
(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.
(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.
(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...
blocking-b2g: 2.0? → 2.0+
Make it graphics, not sure who's tracking "General" bugs.
Component: General → Graphics: Layers
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...
Depends on: 1017781
Fix shmem leak.
Updated TiledContentHost::Detach().
Attachment #8458148 - Attachment is obsolete: true
Fix shmem leak.
Attachment #8458120 - Attachment is obsolete: true
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+
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+
(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
Add clearing TiledContentClient. Carry "nical+".
Attachment #8458861 - Attachment is obsolete: true
Attachment #8459091 - Flags: review+
Flags: needinfo?(sotaro.ikeda.g)
Add Clearing TiledContentClient. Carry "nical+".
Attachment #8458874 - Attachment is obsolete: true
Attachment #8459092 - Flags: review+
(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 :-)
https://hg.mozilla.org/mozilla-central/rev/a9c483baa100
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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]
Tapas

Please test and provide your results here
Flags: needinfo?(tkundu)
(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: 1038461
Unable to verify because this is a backend issue.
QA Whiteboard: [QAnalyst-verify-][QAnalyst-Triage?]
Flags: needinfo?(ktucker)
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.