1176011 - Share TextureClientPools between tabs on e10s

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 2

[Jim Mathies [:jimm]]

speculative upside here, and could be tricky with multi-process. downgrading to P3.

Comment 3

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Move-TextureClientPool-to-CompositorBridgeChild.patch

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

Comment 4

[George Wright (:gw280) (needinfo me!)]

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.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Move-TextureClientPool-to-CompositorBridgeChild.patch

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.

Comment 6

[Nicolas Silva [:nical]]

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.

Comment 7

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Move-TextureClientPool-to-CompositorBridgeChild.patch

Crashes on shutdown atm.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/643e79a2d47a
Move TextureClientPool to CompositorBridgeChild r=nical

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/376b59c84c6f
Revert "Bug 1176011 - Move TextureClientPool to CompositorBridgeChild r=nical" on a CLOSED TREE

Comment 10

[George Wright (:gw280) (needinfo me!)]

I had to revert this because it didn't play nicely with nical's changes from bug 1272600 a few days ago.

Comment 11

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1176011-Move-TextureClientPool-to-CompositorBrid.patch

Your changes from bug 1272600 broke this, and enough has changed in the patch that I don't feel comfortable carrying forward an r+.

Comment 12

[Nicolas Silva [:nical]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a85da8081c68
Move TextureClientPool to CompositorBridgeChild r=nical

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29812097&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0fc657e200

Comment 15

[George Wright (:gw280) (needinfo me!)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7c8ce2b6e82

Looks like this should be fine now.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d167fbd0e4
Move TextureClientPool to CompositorBridgeChild r=nical

Comment 17

[Phil Ringnalda (:philor)]

Non-exhaustive list of tests that crashed:

https://treeherder.mozilla.org/logviewer.html#?job_id=29995364&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30000190&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30004402&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30004204&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30001951&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30003017&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30006149&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30005640&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30008892&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=30008741&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c0590e21cf

Comment 18

[George Wright (:gw280) (needinfo me!)]

(╯°□°）╯︵ ┻━┻

Honestly not trying to annoy everyone here.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada00a4c36c5
Move TextureClientPool to CompositorBridgeChild r=nical

Comment 20

[George Wright (:gw280) (needinfo me!)]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ada00a4c36c5