1259428 - stop generating useless Message subclasses in the IPDL compiler

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nathan Froyd [:froydnj]]

IPC::Message has a lot of subclasses:

https://dxr.mozilla.org/mozilla-central/search?q=derived%3AIPC%3A%3AMessage&redirect=false&case=false

Most of the subclasses are auto-generated from ipdl:

http://mxr.mozilla.org/mozilla-central/source/ipc/ipdl/ipdl/lower.py#1900

And all of them require a vtable:

froydnj@thor:/opt/build/froydnj/build-android$ readelf -sW toolkit/library/libxul.so |c++filt|egrep -c 'vtable.*::Msg_'
1428
froydnj@thor:/opt/build/froydnj/build-android$ readelf -sW toolkit/library/libxul.so |c++filt|egrep -c 'vtable.*::Reply_'
364

So roughly ~1800 Message subclasses, with 16 bytes per vtable, which comes out to ~28KB of vtables (on 32-bit, so double that on 64-bit).

Here's the thing, though: all those vtables are solely due to Message having a virtual destructor, as proper object-oriented design would have it.  Except that *none* of the subclasses have any interesting members to destruct; the only subclass that even has additional members is SyncMessage:

https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/ipc_sync_message.h#23

Both of the additional members are POD and therefore require no special treatment from the subclass's destructor.

So these vtables are pure bloat, size-wise, and add extra instructions at runtime, etc. etc.  We also pay some non-zero cost for relocations of the function pointers in the vtables, both in binary size and startup time.  I think we should fix this.

We could simply make Message's destructor non-virtual.  But that means that if somebody did add a Message subclass with non-POD classes that required destruction, we would be hosed.  The only solution I have to this is a static analysis attribute on Message that enforces POD-ness on all subclasses' members as well as no user-defined destructors for the subclasses...maybe with a few other tricks as well.

Bill, does this sound completely off-base?

Comment 2

[Nathan Froyd [:froydnj]]

A simpler alternative would be to stop generating all the subclasses in IPDL and just have functions to create |Message*| for us.  We'd lose the benefit of the Log() method, but seeing as how that's applied to the concrete class anyway, we could do separate functions for that too.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 1 - don't call Log methods of generated method classes

The first step to eliminating all the generated Message subclasses the
IPDL compiler spits out is to move the functionality of their Log
methods someplace else.  In addition to eliminating the need for the Log
methods, this change has the welcome effect of moving a bunch of code
that would be generated hundreds of times into a single place, which
should reduce code size a bit (debug builds only).  We don't actually
remove the generation of the Log methods; that change will be done for a
future patch.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 2 - remove dodgy static_cast downcasts from logging statements

Various bits of IPDL code would do something like:

  Message* m = ...;
  if (m.type() == particular message type) {
    static_cast<ParticularMessageType*>(m)->name();
  }

The static_cast is a remnant of having to do the downcast to access the
Log() method on the concrete subclass.  Since name() is defined on
Message, there's no need for these casts anymore, so let's remove them.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 3 - remove Log() methods from generated message subclasses

These are no longer called by anything.  The generated Message
subclasses now have no behavior of their own, and can be removed in
subsequent patches.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 4 - remove prtime.h from generated protocol headers

This include was only needed for PR_Now(), which is no longer called by
the headers.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 5 - convert Message subclasses to constructor functions

All we use our Message subclasses for nowadays is the constructor, so we
might as well strip the class away and just have functions that perform
the construction for us.  This change eliminates unnecessary vtables as
well as making the included headers somewhat smaller, which is always
nice.

In addition to removing all the vtables for these classes, this patch also
improves code size on x86-64 Linux by ~140K (!).  I think this is partly
because we no longer have to emit code for the destructors of the Message
subclasses, but the major reason is that the construction of Messages has been
moved out of line and is therefore no longer duplicated in several places,
improves register pressure, etc. etc.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 6 - remove unneeded MessageDecl methods

msgCast and replyCast were only used for the dodgy casts we removed in
part 2; the msgCxxType was only called by msgCast.

Comment 9

[Nathan Froyd [:froydnj]]

Let's change the summary, since our strategy for fixing the problem doesn't address the original title.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 8735486 [details] [diff] [review]
part 5 - convert Message subclasses to constructor functions

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

::: ipc/ipdl/ipdl/lower.py
@@ +1664,5 @@
>          for md in p.messageDecls:
> +            decls = []
> +
> +            mfDecl, mfDefn = _splitFuncDeclDefn(
> +                _generateMessageConstructor(md.msgClass(), md.msgId(),

Nit: this leaves a bunch of things with “Class” in their names, but they're now used for defining functions instead of classes.  Might be worth renaming them as a followup.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/234de0ccd39b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1349ec62f241
https://hg.mozilla.org/integration/mozilla-inbound/rev/9878927b10f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/850eb4d63ebe
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6dee617908
https://hg.mozilla.org/integration/mozilla-inbound/rev/5edbf0d674ed

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/234de0ccd39b
https://hg.mozilla.org/mozilla-central/rev/1349ec62f241
https://hg.mozilla.org/mozilla-central/rev/9878927b10f4
https://hg.mozilla.org/mozilla-central/rev/850eb4d63ebe
https://hg.mozilla.org/mozilla-central/rev/cb6dee617908
https://hg.mozilla.org/mozilla-central/rev/5edbf0d674ed

Comment 13

[Nicholas Nethercote [inactive]]

To summarize: on Linux64 this saves 140 KiB of code and 56 KiB of data -- is that right?

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> To summarize: on Linux64 this saves 140 KiB of code and 56 KiB of data -- is
> that right?

That's correct.