Closed Bug 1309799 Opened 3 years ago Closed 3 years ago

Generate ipc msgtype to name string mapping function and use in IPC

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kanru, Unassigned)

Details

Attachments

(3 files)

Currently we sometimes get error like "IPCError-content | (msgtype=0x8A0017,name=???) Route error: message sent to unknown actor ID" error because we cannot find the actor on the received side.

Signature like this is very unstable because the msgtype changes when protocols are add/removed. We should always be able to get the name from msgtype.
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #1)
> Created attachment 8800539 [details]
> Bug 1309799 - Generate StringFromIPCMessageTypeId and use it in
> MessageChannel::MaybeHandleError.

libxul.so size increased about 40 KiB with -Os build

old size: 100061384
new size: 100102344

If we are concerned about the codesize increase maybe we could build a lookup table instead.
Attachment #8800539 - Flags: review?(wmccloskey)
Besides a lookup table, you could save space by removing all the set_name calls in generated code and replacing uses of Message::name() with your function. Then this patch would probably save space in libxul.
Attachment #8802909 - Flags: review?(wmccloskey)
I didn't use a static lookup table because the compiler generated jump table is smaller.

The third part, to change the profiler label, is optional. With it the binary size is actually reduced a bit, otherwise we still increases the binary size slightly, which I think is somewhat acceptable.
Comment on attachment 8800539 [details]
Bug 1309799 - Generate StringFromIPCMessageType and use it in MessageChannel::MaybeHandleError.

https://reviewboard.mozilla.org/r/85464/#review86316

This looks great! Thanks.

::: ipc/glue/ProtocolUtils.h:573
(Diff revision 2)
>  
>  void
>  TableToArray(const nsTHashtable<nsPtrHashKey<void>>& aTable,
>               nsTArray<void*>& aArray);
>  
> +const char* StringFromIPCMessageTypeId(uint32_t aMessageType);

My just StringFromIPCMessageType.

::: ipc/ipdl/ipdl.py:152
(Diff revision 2)
>      ipdl.gencxx(filename, ast, headersdir, cppdir)
> -    
> +
> +    messages = []
>      if ast.protocol:
> -        allprotocols.append('%sMsgStart' % ast.protocol.name)
> +        for md in ast.protocol.messageDecls:
> +            messages.append(md.prettyMsgName())

Could we instead put this code here:
http://searchfox.org/mozilla-central/rev/703b663355467293fad148ab7c2c5ee2b878e4d9/ipc/ipdl/ipdl/lower.py#1671

Otherwise I'm afraid the IDs could get out of sync with the names and we wouldn't notice.
Attachment #8800539 - Flags: review?(wmccloskey) → review+
Comment on attachment 8802909 [details]
Bug 1309799 - Make error message more stable.

https://reviewboard.mozilla.org/r/87172/#review86318
Attachment #8802909 - Flags: review?(wmccloskey) → review+
Comment on attachment 8802910 [details]
Bug 1309799 - Change IPC profiler label format to share strings.

https://reviewboard.mozilla.org/r/87174/#review86320

::: ipc/ipdl/ipdl/lower.py:5367
(Diff revision 1)
>                                               if receiving
>                                               else 'mozilla::ipc::MessageDirection::eSending') ])) ])
>  
> -    def profilerLabel(self, tag, msgname):
> -        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL'),
> -                                 [ ExprLiteral.String('IPDL::' + self.protocol.name),
> +    def profilerLabel(self, md, tag):
> +        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL_PRINTF'),
> +                                 [ ExprLiteral.String(self.protocol.name),

Does taking out the IPDL:: part save space?

::: ipc/ipdl/ipdl/lower.py:5370
(Diff revision 1)
> -    def profilerLabel(self, tag, msgname):
> -        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL'),
> -                                 [ ExprLiteral.String('IPDL::' + self.protocol.name),
> -                                   ExprLiteral.String(tag + msgname),
> -                                   ExprVar('js::ProfileEntry::Category::OTHER') ]))
> +    def profilerLabel(self, md, tag):
> +        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL_PRINTF'),
> +                                 [ ExprLiteral.String(self.protocol.name),
> +                                   ExprLiteral.String(md.prettyMsgName()),
> +                                   ExprVar('js::ProfileEntry::Category::OTHER'),
> +                                   ExprLiteral.String('(%s)'),

The tag isn't very useful. I think it would be fine to take it out. We can always look up what kind of message something is.
Attachment #8802910 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #8)
> ::: ipc/ipdl/ipdl.py:152
> (Diff revision 2)
> >      ipdl.gencxx(filename, ast, headersdir, cppdir)
> > -    
> > +
> > +    messages = []
> >      if ast.protocol:
> > -        allprotocols.append('%sMsgStart' % ast.protocol.name)
> > +        for md in ast.protocol.messageDecls:
> > +            messages.append(md.prettyMsgName())
> 
> Could we instead put this code here:
> http://searchfox.org/mozilla-central/rev/
> 703b663355467293fad148ab7c2c5ee2b878e4d9/ipc/ipdl/ipdl/lower.py#1671
> 
> Otherwise I'm afraid the IDs could get out of sync with the names and we
> wouldn't notice.

I extract this to a function and use it in both places.
(In reply to Bill McCloskey (:billm) from comment #10)
> Comment on attachment 8802910 [details]
> Bug 1309799 - Change IPC profiler label format to share strings.
> 
> https://reviewboard.mozilla.org/r/87174/#review86320
> 
> ::: ipc/ipdl/ipdl/lower.py:5367
> (Diff revision 1)
> >                                               if receiving
> >                                               else 'mozilla::ipc::MessageDirection::eSending') ])) ])
> >  
> > -    def profilerLabel(self, tag, msgname):
> > -        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL'),
> > -                                 [ ExprLiteral.String('IPDL::' + self.protocol.name),
> > +    def profilerLabel(self, md, tag):
> > +        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL_PRINTF'),
> > +                                 [ ExprLiteral.String(self.protocol.name),
> 
> Does taking out the IPDL:: part save space?

Yes, the PROFILER_LABEL now use the same literal string as the logging code.
 
> ::: ipc/ipdl/ipdl/lower.py:5370
> (Diff revision 1)
> > -    def profilerLabel(self, tag, msgname):
> > -        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL'),
> > -                                 [ ExprLiteral.String('IPDL::' + self.protocol.name),
> > -                                   ExprLiteral.String(tag + msgname),
> > -                                   ExprVar('js::ProfileEntry::Category::OTHER') ]))
> > +    def profilerLabel(self, md, tag):
> > +        return StmtExpr(ExprCall(ExprVar('PROFILER_LABEL_PRINTF'),
> > +                                 [ ExprLiteral.String(self.protocol.name),
> > +                                   ExprLiteral.String(md.prettyMsgName()),
> > +                                   ExprVar('js::ProfileEntry::Category::OTHER'),
> > +                                   ExprLiteral.String('(%s)'),
> 
> The tag isn't very useful. I think it would be fine to take it out. We can
> always look up what kind of message something is.

Done.
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ae23ee055f5
Generate StringFromIPCMessageType and use it in MessageChannel::MaybeHandleError. r=billm
https://hg.mozilla.org/integration/autoland/rev/e6d4412c0cc4
Make error message more stable. r=billm
https://hg.mozilla.org/integration/autoland/rev/6610a1df0570
Change IPC profiler label format to share strings. r=billm
You need to log in before you can comment on or make changes to this bug.