Closed
Bug 1299594
Opened 5 years ago
Closed 5 years ago
remove dead CloneManagees and related code
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(3 files, 1 obsolete file)
23.79 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
1.00 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
15.08 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
As suggested in bug 1297804 comment 9, all this stuff is dead code, so we should get rid of it.
![]() |
Assignee | |
Comment 1•5 years ago
|
||
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)
![]() |
Assignee | |
Comment 2•5 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•5 years ago
|
||
It no longer protects any global state, and is therefore pure overhead.
Attachment #8787003 -
Flags: review?(wmccloskey)
![]() |
Assignee | |
Comment 5•5 years ago
|
||
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+
![]() |
Assignee | |
Updated•5 years ago
|
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
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee21ab5cb479 https://hg.mozilla.org/mozilla-central/rev/67e8b862bdb0 https://hg.mozilla.org/mozilla-central/rev/47f3a6275d66
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•