Share TextureClientPools between tabs on e10s

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mstange, Assigned: gw280)

Tracking

(Blocks 2 bugs, {feature})

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox41 affected, firefox50 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(2 attachments, 2 obsolete attachments)

TextureClientPools are currently owned by a single LayerManager.

Pre-e10s, there was one LayerManager for each window and all tha tabs shared that LayerManager.

With e10s, there is one LayerManager per TabChild, and each creates their own TextureClientPools. There is no sharing of TextureClientPools between LayerManagers.

Also, when a tab is hidden, TabChild::MakeHidden / CompositorLRU triggers a call to ClientLayerManager::ClearCachedResources, which also clears all its TextureClientPools.
Whiteboard: gfx-noted
Once we go to one process per tab, whatever we do in this bug would go away, right?  In other words, do we want to share?
Assignee: nobody → gwright
Priority: -- → P2
speculative upside here, and could be tricky with multi-process. downgrading to P3.
Priority: P2 → P3
Depends on: 1264764
This doesn't currently work, I get a lot of messages:

Crash Annotation GraphicsCriticalError: |[C0][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=1.87596) |[C121][GFX1]: ValidateTile failed (t=10.4655) |[C122][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=10.4656) |[C123][GFX1]: ValidateTile failed (t=10.4656) |[C124][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=10.4835) |[C125][GFX1]: ValidateTile failed (t=10.4836) |[C126][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=10.4984) |[C127][GFX1]: ValidateTile failed (t=10.4986) |[C128][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=10.5143) |[C129][GFX1]: ValidateTile failed (t=10.5144) |[C130][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=10.5318) |[C116][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=7.51398) |[C117][GFX1]: ValidateTile failed (t=7.51407) |[C118][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=7.53088) |[C119][GFX1]: ValidateTile failed (t=7.53097) |[C120][GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B) (t=10.4654) [GFX1]: [Tiling:Client] Failed to allocate a TextureClient (B)

It does build and run though. So that's good.

The gist of this patch is:

- Create a new class called TextureForwarder, and move a bunch of methods from CompositableForwarder to TextureForwarder.
- Move TextureClientPool under CompositorBridgeChild, which is a singleton in the child process.
- Change TextureClientPool/TextureClient/TextureChild from using a CompositableForwarder to a TextureForwarder as its texture allocator
- Make CompositorBridgeChild a TextureForwarder and use that when creating the TextureClientPool
Attachment #8756449 - Flags: feedback?(nical.bugzilla)
Eventually we're going to want to isolate TextureForwarder and CompositableForwarder (in this patch, CompositableForwarder is a subclass of TextureForwarder), but I think that should be handled in a followup bug.
Turns out I had forgotten to implement ShmemAllocator on CompositorBridgeChild.

This works, and I've hooked up the memory pressure handling and other code into CompositorBridgeChild now so it should handle the TextureClientPool like it did before.
Attachment #8756449 - Attachment is obsolete: true
Attachment #8756449 - Flags: feedback?(nical.bugzilla)
Attachment #8756495 - Flags: review?(nical.bugzilla)
Comment on attachment 8756495 [details] [diff] [review]
0001-Move-TextureClientPool-to-CompositorBridgeChild.patch

Review of attachment 8756495 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except for the confusion around TextureForwarder and CompositableForwarder. You need to keep both of them around and anything that is related to transactions must be done by the CompositableForwarder, not the TextureForwarder.

::: gfx/layers/IPDLActor.h
@@ +48,5 @@
>    /// The normal way to destroy the actor.
>    ///
>    /// This will asynchronously send a Destroy message to the parent actor, whom
>    /// will send the delete message.
> +  void Destroy(TextureForwarder* aFwd = nullptr)

Here you need to pass a CompositableForwarder (which is what knows about transactions). TextureForwarder can just allocate textures but it doesn't have transactions.

@@ +65,5 @@
>    ///
>    /// This will block until the Parent actor has handled the Destroy message,
>    /// and then start the asynchronous handshake (and destruction will already
>    /// be done on the parent side, when the async part happens).
> +  void DestroySynchronously(TextureForwarder* aFwd = nullptr)

Same here (need a CompositableForwarder).

::: gfx/layers/client/TextureClient.cpp
@@ +133,5 @@
>      mWaitForRecycle = nullptr;
>      Unlock();
>    }
>  
> +  TextureForwarder* GetForwarder() { return mForwarder; }

Here you probably need two things: the TextureForwarder and the CompositableForwarder. The name GetForwarder is certainly better suited for the abstraction that has a notion of transactions (this is what forwarder originally meant), so an extra GetTextureForwarder() along with the existing GetForwarder() is what would make most sense in my opinion.

@@ +164,5 @@
>    }
>  
>    mutable gfx::CriticalSection mLock;
>  
> +  RefPtr<TextureForwarder> mForwarder;

Here you need both a RefPtr<TextureForwarder> and a RefPtr<CompositableForwarder>. This is because the TextureForwarder is what you will use for all allocation purposes, and the CompositableForwarder is what you pass to the Destroy and DestroyInTransaction methods implemented in IPDLActor.h

::: gfx/layers/ipc/CompositableForwarder.h
@@ -84,5 @@
>                                  const gfx::IntRect& aPictureRect) = 0;
>  #endif
>  
> -  virtual bool DestroyInTransaction(PTextureChild* aTexture, bool synchronously) = 0;
> -  virtual bool DestroyInTransaction(PCompositableChild* aCompositable, bool synchronously) = 0;

This needs to stay. CompositableForwarder is the right abstraction for this because it is where we handle transactions.
Looks like this patch always fails to bundle texture destructions in the current transaction which will cause flickering.

::: gfx/layers/ipc/TextureForwarder.h
@@ +35,5 @@
> +    TextureFlags aFlags) = 0;
> +
> +
> +  virtual bool DestroyInTransaction(PTextureChild* aTexture, bool synchronously) = 0;
> +  virtual bool DestroyInTransaction(PCompositableChild* aCompositable, bool synchronously) = 0;

This is the wrong abstraction for this API. This should stay at the CompositableForwarder level which is where the transactions are managed.
Attachment #8756495 - Flags: review?(nical.bugzilla) → review-
Crashes on shutdown atm.
Attachment #8756495 - Attachment is obsolete: true
Attachment #8758465 - Flags: review?(nical.bugzilla)
Attachment #8758465 - Flags: review?(nical.bugzilla) → review+
Blocks: 1276811
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/643e79a2d47a
Move TextureClientPool to CompositorBridgeChild r=nical
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/376b59c84c6f
Revert "Bug 1176011 - Move TextureClientPool to CompositorBridgeChild r=nical" on a CLOSED TREE
I had to revert this because it didn't play nicely with nical's changes from bug 1272600 a few days ago.
Your changes from bug 1272600 broke this, and enough has changed in the patch that I don't feel comfortable carrying forward an r+.
Attachment #8761022 - Flags: review?(nical.bugzilla)
Comment on attachment 8761022 [details] [diff] [review]
0001-Bug-1176011-Move-TextureClientPool-to-CompositorBrid.patch

Review of attachment 8761022 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/TextureClient.cpp
@@ +1370,4 @@
>  {
>    MOZ_COUNT_CTOR(ShmemTextureReadLock);
>    MOZ_ASSERT(mAllocator);
> +  if (mAllocator && mAllocator->AsTextureForwarder()) {

If mAllocator->AsTextureForwarder() is null in this constructor it's a bug and it should never happen. I'm not sure why we null-checked mAllocator, but either don't check, or add a MOZ_ASSERT(mAllocator->AsTextureForwarder()) to make sure we catch the issue.
Attachment #8761022 - Flags: review?(nical.bugzilla) → review+
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85da8081c68
Move TextureClientPool to CompositorBridgeChild r=nical
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7c8ce2b6e82

Looks like this should be fine now.
Flags: needinfo?(gwright)
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d167fbd0e4
Move TextureClientPool to CompositorBridgeChild r=nical
(╯°□°)╯︵ ┻━┻

Honestly not trying to annoy everyone here.
See Also: → 1261166
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada00a4c36c5
Move TextureClientPool to CompositorBridgeChild r=nical
Full try run was nice and green this time. The M1 crash on OS X seems to be pre-existing based on treeherder's inbound and central history at least over the last day. Same for the dt2 linux-x64-opt crash.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d51746f20bfb
See Also: → 1280715
https://hg.mozilla.org/mozilla-central/rev/ada00a4c36c5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1281169
No longer blocks: 1281169
Depends on: 1281169
Depends on: 1281775
Depends on: 1293908
See Also: → 1324896
You need to log in before you can comment on or make changes to this bug.