Closed Bug 1154231 Opened 9 years ago Closed 9 years ago

[B2G] Collect layers data only when needed

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

PoC patch
5.70 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1154231 - Part 1. Reclaim cached resources when memory-pressure occurs. r=mattwoodrow
39 bytes, text/x-review-board-request
kanru
: review+
Details
MozReview Request: Bug 1154231 - Part 2. Use LRU to manage remote layers. r=mattwoodrow
39 bytes, text/x-review-board-request
kanru
: review+
Details 
Currently the layers data in a content process is destroyed when the process is sent to background. This caused bug 1034001 and others. I propose we revert that change and use a smarter scheme to reclaim memories. For example when we need more memories for current process we could send a message to other child process and reclaim their layers. I think there is already a bug opened by vivien but I don't remember which one.
(In reply to Kan-Ru Chen [:kanru] from comment #0)
> I think there is already a bug
> opened by vivien but I don't remember which one.
Flags: needinfo?(21)
Whiteboard: [gfx-noted]
Attached patch PoC patch — — Splinter Review 
Bill, looks like this approach will affect how e10s handles tabswitch. What do you think?
Flags: needinfo?(wmccloskey)
I don't think this is feasible on desktop. Some people have hundreds of tabs open at once. I don't think we could store the layers for all of them. My understanding is that textures can be quite large. And memory pressure events on desktop are not at all reliable. If they happen at all, it's usually so late that we can't do anything about them.

If we make this change, it should either be restricted to b2g or else we need a better way of deciding when to throw away layers.
Flags: needinfo?(wmccloskey)
Tentative plan:

Add a LRU to chrome process or each CompositorParent. We keep at most N remote layer tree in the LRU. If we grow over the size limit, the oldest layer tree is evicted. If we encounter memory-pressure we also evict the oldest layer tree in the LRU until memory-pressure is over. On desktop we could set N to 0 initially, on b2g we could set it to 5, for example.
Attached file MozReview Request: bz://1154231/kanru (obsolete) — 
/r/8329 - Bug 1154231 - Part 1. Reclaim cached resources when memory-pressure occurs. r=mattwoodrow
/r/8331 - Bug 1154231 - Part 2. Use LRU to manage remote layers. r=mattwoodrow

Pull down these commits:

hg pull -r 6e612a159e2a796e88da6280a85e5bebb9be05e9 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602541 - Flags: review?(matt.woodrow)
Assignee: nobody → kchen
Hi Matt, do you have time to review this or give feedback? Should I redirect the review to others?
Flags: needinfo?(matt.woodrow)
https://reviewboard.mozilla.org/r/8331/#review7395

Ship It!

::: b2g/app/b2g.js:1144
(Diff revision 1)
> +pref("layers.compositor-lru-size", 0);

Do you want the value of this pref to be 10?

::: gfx/layers/ipc/CompositorLRU.cpp:23
(Diff revision 1)
> +CompositorLRU::Init()

Do we even need this? Could just skip it and have CompositorLRU be lazily initialized.
https://reviewboard.mozilla.org/r/8331/#review7405

::: b2g/app/b2g.js:1144
(Diff revision 1)
> +pref("layers.compositor-lru-size", 0);

Ah, yes. I was testing pref 0 on the device.
https://reviewboard.mozilla.org/r/8331/#review7409

::: gfx/layers/ipc/CompositorLRU.cpp:23
(Diff revision 1)
> +CompositorLRU::Init()

Couldn't use Preferences on the compositor thread so I will keep this.
Comment on attachment 8602541 [details]
MozReview Request: bz://1154231/kanru

/r/8329 - Bug 1154231 - Part 1. Reclaim cached resources when memory-pressure occurs. r=mattwoodrow
/r/8331 - Bug 1154231 - Part 2. Use LRU to manage remote layers. r=mattwoodrow

Pull down these commits:

hg pull -r 20bc98fda9ccaf107924ca8d6ffcd1d258dd010c https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602541 - Flags: review+
Comment on attachment 8602541 [details]
MozReview Request: bz://1154231/kanru

https://reviewboard.mozilla.org/r/8327/#review7417

Ship It!
On desktop we try to load the layers for only one tab at a time. We wait until the ClearCachedResources request for an old tab has been processed before we request the layers for a new tab. I think this patch makes us go through the event loop one extra time before that happens, which can be expensive if the child process is busy.

Could we add a special case for desktop so that, if compositor-lru-size == 0, we immediately do ClearCachedResources in the child?
(In reply to Bill McCloskey (:billm) from comment #15)
> On desktop we try to load the layers for only one tab at a time. We wait
> until the ClearCachedResources request for an old tab has been processed
> before we request the layers for a new tab. I think this patch makes us go
> through the event loop one extra time before that happens, which can be
> expensive if the child process is busy.
> 
> Could we add a special case for desktop so that, if compositor-lru-size ==
> 0, we immediately do ClearCachedResources in the child?

Yeah, that is suboptimal. I will fix it in an followup.
Flags: needinfo?(matt.woodrow)
Depends on: 1164763
Depends on: 1166649
The patch seems to check only "memory-pressure". On recent gonk, we could get more gralloc than old gonk. But on older gonk, we faced out-of-gralloc even on current layer management.
(In reply to Sotaro Ikeda [:sotaro] from comment #54)
> The patch seems to check only "memory-pressure". On recent gonk, we could
> get more gralloc than old gonk. But on older gonk, we faced out-of-gralloc
> even on current layer management.

I heard that the new ion memory allocator shares system memory. So memory-pressure would not be a too bad indicator.
https://hg.mozilla.org/mozilla-central/rev/77655fe1aab5
https://hg.mozilla.org/mozilla-central/rev/b9d272c24804
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8602541 - Flags: review?(matt.woodrow)
Does this means we can remove some of the deferred |setVisible(false)| in app window management? Alive could you assign people to do so?
Flags: needinfo?(alive)
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #59)
> Does this means we can remove some of the deferred |setVisible(false)| in
> app window management? Alive could you assign people to do so?

I read the bug but I am not sure.. looks like it's still gecko drops the layer data but system does not know the timing to drop. Since we don't know the timing and we don't know which frame is having the layer data still or not, we can not drop the setVisible(false) call. Kanru, could you confirm?
Flags: needinfo?(alive) → needinfo?(kchen)
I don't think we can remove the setVisible(false), but rather remove the various workarounds that kept frame active.
Okay, filing a bug.
Flags: needinfo?(kchen)

        Attachment #8602541 -
        Attachment is obsolete: true

        Attachment #8620035 -
        Flags: review+

        Attachment #8620036 -
        Flags: review+
Blocks: 1176019
See Also:  → 1179040

    
    

    
  
(In reply to Kan-Ru Chen [:kanru] from comment #0)
> Currently the layers data in a content process is destroyed when the process
> is sent to background. This caused bug 1034001 and others. I propose we
> revert that change and use a smarter scheme to reclaim memories. For example
> when we need more memories for current process we could send a message to
> other child process and reclaim their layers. I think there is already a bug
> opened by vivien but I don't remember which one.

For the record, it was bug 1002430.

The LRU impl is much better than keeping everything all the time as I did in the bug. Thanks for that.
Flags: needinfo?(21)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: