1448060 - devirtualize IProtocol::GetIPCChannel()

Status: RESOLVED INCOMPLETE

(Core :: IPC, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

IProtocol::GetIPCChannel() in const and non-const variations, is virtual.  By default, this method returns the member `mChannel` in IProtocol.

This virtualness is exploited by our IPDL code generator to override GetIPCChannel() for all of our toplevel protocols such that GetIPCChannel() will return the toplevel protocol's channel.  For non-toplevel protocols, the managing protocol will call SetIPCChannel() on the managed protocol so the non-toplevel's protocol will return the appropriate thing.

This setup seems a little bit bonkers to me: IProtocol has to include the space for a MessageChannel* already, and IToplevelProtocol inherits from IProtocol.  So any IToplevelProtocol subclasses have this pointer member already...why should we not just do the following instead:

1) Make all of the codegenned toplevel protocols set IProtocol::mChannel to point at their own MessageChannel member (confusingly also called `mChannel`).
2) GetIPCChannel() can then be made non-virtual, because it will always return the right thing: for top-level protocols, it will return a pointer to their own message channel, and for non-top-level protocols, it will return a pointer to whatever got passed in to SetIPCChannel().

I guess one concern is that if somebody mistakenly calls SetIPCChannel() on a toplevel protocol (do we do that?), then the whole scheme falls apart, hard.  But I have a hard time seeing how that would do the right thing today, because there's no way to get whatever you passed in to SetIPCChannel() back out of toplevel protocols; GetIPCChannel() doesn't work, because it gets overridden.  And maybe we could bypass that whole problem by setting up `mChannel` during construction of the protocol anyway, and eliminate SetIPCChannel entirely.

We'd save a bunch of vtable entries by making these functions non-virtual, which would be nice from a size perspective.

Our IPC setup doesn't always make sense to me, but is there something I am missing here?  (Feel free to forward the ni? if you aren't sure...maybe bkelly would know more here?)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

That sounds reasonable to me. We might need to explicitly delete IProtocol's move constructor to make sure that we don't cause that pointer to become dangling due to the internal reference.

Comment 2

[Eric Rahm [:erahm]]

Nathan, do you have a guess of how much overhead this might save?

Comment 3

[Nathan Froyd [:froydnj]]

We moved GetIPCChannel into a protocol-specific State data structure in bug 1451363, so instead of several hundred vtable entries, we now have two.  We could make these non-virtual, I guess, but at this point, it's probably not worth it.