1049879 - Remove urgent and rpc message types and replace with message priorities

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll post details when I have a patch ready.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: ipdl-change

This patch introduces support for the different message priorities in IPDL.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: signature-change

This patch just changes a bunch of CallX calls to SendX and AnswerY to RecvY.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: messagechannel-change

This is the big change to MessageChannel.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: ipdl-test-fixes

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: makefile-fix

Simple fix to ensure that adding new tests causes the test code to be rebuilt.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rename-parent-is-blocked

This is a renaming that we need now that urgent messages are (sort of) gone.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: ipdl-change v2

New changes for IPDL to allow message priorities.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: messagechannel-change v2

The corresponding MessageChannel changes. This should be ready to review now.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Any idea when you can review this Ben? I wasn't able to get hold of dvander.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, darn :(

Maybe next week? This week is crazy...

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Working on this today.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Ben, can you let me know if I should try to pull in dvander?

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(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.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks for taking these reviews, dvander!

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

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?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e56bf4ea89fb

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40263f6c0fbc