1323532 - Stop codegenning State and maybe Transition

Status: RESOLVED FIXED

(Core :: IPC, defect, P3)

Description

[Andrew McCreight [:mccr8]]

We codegen an enum State for every protocol, but there are only two different possible implementations, depending on whether hasReentrantDelete() is true or not. We could thus move the definition to ProtocolUtils.h and avoid doing that.

Similarly, a Transition function is codegenned for every protocol. Information about what message is being called is passed in, which is protocol specific, but we really only care about whether it is the delete or not, so this could be changed to a boolean, and instead compute if the method is delete at the call site. The message is fixed at each call site, so there should be no overhead, even though we're now unconditionally computing this. (Well, plus the other cases are ones where we just crash so optimizing them is irrelevant...) Anyways, with that change, and State shared between protocols, Transition() could be implemented twice in ProtocolUtils instead of in every protocol.

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1323532 - Part 1 - don't codegen State enums in ipdl, just statically define the two types we need;

Review commit: https://reviewboard.mozilla.org/r/240984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240984/

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1323532 - Part 2 - don't codegen Transition functions in ipdl, just statically define the two variants we need;

Review commit: https://reviewboard.mozilla.org/r/240986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240986/

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce3b1317264c30e83a552868c3e92b4db37b643d&group_state=expanded

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8972363 [details]
Bug 1323532 - Part 1 - don't codegen State enums in ipdl, just statically define the two types we need;

https://reviewboard.mozilla.org/r/240984/#review246758

The patches for this bug are so great.  Thank you!

::: ipc/ipdl/ipdl/lower.py:3048
(Diff revision 1)
> +        self.hdrfile.addthings([
> +            CppDirective('include', '"mozilla/ipc/ProtocolUtils.h"'),
> +            Whitespace.NL ])

I am surprised this is not already included somewhere.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8972364 [details]
Bug 1323532 - Part 2 - don't codegen Transition functions in ipdl, just statically define the two variants we need;

https://reviewboard.mozilla.org/r/240986/#review246760

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c6d8b9e3b3b
Part 1 - don't codegen State enums in ipdl, just statically define the two types we need; r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e8097237f4be
Part 2 - don't codegen Transition functions in ipdl, just statically define the two variants we need; r=froydnj

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4c6d8b9e3b3b
https://hg.mozilla.org/mozilla-central/rev/e8097237f4be