Closed Bug 636063 Opened 9 years ago Closed 7 years ago

Add a |compress| qualifier for async messages

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(3 files)

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.
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.
Summary: Implement a ProcessPendingMessagesOfType() API → Add a |compress| qualifier for async messages
Doug, we can try using this mechanism to remove the artificial framemetrics-update throttling.
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
Attachment #654161 - Flags: review?(bent.mozilla) → review+
Attachment #654162 - Flags: review?(bent.mozilla) → review+
Attachment #654163 - Flags: review?(bent.mozilla) → review+
(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.
You need to log in before you can comment on or make changes to this bug.