636063 - Add a |compress| qualifier for async messages

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We've discussed this kind of API for over a year and a half, but haven't had a use case until bug 624900.  This API would be in the same spirit as win32's PeekMessage().

The "public" API would be
  PFoo::ProcessPendingMessagesOfType(msgid_t message);
  PFoo::ProcessPendingMessagesOfType(const set<msgid_t>& messageSet);

This would be implemented with a "private" API like
  AsyncChannel::ProcessPendingMessagesOfTypeFor(const set<msgid_t>& messageSet,
                                                int32 routeId);

The work would go something like
  part 1: Switch AsyncChannel and SyncChannel over to using the mPending queue for all incoming messages (i.e., move mPending into AsyncChannel)
  part 2: Implement ProcessPendingMessagesOfTypeFor
  part 3: Implement PFoo::ProcessPendingMessagesOfType

Part 2 would be the hard work.  I think the implementation should be, lock mPending, find all (async) messages matching the spec, mark them as "processed" but don't remove them from the queue, then loop over the temporary list of matched messages and dispatch them.  Allowing peeking of sync and RPC messages seems pretty scary, I'd rather not do that initially.

Clients would do something like
  msgid_t toPeek[] = { PFoo::Msg_Bar, PFoo::Msg_Baz };
  this->ProcessPendingMessagesOfType(
    set<msgid_t>(toPeek, toPeek + ALEN(toPeek)));

Bad things introduced by this API
 - allows clients to selectively break in-order delivery, though preserving it within the set of "peeked" messages for the requesting actor
 - potential for relatively scary re-entry
 - more bad interactions with RPCs, like most things

This is too risky for 4.0; invasive work in relatively sensitive code.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With a year and a half of hindsight, I'm going to implement something significantly simpler, more efficient, and less risky.  Authors will write

 async Foo() compress;

We'll add another bit to IPC::Message that specifies whether the message should be compressed or not.  (Maybe also another bit to mark whether the message should be skipped, but not sure yet.)

The semantics of this will be, when we go to enqueue an incoming message X of type Foo for actor A, if the back of the message queue already has Message Y {Foo, A}, then we'll replace Y with X.

Unlike ProcessPendingMessagesOfType(), this will lose data.  It's also strictly less powerful.  But it also means no invasive changes to ipc/glue, and no scary reentrancy concerns.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Frontend support for |compress|d messages

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Backend support for |compress|d messages

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Honor compression requests when queuing messages

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Doug, we can try using this mechanism to remove the artificial framemetrics-update throttling.

Comment 6

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

Comment on attachment 654161 [details] [diff] [review]
part 1: Frontend support for |compress|d messages

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

::: ipc/ipdl/ipdl/type.py
@@ +1459,5 @@
> +        if (mtype.compress and
> +            (not mtype.isAsync() or mtype.isCtor() or mtype.isDtor())):
> +            self.error(
> +                loc,
> +                "message `%s' in protocol `%s' requests compression but is not async or is special (ctor or dtor)", 

Nit: trailing whitespace

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to ben turner [:bent] from comment #6)
> Comment on attachment 654161 [details] [diff] [review]
> part 1: Frontend support for |compress|d messages
> 
> Review of attachment 654161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/type.py
> @@ +1459,5 @@
> > +        if (mtype.compress and
> > +            (not mtype.isAsync() or mtype.isCtor() or mtype.isDtor())):
> > +            self.error(
> > +                loc,
> > +                "message `%s' in protocol `%s' requests compression but is not async or is special (ctor or dtor)", 
> 
> Nit: trailing whitespace

Fixed.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d47636be8fac
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3e4559bfa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a719a38c25c8

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/941fff75a9e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee8dd0d4bd7

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This was all squeaky-clean green tryserver.

https://tbpl.mozilla.org/?tree=Try&rev=6ae17411083c

Yay!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Agreed, with a post m-c merge run as well (see the discussion in bug 774988 comment 6 and on).
https://tbpl.mozilla.org/?tree=Try&rev=eabc0f1e0f2e

Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/10547af1914b
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3d78f6623c
https://hg.mozilla.org/integration/mozilla-inbound/rev/34081ff6b8ac

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10547af1914b
https://hg.mozilla.org/mozilla-central/rev/9a3d78f6623c
https://hg.mozilla.org/mozilla-central/rev/34081ff6b8ac