Closed
Bug 1309799
Opened 8 years ago
Closed 8 years ago
Generate ipc msgtype to name string mapping function and use in IPC
Categories
(Core :: IPC, defect)
Core
IPC
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.
| Comment hidden (mozreview-request) |
|
Reporter | |
Comment 2•8 years ago
|
||
(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.
|
|
Reporter | |
Updated•8 years ago
|
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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Reporter | |
Updated•8 years ago
|
Attachment #8802909 -
Flags: review?(wmccloskey)
|
|
Reporter | |
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
| mozreview-review | ||
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 9•8 years ago
|
||
| mozreview-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 10•8 years ago
|
||
| mozreview-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+
|
|
Reporter | |
Comment 11•8 years ago
|
||
(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.
|
|
Reporter | |
Comment 12•8 years ago
|
||
(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.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 16•8 years ago
|
||
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
|
||
Comment 17•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/7ae23ee055f5 https://hg.mozilla.org/mozilla-central/rev/e6d4412c0cc4 https://hg.mozilla.org/mozilla-central/rev/6610a1df0570
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•