Closed Bug 1299594 Opened 5 years ago Closed 5 years ago

remove dead CloneManagees and related code

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files, 1 obsolete file)

As suggested in bug 1297804 comment 9, all this stuff is dead code, so we should get rid of it.
CloneOpenedToplevels, which is never called, is the only interesting
caller of CloneToplevel.  And CloneToplevel, in turn, is the only
interesting caller of CloneManagees.  Which means we can ditch all this
code for a decent amount of space savings, both in code and writable
static data (no more useless virtual function entries in vtables).

This part is the big win, about 128K on x86-64 Linux.
Attachment #8786901 - Flags: review?(wmccloskey)
The only thing we needed opened actor tracking for was the ability to
clone all the actors.  But now that we no longer have support for
cloning actors, we no longer need to track the actors that we've cloned,
which makes a number of things significantly simpler.

I'm less sure about this patch--whether or not I did it correctly--so I split
this one out as a separate part to make it hopefully easier to review.
Attachment #8786902 - Flags: review?(wmccloskey)
Attachment #8786901 - Flags: review?(wmccloskey) → review+
Comment on attachment 8786902 [details] [diff] [review]
part 2 - remove opened actor tracking from IToplevelProtocol

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

I wish I could give this r++!

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -732,5 @@
>  
>  bool
>  CompositorBridgeParent::Bind(Endpoint<PCompositorBridgeParent>&& aEndpoint)
>  {
> -  if (!aEndpoint.Bind(this, nullptr)) {

I think removing this is incorrect. We just want to get rid of the null parameter.

::: ipc/glue/ProtocolUtils.cpp
@@ +72,5 @@
>  }
>  
>  IToplevelProtocol::~IToplevelProtocol()
>  {
>    StaticMutexAutoLock al(gProtocolMutex);

Let's get rid of this mutex too (the var as well as the acquisition).
Attachment #8786902 - Flags: review?(wmccloskey) → review+
It no longer protects any global state, and is therefore pure overhead.
Attachment #8787003 - Flags: review?(wmccloskey)
With the removed Bind() call put back.  I was removing them before I realized
what they were used for, and clearly forgot to put that one back.  Carrying
over r+.
Attachment #8787004 - Flags: review+
Attachment #8786902 - Attachment is obsolete: true
Attachment #8787003 - Flags: review?(wmccloskey) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee21ab5cb479
part 1 - remove CloneManagees/CloneToplevel code from IPDL; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e8b862bdb0
part 2 - remove opened actor tracking from IToplevelProtocol; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f3a6275d66
part 3 - remove gProtocolMutex; r=billm
You need to log in before you can comment on or make changes to this bug.