Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add a |compress| qualifier for async messages

RESOLVED FIXED in mozilla17

Status

()

Core
IPC
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

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.
Blocks: 774988
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
Created attachment 654161 [details] [diff] [review]
part 1: Frontend support for |compress|d messages
Attachment #654161 - Flags: review?(bent.mozilla)
Created attachment 654162 [details] [diff] [review]
part 2: Backend support for |compress|d messages
Attachment #654162 - Flags: review?(bent.mozilla)
Created attachment 654163 [details] [diff] [review]
part 3: Honor compression requests when queuing messages
Attachment #654163 - Flags: review?(bent.mozilla)
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.
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/941fff75a9e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee8dd0d4bd7
This was all squeaky-clean green tryserver.

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

Yay!
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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/10547af1914b
https://hg.mozilla.org/mozilla-central/rev/9a3d78f6623c
https://hg.mozilla.org/mozilla-central/rev/34081ff6b8ac
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.