1451363 - separate out some IProtocol/IToplevelProtocol virtual methods into a separate State object

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

There are a bunch of virtual methods on IProtocol that are relevant only for the managed/toplevel distinction: all the methods relating to manging protocols (Register/Lookup/etc.) and all the methods relating to shared memory (CreateSharedMemory et al).  We have default implementations in IProtocol that forward to Manager() and default implementations in IToplevelProtocol that consult various hash tables...and then no subclasses override them.  Such methods consume a bunch of space in *every* protocol's vtable (including the vtables for the abstract PFoo(ParentChild) classes, which stick around because of reasons--we could get rid of these, I think, but that is a separate bug).

In a similar fashion, there are a number of virtual methods in IToplevelProtocol that exist mainly to be overriden in PluginModule(Parent|Child): {Entered,Exited}{CxxStack,Call}, for instance.  But every toplevel protocol pays the cost of having them in its vtable.  There are fewer toplevel protocols, of course, but this scheme is still unnecessary waste that could be trimmed.

One way to deal with this is to have a separate object implementing the necessary methods (and any necessary state), and then have IProtocol hold a pointer to a (heap-allocated) object:

class IProtocol
{
protected:
  class ProtocolState {
    virtual int32_t Register(IProtocol*) = 0;
    virtual int32_t RegisterID(IProtocol*, int32_t) = 0;
    ... and so on
  };

  class ManagedProtocolState : public ProtocolState { ... };
  class ToplevelProtocolState: public ProtocolState { ... };

public:
  // Functions that are virtual today would forward to mState instead.
  int32_t Register(IProtocol* p) {
    return mState->Register(p);
  }
  int32_t RegisterID(IProtocol* p, int32_t id) {
    return mState->RegisterID(p, id);
  }

private:
  ProtocolState* const mState;
};

Managed protocols would initialize mState with a ManagedProtocolState, and toplevel protocols would use ToplevelProtocolState.  Then we'd only have these methods in the vtables for those two object types, rather than having these methods in the vtable for every protocol.

This scheme would be somewhat slower for calls to the relevant functions, as we'd need three (dependent!) loads (fetch mState, fetch vtable, fetch method pointer) for every virtual function call.  You could deal with this by hand-rolling vtables, so IProtocol would effectively contain a pointer to a ProtocolState vtable directly, or by cleverly tail-allocating the relevant ProtocolState.  I don't know how much of a slowdown the straightforward C++ code would be in practice, though, so the additional complexity might not be worth it.

The same scheme could be used for the IToplevelProtocol methods.

Back of the envelope calculations for the methods in IProtocol: we have ~300 parent/child protocols in ipc/ipdl, plus concrete implementations of each, so 600 vtables.  Each method (8 bytes of vtable pointer) is worth ~5k of space savings (!).  That seems worth some implementation effort.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move Shmem-related IProtocol interfaces into an intermediate State class

IProtocol, which is inherited by every generated IPDL protocol and every
concrete protocol implementation in-tree, has a number of virtual
methods that are only relevant when distinguishing between top-level
protocols (IToplevelProtocol) and managed protocols (everything else).
These virtual methods require pointers in every protocol's vtable, which
is wasteful, and it's also somewhat confusing that many methods exist
but don't really need to be overridable in any useful way.

Let's clean this up, by creating a ProtocolState class to hold methods
that solely differ between top-level protocols and everything else.
This commit does that work and moves Shmem-related methods into this
class as a proof that this can be done in a reasonable way.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2a - move protocol register/lookup functions into ProtocolState

This functionality is base functionality for top-level and non-toplevel
protocols; nobody overrides this stuff, so it's safe to move into
ProtocolState.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2b - move protocol event target access into ProtocolState

The reasoning here is the same as for the protocol register/lookup
functions: these functions are all basic functionality that should not
be overriden by subclasses.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 3 - make OnChannelReceivedMessage defined in nightlies only

This function is only overriden in ContentChild in nightly builds, and
it's only overriden for the purpose of annotating hangs.  We shouldn't
be paying for its vtable entry in non-nightly builds.

I think have one or two more patches coming in this vein.

Comment 5

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8968302 [details] [diff] [review]
part 1 - move Shmem-related IProtocol interfaces into an intermediate State class

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

::: ipc/glue/ProtocolUtils.cpp
@@ +435,1 @@
>                                SharedMemory::SharedMemoryType aType,

nit: please fix the indentation.

::: ipc/glue/ProtocolUtils.h
@@ +142,5 @@
>          NormalShutdown,
>          AbnormalShutdown
>      };
>  
> +    class ProtocolState

Throw a comment in here describing what this is for? Just something along the lines of " virtual methods that are only relevant when distinguishing between top-level protocols (IToplevelProtocol) and managed protocols (everything else)". Also, a brief justification for why you are making things more complex with this extra layer of indirection, so somebody later doesn't think they've spotted an easy clean up to make.

The name "state" doesn't seem descriptive to me as somebody who isn't familiar with this protocol code, but I have no better suggestion. :)

@@ +153,5 @@
> +        virtual bool DestroySharedMemory(Shmem&) = 0;
> +        virtual ~ProtocolState() = default;
> +    };
> +
> +    class ManagedProtocolState final : public ProtocolState

Please add a little comment like "forward methods to our top-level protocol". Then people reading this code wouldn't have to dig around in the cpp file to figure that out.

@@ +184,5 @@
>      virtual IProtocol* Lookup(int32_t);
>      virtual void Unregister(int32_t);
>      virtual void RemoveManagee(int32_t, IProtocol*) = 0;
>  
> +    Shmem::SharedMemory* CreateSharedMemory(

You missed your opportunity to make horribly incomprehensible forwarding macros. ;)

@@ +328,5 @@
>          eReady,
>          eError
>      };
>  
> +    class State final : public ProtocolState

The naming here feels inconsistent. Why is this just "State" but the other one is "ManagedProtocolState"? It kind of feels like either they should both be State or this one should be ToplevelProtocolState or something. Feel free to ignore this.

@@ +398,5 @@
>      virtual int32_t RegisterID(IProtocol*, int32_t) override;
>      virtual IProtocol* Lookup(int32_t) override;
>      virtual void Unregister(int32_t) override;
>  
> +    void DeallocShmems() { static_cast<State*>(mState.get())->DeallocShmems(); }

Please encapsulate this "static_cast<State*>(mState.get())" as a private getter.

Comment 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8968303 [details] [diff] [review]
part 2a - move protocol register/lookup functions into ProtocolState

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

::: ipc/glue/ProtocolUtils.h
@@ +150,5 @@
>              size_t, SharedMemory::SharedMemoryType, bool, int32_t*) = 0;
>          virtual Shmem::SharedMemory* LookupSharedMemory(int32_t) = 0;
>          virtual bool IsTrackingSharedMemory(Shmem::SharedMemory*) = 0;
>          virtual bool DestroySharedMemory(Shmem&) = 0;
>          virtual ~ProtocolState() = default;

Maybe put the dtor into a separate section, maybe first? Also it might be nice if you labelled the different subsections.

@@ +152,5 @@
>          virtual bool IsTrackingSharedMemory(Shmem::SharedMemory*) = 0;
>          virtual bool DestroySharedMemory(Shmem&) = 0;
>          virtual ~ProtocolState() = default;
> +
> +        virtual int32_t Register(IProtocol*);

Am I right in thinking that these should be pure virtual because you never define them?

@@ +174,5 @@
>  
> +        int32_t Register(IProtocol*) override;
> +        int32_t RegisterID(IProtocol*, int32_t) override;
> +        IProtocol* Lookup(int32_t) override;
> +        void Unregister(int32_t) override;

nit: blank line before the "private".

@@ +369,5 @@
>  
> +        int32_t Register(IProtocol*) override;
> +        int32_t RegisterID(IProtocol*, int32_t) override;
> +        IProtocol* Lookup(int32_t) override;
> +        void Unregister(int32_t) override;

nit: blank line before "private".

Comment 7

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8968304 [details] [diff] [review]
part 2b - move protocol event target access into ProtocolState

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

::: ipc/glue/ProtocolUtils.h
@@ +188,5 @@
> +
> +        nsIEventTarget* GetActorEventTarget() override;
> +        void SetEventTargetForActor(IProtocol* aActor, nsIEventTarget* aEventTarget) override;
> +        void ReplaceEventTargetForActor(IProtocol* aActor, nsIEventTarget* aEventTarget) override;
> +        already_AddRefed<nsIEventTarget> GetActorEventTarget(IProtocol* aActor) override;

nit: blank line before "private".

@@ +525,5 @@
>  
> +    already_AddRefed<nsIEventTarget>
> +    GetMessageEventTarget(const Message& aMsg)
> +    {
> +        return static_cast<State*>(mState.get())->GetMessageEventTarget(aMsg);

Use the getter I asked for in part 1 here, please.

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8968305 [details] [diff] [review]
part 3 - make OnChannelReceivedMessage defined in nightlies only

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

::: ipc/glue/ProtocolUtils.h
@@ +517,5 @@
>      {
>          return false;
>      }
>  
> +#ifdef NIGHTLY_BUILD

Nice catch.

Don't you also need to add this guard to the call site for this method in MessageChannel::OnMessageReceivedFromLink()?

I also see an override in BackgroundChildImpl that is defined #ifdef EARLY_BETA_OR_EARLIER, so I think this will break the build for early beta. (and maybe developer edition or something...)

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 3 - make OnChannelReceivedMessage defined in early beta only

OK, let's only define it in early beta, then.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 4 - consolidate generated code into IProtocol

lower.py generates repetitious:

  SetManager(...);
  Register(...); // Or RegisterID.
  SetIPCChannel(...);

calls, which are moderately sized, given that the above call sequence
requires virtual calls in several places.  Instead of codegenning this
sequence, let's consolidate the sequence into IProtocol and change the
code generator to call into the consolidated function instead.

(This is just a small codesize win, but it is at least related to virtual
methods, so...)

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 5 - remove ProtocolName virtual function

ProtocolName() is only used for producing error messages and annotating
crash reports.  But examining actual crash reports that would have used
the result of ProtocolName() indicates that we can always tell what the
erroring protocol is due to the stack backtrace.  So having this virtual
function around just provides duplicate information, and it takes up too
much space in the vtable besides.  Let's get rid of it.

I looked through the crashes at:

https://crash-stats.mozilla.com/search/?signature=~FatalError&date=%3E%3D2018-04-13T16%3A42%3A26.000Z&date=%3C2018-04-20T16%3A42%3A26.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

to validate my intuition about the crashing protocol being in the backtrace.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 6 - move GetIPCChannel into ProtocolState

We can move this information into ProtocolState and save having two
virtual functions for every protocol.  Moving some bits out of the
codegen'd IPC code is a nice bonus, though we keep the strange setup
where toplevel protocols have two mChannel member variables.

Comment 13

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8969806 [details] [diff] [review]
part 4 - consolidate generated code into IProtocol

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

::: ipc/glue/ProtocolUtils.cpp
@@ +545,5 @@
> +  SetIPCChannel(aManager->GetIPCChannel());
> +}
> +
> +void
> +IProtocol::SetManagerAndRegister(IProtocol* aManager, int32_t aId)

Should this be called "SetManagerAndRegisterID"? Oh, I see, you overload to simplify the code generation. Fair enough.

::: ipc/glue/ProtocolUtils.h
@@ +319,5 @@
>      void SetManager(IProtocol* aManager);
> +
> +    // Sets the manager for the protocol and registers the protocol with
> +    // its manager, setting up channels for the protocol as well.  Not
> +    // for use outside of IPDL.

Could you please add a sentence saying why these tiny wrappers are useful? Maybe if the win is small enough, it doesn't matter if it gets reverted so you don't need an explanation.

@@ +326,1 @@
>      void SetIPCChannel(MessageChannel* aChannel) { mChannel = aChannel; }

Add a blank line before SetIPCChannel? Assuming that the comment does not apply to it.

::: ipc/ipdl/ipdl/lower.py
@@ -3925,4 @@
>  
>          return [
>              self.failIfNullActor(actorvar, errfn, msg="Error constructing actor %s" % actortype.name() + self.side.capitalize()),
> -            # set manager in prior to register to inherit EventTarget from manager.

Should this comment be added to SetManagerAndRegister()? (I confess that I have no idea what that comment means.)

Comment 14

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8969807 [details] [diff] [review]
part 5 - remove ProtocolName virtual function

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

I guess if this is a problem we can back it out.

::: ipc/ipdl/ipdl/lower.py
@@ +3910,5 @@
>              setManagerArgs = [ExprVar.THIS]
>          else:
>              setManagerArgs = [ExprVar.THIS, idexpr]
>          setmanager = ExprCall(ExprSelect(actorvar, '->', 'SetManagerAndRegister'),
> +                              args=setManagerArgs)

I guess this is a fix for the previous patch?

::: ipc/ipdl/test/cxx/TestActorPunning.cpp
@@ -33,2 @@
>  {
> -  if (!!strcmp(aProtocolName, "PTestActorPunningParent")) {

I'd complain about losing a bit of test coverage here if I thought anybody ran these tests regularly.

Comment 15

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8969816 [details] [diff] [review]
part 6 - move GetIPCChannel into ProtocolState

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

::: ipc/glue/ProtocolUtils.h
@@ +187,5 @@
> +
> +        virtual const MessageChannel* GetIPCChannel() const = 0;
> +        virtual MessageChannel* GetIPCChannel() = 0;
> +
> +        // XXX

What's the deal with this XXX comment? Did you mean to delete it or add some more text?

@@ +200,5 @@
>      class ManagedState final : public ProtocolState
>      {
>      public:
>          ManagedState(IProtocol* aProtocol)
> +            : ProtocolState()

Is this explicit call to the parent ctor really needed given that there's only one ctor? (Likewise for the other ctor.)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5ce8fcfdff
part 1 - move Shmem-related IProtocol interfaces into an intermediate State class; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/78353bf75968
part 2a - move protocol register/lookup functions into ProtocolState; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3978808c33d9
part 2b - move protocol event target access into ProtocolState; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea228cf8cee5
part 3 - make OnChannelReceivedMessage defined in early beta only; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccddb56135e6
part 4 - consolidate generated code into IProtocol; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/52f24d629711
part 5 - remove ProtocolName virtual function; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3f19666037
part 6 - move GetIPCChannel into ProtocolState; r=mccr8

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5b5ce8fcfdff
https://hg.mozilla.org/mozilla-central/rev/78353bf75968
https://hg.mozilla.org/mozilla-central/rev/3978808c33d9
https://hg.mozilla.org/mozilla-central/rev/ea228cf8cee5
https://hg.mozilla.org/mozilla-central/rev/ccddb56135e6
https://hg.mozilla.org/mozilla-central/rev/52f24d629711
https://hg.mozilla.org/mozilla-central/rev/ce3f19666037

Comment 18

[Eric Rahm [:erahm]]

It looks like this may have regressed libxul.so size by 444k.

https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=mozilla-inbound,1299711,1,2&zoom=1524505968353.196,1524507487210.4395,128938666.45707847,129422836.68300332&selected=mozilla-inbound,1299711,329791,456168293,2