Closed Bug 1259428 Opened 4 years ago Closed 4 years ago

stop generating useless Message subclasses in the IPDL compiler

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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?
Duplicate of this bug: 1259429
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.
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.
Attachment #8735482 - Flags: review?(jld)
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.
Attachment #8735483 - Flags: review?(jld)
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.
Attachment #8735484 - Flags: review?(jld)
This include was only needed for PR_Now(), which is no longer called by
the headers.
Attachment #8735485 - Flags: review?(jld)
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.
Attachment #8735486 - Flags: review?(jld)
msgCast and replyCast were only used for the dodgy casts we removed in
part 2; the msgCxxType was only called by msgCast.
Attachment #8735487 - Flags: review?(jld)
Let's change the summary, since our strategy for fixing the problem doesn't address the original title.
Assignee: nobody → nfroyd
Summary: stop using a virtual destructor in IPC::Message → stop generating useless Message subclasses in the IPDL compiler
Attachment #8735487 - Flags: review?(jld) → review+
Attachment #8735485 - Flags: review?(jld) → review+
Attachment #8735484 - Flags: review?(jld) → review+
Attachment #8735483 - Flags: review?(jld) → review+
Attachment #8735482 - Flags: review?(jld) → review+
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.
Attachment #8735486 - Flags: review?(jld) → review+
To summarize: on Linux64 this saves 140 KiB of code and 56 KiB of data -- is that right?
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
You need to log in before you can comment on or make changes to this bug.