Closed Bug 1323532 Opened 3 years ago Closed 2 years ago

Stop codegenning State and maybe Transition

Categories

(Core :: IPC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mccr8, Assigned: Alex_Gaynor)

References

Details

Attachments

(2 files)

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.
Priority: -- → P3
Blocks: 1457536
Assignee: nobody → agaynor
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.
Attachment #8972363 - Flags: review?(nfroyd) → review+
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
Attachment #8972364 - Flags: review?(nfroyd) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c6d8b9e3b3b
https://hg.mozilla.org/mozilla-central/rev/e8097237f4be
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1459270
You need to log in before you can comment on or make changes to this bug.