Closed Bug 1049879 Opened 5 years ago Closed 5 years ago

Remove urgent and rpc message types and replace with message priorities

Categories

(Core :: IPC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(6 files, 2 obsolete files)

I'll post details when I have a patch ready.
I finally got this working. I had to implement something a little more restricted than what I originally hoped. Here's an outline:

- There are three message priorities: normal, high, and urgent.
- Messages with high and urgent priorities must be sync.
- Messages of the same priority are dispatched in the order they were sent.
- When waiting for a reply to a message, we will only dispatch messages of equal or greater priority. For normal priority messages, we only dispatch messages of greater priority.
- Messages with high and urgent priorities can be nested.
- When two messages of the same priority are sent simultaneously by the parent and the child, the child's message is dispatched first.

The main concession I had to make is that async messages can't have high or urgent priority. To make that work, I would have had to deal with out-of-order replies to sync messages as well as some weird nesting issues. Essentially, sync messages would have become more like intr messages are now, which I didn't think would pass review.

The downside of this is that we can't just convert all IME messages to be of urgent priority since some of them are async. I'll have to work out which ones need to be ordered with respect to each other, and we may have to make some of the async messages sync to get the ordering right. Additionally, we won't be able to use this stuff to eliminate the intr CreateWindow message in PBrowser since the PBrowser constructor is async.
Attached patch ipdl-change (obsolete) — Splinter Review
This patch introduces support for the different message priorities in IPDL.
Attachment #8475613 - Flags: review?(bent.mozilla)
Attached patch signature-changeSplinter Review
This patch just changes a bunch of CallX calls to SendX and AnswerY to RecvY.
Attachment #8475614 - Flags: review?(bent.mozilla)
Attached patch messagechannel-change (obsolete) — Splinter Review
This is the big change to MessageChannel.
Attachment #8475615 - Flags: review?(bent.mozilla)
Attached patch ipdl-test-fixesSplinter Review
I also had to fix some IPDL tests. The big change was to TestRPC. It depended on the fact that, when the child and the parent send simultaneously, the parent's message is dispatched first. Now we dispatch the child's message first.

I also needed to change some more CallX and AnswerX signatures in the tests.
Attachment #8475616 - Flags: review?(bent.mozilla)
Attached patch makefile-fixSplinter Review
Simple fix to ensure that adding new tests causes the test code to be rebuilt.
Attachment #8477069 - Flags: review?(bent.mozilla)
This is a renaming that we need now that urgent messages are (sort of) gone.
Attachment #8477071 - Flags: review?(bent.mozilla)
After posting the original patches, I thought about this some more, and it really is a problem if not to have async messages of the highest priority. It makes it a lot harder to convert stuff like IME to use the new message types.

However, I realized that allowing these messages is fine as long as they don't nest. So I just made a change so that only the child can send messages of the highest priority. That seems to work fine.
Attached patch ipdl-change v2Splinter Review
New changes for IPDL to allow message priorities.
Attachment #8475613 - Attachment is obsolete: true
Attachment #8475613 - Flags: review?(bent.mozilla)
Attachment #8477076 - Flags: review?(bent.mozilla)
The corresponding MessageChannel changes. This should be ready to review now.
Attachment #8475615 - Attachment is obsolete: true
Attachment #8475615 - Flags: review?(bent.mozilla)
Attachment #8477077 - Flags: review?(bent.mozilla)
Depends on: 1021053
Attachment #8477069 - Flags: review?(bent.mozilla) → review+
Any idea when you can review this Ben? I wasn't able to get hold of dvander.
Oh, darn :(

Maybe next week? This week is crazy...
Attachment #8477071 - Flags: review?(bent.mozilla) → review+
Ben, can you let me know if I should try to pull in dvander?
(In reply to Bill McCloskey (:billm) from comment #14)
> Ben, can you let me know if I should try to pull in dvander?

Yeah, I think so.
Attachment #8475614 - Flags: review?(bent.mozilla) → review?(dvander)
Attachment #8477076 - Flags: review?(bent.mozilla) → review?(dvander)
Comment on attachment 8477076 [details] [diff] [review]
ipdl-change v2

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

I like this since it clears up the semantics. In retrospect, once we introduced the RPC type, "urgent" stopped meaning anything.
Attachment #8477076 - Flags: review?(dvander) → review+
Attachment #8475614 - Flags: review?(dvander) → review+
Attachment #8475616 - Flags: review?(bent.mozilla) → review?(dvander)
Attachment #8477077 - Flags: review?(bent.mozilla) → review?(dvander)
Thanks for taking these reviews, dvander!
Attachment #8475616 - Flags: review?(dvander) → review+
Comment on attachment 8477077 [details] [diff] [review]
messagechannel-change v2

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

::: ipc/glue/MessageChannel.cpp
@@ +23,5 @@
> + *
> + * There are three kinds of messages: async, sync, and intr. Sync and intr
> + * messages are blocking. Only intr and high-priority sync messages can nest.
> + *
> + * Sync messages are prioritized while async and intr messages always have

Should this say that async messages can be urgent priority?
Attachment #8477077 - Flags: review?(dvander) → review+
sorry had to back this out for bustages on emulator builds like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2835507&repo=mozilla-inbound
Pushed again under the theory that I just needed a clobber (it looks like ICS emulator debug worked one time and not another).
https://hg.mozilla.org/integration/mozilla-inbound/rev/40263f6c0fbc
https://hg.mozilla.org/mozilla-central/rev/40263f6c0fbc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.