Last Comment Bug 636063 - Add a |compress| qualifier for async messages
: Add a |compress| qualifier for async messages
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
: [PTO to Dec5] Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks: 774988
  Show dependency treegraph
 
Reported: 2011-02-22 18:09 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-08-25 19:26 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: Frontend support for |compress|d messages (7.20 KB, patch)
2012-08-22 03:54 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 2: Backend support for |compress|d messages (6.45 KB, patch)
2012-08-22 03:55 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 3: Honor compression requests when queuing messages (13.04 KB, patch)
2012-08-22 03:55 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-02-22 18:09:47 PST
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 2012-08-22 01:29:32 PDT
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 2012-08-22 03:54:46 PDT
Created attachment 654161 [details] [diff] [review]
part 1: Frontend support for |compress|d messages
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-22 03:55:04 PDT
Created attachment 654162 [details] [diff] [review]
part 2: Backend support for |compress|d messages
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-22 03:55:27 PDT
Created attachment 654163 [details] [diff] [review]
part 3: Honor compression requests when queuing messages
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-22 23:31:11 PDT
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!) 2012-08-24 13:48:15 PDT
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 2012-08-24 17:44:41 PDT
(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 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-25 12:12:26 PDT
This was all squeaky-clean green tryserver.

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

Yay!

Note You need to log in before you can comment on or make changes to this bug.