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

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.

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

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.

Almost complete, just need to replace SendPCompositableConstructor.

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

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.

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

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 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?

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.)

Pushed by danderson@mozilla.com: 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)

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

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

(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.

(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.

Ah, okay. Thanks for explaining!

Pushed by danderson@mozilla.com: 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)

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

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

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

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

Pushed by kgupta@mozilla.com: https://hg.mozilla.org/projects/graphics/rev/09088789a419 Follow-up to fix debug build bustage on the graphics branch. r=bustage