1323957 - Remove PCompositable

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

This is similar to bug 1323539. PCompositable is a heavyweight way to associate cross-channel objects. However unlike PLayer, it supports bidirectional destruction [1].

Nical, do you know why the compositor initiates actor destruction of PCompositables? If it's just an actor lifetime issue, we should be able to easily get rid of PCompositable. But I'm worried it might be something deeper.

[1] http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/layers/IPDLActor.h#34

Comment 1

[Nicolas Silva [:nical]]

Hm, I think that the reason PCompositable was made to inherit the ChildActor/ParentActor stuff was to make their destruction part of the current transaction rather than just Send_delete__'ing them as soon as the child's reference count gets to zero. This is important because if a compositable gets destroyed while the transaction is built and the delete message is sent before the transaction, you can get ugly flickering if the compositable is dead because it is replaced by a new one (as part of the transaction).

I don't see why we would need the full destruction handshake that PCompositable got as part of that since PCompositable doesn't manage some shared data like TextureClient does.

So the only thing you should have to preserve is that if a CompositableClient is destroyed during a transaction, the CompositableHost must not be destroyed before the transaction is applied on the host side.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, use typed structs for IDs

This adds CompositableHandle, which is basically the same as LayerHandle, to replace the raw uint64_t ids used in CompositableClient.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, move EditReply handling

If PCompositable is replaced with a shared ID, the most sensible place to store the id:CompositableClient mapping is ShadowLayerForwarder. Processing EditReplies would be much easier there, than in ClientLayerManager.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, don't use PCompositable WIP

Almost complete, just need to replace SendPCompositableConstructor.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, don't use PCompositable WIP

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, use CompositableHandle in ImageNotification

More uint64_t -> CompositableHandle conversion. Some of the calls to Value() will go away in the next patch.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, don't use PCompositable WIP v2

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, don't use PCompositable

This replaces PCompositable with CompositableHandle. The patch has the following major changes:

PLayerTransaction and PImageBridge no longer send PCompositable constructors. Instead, they send "NewCompositable" and "ReleaseCompositable" messages that contain an explicit id. In the ImageBridge case, this uses the "asynchronous ID" introduced in bug 1325784. CompositableForwarder expects both protocols to implement Release functions, as before.

bug 1325784 introduced a map on ImageBridgeParent from id -> CompositableHost*. That moves to CompositableParentManager, and maps id -> RefPtr<CompositableHost>, since PCompositableParent no longer keeps the host alive.

ShadowLayerForwarder now keeps a map from id -> CompositableHost*, in order to process EditReplies. It does not retain a ref because CompositableClient's destructor is responsible for clearing the mapping.

This patch should not change functionality - in theory it's just moving the automatic actor mapping in IPDL to a manual one.

The bulk of actual PCompositable removal will be in another patch. Anything removed here is minimal.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, rm PCompositable

Now that PCompositable is not used, we can remove all references to it.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8823153 [details] [diff] [review]
part 2, move EditReply handling

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

Note that bug 1325227 (which I will try land ASAP) gets rid of EditReply entirely, so this may not be necessary.

r+ anyway so that you don't need to be blocked on me.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8823379 [details] [diff] [review]
part 4, don't use PCompositable

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

Looks like FindCompositable in ShadowLayers is only used by replies, which are going away. Can we get rid of the client side hashtable entirely in that case?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6, fix debug build issues

The first issue is that CompositableHost can be freed on the main thread now, if ImageBridgeParent holds the last reference in its compositable map. It just needs to be threadsafe refcounted.

The second issue is that the last ref to ImageBridgeChild might be in the DOM, and might be freed via the cycle collector during shutdown. When this happens, nsThreadManager is already gone and ImageBridgeChild can't post a "ReleaseCompositable" message to the IPDL thread. This never came up before because when using actors, ImageBridgeChild had some previously-never-understood code to forcefully nuke all its actors. Now we know why.

Anyway, the fix here is to just track whether ImageBridgeChild is destroyed and to check that before posting tasks. It only needs to be checked on functions that might be called during shutdown (like ReleaseCompositable).

(mCalledClose goes away because IPDL does that check for us these days.)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ca5b7fcacd
Replace async image container IDs with a typed struct. (bug 1323957 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/70fab4f6d367
Move EditReply handling from ClientLayerManager to ShadowLayerForwarder. (bug 1323957 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a97bbdd54d5
Use CompositableHandle in ImageNotification. (bug 1323957 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec74a022e80
Link Compositables via IDs instead of actors. (bug 1323957 part 4, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d1615317a36
Remove PCompositable. (bug 1323957 part 5, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe93d5f82a8
Fix ImageBridgeChild memory tracking errors on shutdown. (bug 1323957 part 6, r=mattwoodrow)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing frequent GPU process leaks in dom/canvas/test on OSX and Windows.
https://treeherder.mozilla.org/logviewer.html#?job_id=68272075&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/81cb9fa2f304dace120f625953aa8b711a30dd9e

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

as a note this increased the number of constructors for linux32 builds (and reduced them on backout);
== Change summary for alert #4772 (as of January 11 2017 17:21 UTC) ==

Regressions:

  1%  compiler_metrics num_constructors linux32 pgo     99 -> 100
  1%  compiler_metrics num_constructors linux64 pgo     99 -> 100
  1%  compiler_metrics num_constructors linux32 opt     99 -> 100

Improvements:

  1%  compiler_metrics num_constructors linux32 pgo     100 -> 99
  1%  compiler_metrics num_constructors linux64 pgo     100 -> 99
  1%  compiler_metrics num_constructors linux32 opt     100 -> 99

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4772

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Joel Maher ( :jmaher) from comment #15)
> as a note this increased the number of constructors for linux32 builds (and
> reduced them on backout);
> == Change summary for alert #4772 (as of January 11 2017 17:21 UTC) ==
> 
> Regressions:
> 
>   1%  compiler_metrics num_constructors linux32 pgo     99 -> 100
>   1%  compiler_metrics num_constructors linux64 pgo     99 -> 100
>   1%  compiler_metrics num_constructors linux32 opt     99 -> 100
> 
> Improvements:
> 
>   1%  compiler_metrics num_constructors linux32 pgo     100 -> 99
>   1%  compiler_metrics num_constructors linux64 pgo     100 -> 99
>   1%  compiler_metrics num_constructors linux32 opt     100 -> 99
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=4772

There are no new static global declarations in these patches, so I'm not sure how that is being measured.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to David Anderson [:dvander] from comment #16)
> There are no new static global declarations in these patches, so I'm not
> sure how that is being measured.

What you're seeing in that case is the effects of unified compilation.  Consider:

Unified0.cpp -> a.cpp, b.cpp, ..., m.cpp
Unified1.cpp -> n.cpp, ..., z.cpp

And n.cpp and z.cpp, say, both have static global declarations that cause constructors.  The compiler will create a single static constructor for Unified1.cpp to invoke the constructors from n.cpp and z.cpp.

If m.cpp (or any other file from Unified0.cpp) gets deleted, then n.cpp will move into Unified0.cpp.  Now you will have two distinct constructors, one for Unified0.cpp and one for Unified1.cpp.

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Ah, okay. Thanks for explaining!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8205e8a9e7a3
Replace async image container IDs with a typed struct. (bug 1323957 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b930d01d6aee
Move EditReply handling from ClientLayerManager to ShadowLayerForwarder. (bug 1323957 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d531b1b53676
Use CompositableHandle in ImageNotification. (bug 1323957 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/58db09989d05
Link Compositables via IDs instead of actors. (bug 1323957 part 4, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4704ac96489a
Remove PCompositable. (bug 1323957 part 5, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b518e5d081
Fix ImageBridgeChild memory tracking errors on shutdown. (bug 1323957 part 6, r=mattwoodrow)

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

I didn't see any memory leaks on my original try push, but it's possible that there is somehow a cycle between CompositableHost and CompositableTransactionParent. In that case we should clear mCompositables before we shutdown IPC, which is basically what we did when actors were involved.

Landed with that change.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bd4c23eb9a7b23632af24a1136e53c6731432ef

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8205e8a9e7a3
https://hg.mozilla.org/mozilla-central/rev/b930d01d6aee
https://hg.mozilla.org/mozilla-central/rev/d531b1b53676
https://hg.mozilla.org/mozilla-central/rev/58db09989d05
https://hg.mozilla.org/mozilla-central/rev/4704ac96489a
https://hg.mozilla.org/mozilla-central/rev/e3b518e5d081

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/10bef3ba02f1
Follow-up to remove PCompositable entrails in the graphics branch. r=mattwoodrow?

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Please review ^ carefully, since I just monkeyed around in the code until I got it to compile and looked sorta correct

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/09088789a419
Follow-up to fix debug build bustage on the graphics branch. r=bustage