Closed
Bug 636063
Opened 13 years ago
Closed 12 years ago
Add a |compress| qualifier for async messages
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(3 files)
7.20 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
6.45 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
13.04 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #654161 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #654162 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #654163 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 5•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #654162 -
Flags: review?(bent.mozilla) → review+
Updated•12 years ago
|
Attachment #654163 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/941fff75a9e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee8dd0d4bd7
Assignee | ||
Comment 10•12 years ago
|
||
This was all squeaky-clean green tryserver. https://tbpl.mozilla.org/?tree=Try&rev=6ae17411083c Yay!
Comment 11•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•