1297801 - small size improvements to IPDL generated code

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Nathan Froyd [:froydnj]]

I think these improvements save ~100K on x86-64 Linux.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - commonize the managee->array functions in IPDL generated code

There's no reason to generate identical code for every kind of managed
protocol; we can have a single instance that operates on void* and cast
our way to victory.  This change saves ~60K of text on x86-64 Linux.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - make IPDL Transition functions take an inout parameter for State

The calls to IPDL Transition() functions consistently look like:

  Transition(VAR, ..., &VAR);

We can save space by only passing &VAR and deriving the state we're
coming from by loading VAR in Transition itself.  It's not great using
inout parameters here, but we call Transition enough times that this
change saves a reasonable amount of space: about 10K on x86-64 Linux.
The unsightliness of inout parameters here is lessened by the only
callers of Transition being generated code.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - make mozilla::ipc::Trigger 4 bytes instead of 8

We construct Trigger structures for every IPDL send and recv to run
the (currently defunct) IPDL state machine for each protocol.  These
structures are 64 bits to hold an enum that could be represented in 1
bit and an IPDL message enum that can easily be represented in 31 bits.
Therefore, we can make the Trigger structure smaller by storing the
members as bitfields, rather than full-width integers.  (The message
enums are formed from a 16-bit protocol enum and a 16-bit
protocol-specific message enum; since the protocol enum goes in the
upper bits, we'd need 32768 protocols to overflow the bitfield we're
using in Trigger...a number that we're not going to hit anytime soon.)

This change saves ~15K of space on x86-64 Linux.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - use non-null-checked operator new in IPDL code

The standard placement new function requires a null check, implicitly
generated by the compiler, on its returned value.  For places we know
don't deal with null pointers, such as all the generated methods of IPDL
code, we can use an overloaded operator new that doesn't require the
null check.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8784513 [details] [diff] [review]
part 1 - commonize the managee->array functions in IPDL generated code

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

Thanks. I've also been considering how much type safety we need in IPDL. We already have to cast pretty much every actor we get out of IPDL (from PFoo* to Foo*). Would it be so bad to instead cast from IProtocol* to Foo*? You lose the ability to guarantee that we're not going from PHttpChannelChild* to TabParent*, but I wonder if a little bit of dynamic checking could solve that? Perhaps every protocol could have a virtual method returning its type (as an ID or something) and then we would add a cast operator on PFoo like this:
  template<class T>
  T* PFooChild::From(IProtocol* proto) {
    MOZ_RELEASE_ASSERT(proto->GetType() == kFooChild);
    return static_cast<T*>(static_cast<PFooChild*>(proto));
  }

Anyway, just a thought. It depends on how much more stuff like this there is.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #5)
> Comment on attachment 8784513 [details] [diff] [review]
> part 1 - commonize the managee->array functions in IPDL generated code
> 
> Review of attachment 8784513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. I've also been considering how much type safety we need in IPDL. We
> already have to cast pretty much every actor we get out of IPDL (from PFoo*
> to Foo*). Would it be so bad to instead cast from IProtocol* to Foo*? You
> lose the ability to guarantee that we're not going from PHttpChannelChild*
> to TabParent*, but I wonder if a little bit of dynamic checking could solve
> that? Perhaps every protocol could have a virtual method returning its type
> (as an ID or something) and then we would add a cast operator on PFoo like
> this:
>   template<class T>
>   T* PFooChild::From(IProtocol* proto) {
>     MOZ_RELEASE_ASSERT(proto->GetType() == kFooChild);
>     return static_cast<T*>(static_cast<PFooChild*>(proto));
>   }
> 
> Anyway, just a thought. It depends on how much more stuff like this there is.

Yeah, something like this could be useful.  I think some of the code that we generate, like CloneManagees, DestroySubtree, etc. are artifacts of how the class hierarchy is laid out, rather than actually useful code, and we could do better in many cases, writing more of the code in ProtocolUtils.{h,cpp} or similar, rather than having to write it all in ipdl.py.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7513fc352c1a
part 1 - commonize the managee->array functions in IPDL generated code; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ebc0f53fdb9
part 2 - make IPDL Transition functions take an inout parameter for State; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f619a7e052
part 3 - make mozilla::ipc::Trigger 4 bytes instead of 8; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6323b1ad052
part 4 - use non-null-checked operator new in IPDL code; r=billm

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7513fc352c1a
https://hg.mozilla.org/mozilla-central/rev/9ebc0f53fdb9
https://hg.mozilla.org/mozilla-central/rev/17f619a7e052
https://hg.mozilla.org/mozilla-central/rev/b6323b1ad052