|
|
Created:
5 years, 1 month ago by Anand Mistry (off Chromium) Modified:
5 years ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAPI definition for WaitSet.
BUG=556865
Committed: https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de
Cr-Commit-Position: refs/heads/master@{#363914}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update API. #
Total comments: 2
Patch Set 3 : Rebase. #Patch Set 4 : Fix comment. #
Total comments: 7
Patch Set 5 : Review comments. #
Total comments: 10
Patch Set 6 : Change GetHandles to return multiple handles, and address other comments. #Patch Set 7 : Rebase #Patch Set 8 : Rebase #Patch Set 9 : Rebase. #
Dependent Patchsets: Messages
Total messages: 48 (9 generated)
amistry@chromium.org changed reviewers: + jam@chromium.org, rockot@chromium.org, sammc@chromium.org, yzshen@chromium.org
Hi all, This change is what I would consider and API for wait sets in Mojo. It's analogous to epoll, but without all the options (and only level-triggered). Rather than debating the API in a doc, I thought I'd put together this change and kill two birds with one stone. Hopefully, it'll let me make progress a bit quicker than I've been doing so far. There are lots of questionable design choices which I'd like everyone to comment on. Specifically (but not limited to): 1. Re-use MojoHandle vs. a new handle type. In this change, I've gone for the latter. But I can certainly see the pros and cons of each. For reusing MojoHandle, on one hand, very few of the usual handle properties would apply. You can't read/write or pass the handle. But you can close it, and wait on it, leading to interesting cases of nested waits (a wait set handle being added to a wait set). It would reduce the API surface by one function (eliminating WaitSetClose()), but probably wouldn't affect implementation complexity very much. 2. Separate functions for add/remove. epoll() has a single function that does add/remove/modify. This didn't 'feel' like it fit into the current Mojo API design, so I went with separate functions. There's no modify, since I didn't think it was necessary. 3. A handle can only be added to a wait set once. This is different from WaitMany, where you can add a handle multiple times with different signals. WaitSet is a stateful API whereas WaitMany is one-shot. You need to be able to add and remove waiters, and having keying by handle+signal seemed problematic. Note, epoll also only allows an fd to be added once to the set. 4. Wait() only returns one handle, unlike WaitMany and epoll which can return many. The upside is a smaller API surface and possibly simpler implementation. The negative is increased overhead (having to call Wait() many times) when several handles are signalled. 5. What happens when you call wait at the same time from multiple threads? I've defined it, but I'm not 100% happy with it.
Oh yeah, and problem #6, naming.
darin@chromium.org changed reviewers: + darin@chromium.org
https://codereview.chromium.org/1429553002/diff/1/third_party/mojo/src/mojo/p... File third_party/mojo/src/mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/1/third_party/mojo/src/mojo/p... third_party/mojo/src/mojo/public/c/system/wait_set.h:44: MojoWaitSetHandle* wait_set_handle); // Out. Even though it is technically a handle like thing, I might avoid calling this a "*Handle" as that evokes thoughts of MojoHandle. This is not a MojoHandle, so it seems like it might be best to avoid the two sounding related.
Actually, if you do reuse MojoHandle, then you can reuse MojoClose and ScopedHandle. Hmmm...
On 2015/10/26 23:50:25, Anand Mistry wrote: > Hi all, > > This change is what I would consider and API for wait sets in Mojo. It's > analogous to epoll, but without all the options (and only level-triggered). > Rather than debating the API in a doc, I thought I'd put together this change > and kill two birds with one stone. Hopefully, it'll let me make progress a bit > quicker than I've been doing so far. > > There are lots of questionable design choices which I'd like everyone to comment > on. Specifically (but not limited to): > 1. Re-use MojoHandle vs. a new handle type. In this change, I've gone for the > latter. But I can certainly see the pros and cons of each. For reusing > MojoHandle, on one hand, very few of the usual handle properties would apply. > You can't read/write or pass the handle. But you can close it, and wait on it, > leading to interesting cases of nested waits (a wait set handle being added to a > wait set). It would reduce the API surface by one function (eliminating > WaitSetClose()), but probably wouldn't affect implementation complexity very > much. I'm a fan of having a single unified handle type. You also can't read or write shared buffers (and likewise you can't dupe message pipe endpoints), but they all use MojoHandle. > > 2. Separate functions for add/remove. epoll() has a single function that does > add/remove/modify. This didn't 'feel' like it fit into the current Mojo API > design, so I went with separate functions. There's no modify, since I didn't > think it was necessary. Seems completely reasonable to have separate add/remove functions. IMHO this API is easier to read and write than a unified function with operational flags. > > 3. A handle can only be added to a wait set once. This is different from > WaitMany, where you can add a handle multiple times with different signals. > WaitSet is a stateful API whereas WaitMany is one-shot. You need to be able to > add and remove waiters, and having keying by handle+signal seemed problematic. > Note, epoll also only allows an fd to be added once to the set. It seems acceptable to enforce this constraint, but I wonder if it's necessary. It seems non-problematic to still key on handle alone, but treat addition/removal as flag set union/subtraction. If a remove operation results in 0 signal flags, the handle is removed from the wait set. > > 4. Wait() only returns one handle, unlike WaitMany and epoll which can return > many. The upside is a smaller API surface and possibly simpler implementation. > The negative is increased overhead (having to call Wait() many times) when > several handles are signalled. I think emulating MojoWaitMany here adds a little complexity to the API but not much. Maybe we could do something in between, where you explicitly pass the max number of signals you are willing to hear about for a single call. Passing 1 as that max would let us use the API as you have it defined now. > > 5. What happens when you call wait at the same time from multiple threads? I've > defined it, but I'm not 100% happy with it. I probably haven't put nearly enough thought into this, but why would anyone legitimately want to wait on the same handle from multiple threads? I'd be happy to go with your description as-is with the addendum that nobody should ever cause this to happen. > 6. naming Seems fine to me, though the resulting name of WaitSetWait is a little unfortunate. We could throw around some other synonyms for WaitSet, e.g., WatchSet or something.
> On 2015/10/26 23:50:25, Anand Mistry wrote: > > Hi all, > > > > This change is what I would consider and API for wait sets in Mojo. It's > > analogous to epoll, but without all the options (and only level-triggered). > > Rather than debating the API in a doc, I thought I'd put together this change > > and kill two birds with one stone. Hopefully, it'll let me make progress a bit > > quicker than I've been doing so far. > > > > There are lots of questionable design choices which I'd like everyone to > comment > > on. Specifically (but not limited to): > > 1. Re-use MojoHandle vs. a new handle type. In this change, I've gone for the > > latter. But I can certainly see the pros and cons of each. For reusing > > MojoHandle, on one hand, very few of the usual handle properties would apply. > > You can't read/write or pass the handle. But you can close it, and wait on it, > > leading to interesting cases of nested waits (a wait set handle being added to > a > > wait set). It would reduce the API surface by one function (eliminating > > WaitSetClose()), but probably wouldn't affect implementation complexity very > > much. > > I'm a fan of having a single unified handle type. You also can't read or write > shared buffers (and likewise you can't dupe message pipe endpoints), but they > all use MojoHandle. I'm kinda coming around to this as well. I like Darin's point that using MojoHandle would allow us to re-use some of the C++ bindings (ScopedHandle, and maybe more). > > > > > 2. Separate functions for add/remove. epoll() has a single function that does > > add/remove/modify. This didn't 'feel' like it fit into the current Mojo API > > design, so I went with separate functions. There's no modify, since I didn't > > think it was necessary. > > Seems completely reasonable to have separate add/remove functions. IMHO this API > is easier to read and write than a unified function with operational flags. Thanks. Pretty much my thought as well. > > > > > 3. A handle can only be added to a wait set once. This is different from > > WaitMany, where you can add a handle multiple times with different signals. > > WaitSet is a stateful API whereas WaitMany is one-shot. You need to be able to > > add and remove waiters, and having keying by handle+signal seemed problematic. > > Note, epoll also only allows an fd to be added once to the set. > > It seems acceptable to enforce this constraint, but I wonder if it's necessary. > It seems non-problematic to still key on handle alone, but treat > addition/removal as flag set union/subtraction. If a remove operation results in > 0 signal flags, the handle is removed from the wait set. Hmmm. Implementation wise, this would be easy. But my fear is that this would lead to issues with semantics, and be difficult to use correctly. For instance, if you key by handle alone, which one do you remove when you call Remove()? I think the effect, as long as you have a single controller for the wait call, can be achieved by the caller instead of doing this inside the wait set primitive. > > > > > 4. Wait() only returns one handle, unlike WaitMany and epoll which can return > > many. The upside is a smaller API surface and possibly simpler implementation. > > The negative is increased overhead (having to call Wait() many times) when > > several handles are signalled. > > I think emulating MojoWaitMany here adds a little complexity to the API but not > much. Maybe we could do something in between, where you explicitly pass the max > number of signals you are willing to hear about for a single call. Passing 1 as > that max would let us use the API as you have it defined now. What you describe is exactly what epoll does. It lets you wait for up to N fds to become ready. It's not a huge deal, but I thought it's worth bringing up anyways. I'm not attached to either way. > > > > > 5. What happens when you call wait at the same time from multiple threads? > I've > > defined it, but I'm not 100% happy with it. > > I probably haven't put nearly enough thought into this, but why would anyone > legitimately want to wait on the same handle from multiple threads? I'd be happy > to go with your description as-is with the addendum that nobody should ever > cause this to happen. So, it's unlikely we'll use this in Chrome. However, if we look at epoll, there are cases where it's useful to have multiple threads call wait on the same set. In epoll, it lets you have an efficient, multi-threaded IO loop thread-pool (think EventManager in google3). On the flip side, epoll is much better at supporting this because it's edge-triggered and has a one-shot wait feature, whereas Mojo's level-triggering would cause a thundering-herd effect because multiple threads would get woken up. Regardless, this still needs to be defined because Mojo should be thread-safe. We could cop-out and define it to return BUSY if you wait from multiple threads. > > > 6. naming > > Seems fine to me, though the resulting name of WaitSetWait is a little > unfortunate. We could throw around some other synonyms for WaitSet, e.g., > WatchSet or something. I'd like to keep "wait" as part of it since the concept of waiting is a well-defined action in Mojo. But WaitSetWait is... nasty.
Can MojoWait be used to wait on a WaitSet? Could the signalled state be queried from the WaitSet after MojoWait returns? -Darin On Oct 28, 2015 11:36 PM, <amistry@chromium.org> wrote: > On 2015/10/26 23:50:25, Anand Mistry wrote: >> > Hi all, >> > >> > This change is what I would consider and API for wait sets in Mojo. It's >> > analogous to epoll, but without all the options (and only >> level-triggered). >> > Rather than debating the API in a doc, I thought I'd put together this >> > change > >> > and kill two birds with one stone. Hopefully, it'll let me make >> progress a >> > bit > >> > quicker than I've been doing so far. >> > >> > There are lots of questionable design choices which I'd like everyone to >> comment >> > on. Specifically (but not limited to): >> > 1. Re-use MojoHandle vs. a new handle type. In this change, I've gone >> for >> > the > >> > latter. But I can certainly see the pros and cons of each. For reusing >> > MojoHandle, on one hand, very few of the usual handle properties would >> > apply. > >> > You can't read/write or pass the handle. But you can close it, and wait >> on >> > it, > >> > leading to interesting cases of nested waits (a wait set handle being >> added >> > to > >> a >> > wait set). It would reduce the API surface by one function (eliminating >> > WaitSetClose()), but probably wouldn't affect implementation complexity >> very >> > much. >> > > I'm a fan of having a single unified handle type. You also can't read or >> write >> shared buffers (and likewise you can't dupe message pipe endpoints), but >> they >> all use MojoHandle. >> > > I'm kinda coming around to this as well. I like Darin's point that using > MojoHandle would allow us to re-use some of the C++ bindings > (ScopedHandle, and > maybe more). > > > > >> > 2. Separate functions for add/remove. epoll() has a single function that >> > does > >> > add/remove/modify. This didn't 'feel' like it fit into the current Mojo >> API >> > design, so I went with separate functions. There's no modify, since I >> didn't >> > think it was necessary. >> > > Seems completely reasonable to have separate add/remove functions. IMHO >> this >> > API > >> is easier to read and write than a unified function with operational >> flags. >> > > Thanks. Pretty much my thought as well. > > > > >> > 3. A handle can only be added to a wait set once. This is different from >> > WaitMany, where you can add a handle multiple times with different >> signals. >> > WaitSet is a stateful API whereas WaitMany is one-shot. You need to be >> able >> > to > >> > add and remove waiters, and having keying by handle+signal seemed >> > problematic. > >> > Note, epoll also only allows an fd to be added once to the set. >> > > It seems acceptable to enforce this constraint, but I wonder if it's >> > necessary. > >> It seems non-problematic to still key on handle alone, but treat >> addition/removal as flag set union/subtraction. If a remove operation >> results >> > in > >> 0 signal flags, the handle is removed from the wait set. >> > > Hmmm. Implementation wise, this would be easy. But my fear is that this > would > lead to issues with semantics, and be difficult to use correctly. For > instance, > if you key by handle alone, which one do you remove when you call > Remove()? I > think the effect, as long as you have a single controller for the wait > call, can > be achieved by the caller instead of doing this inside the wait set > primitive. > > > > >> > 4. Wait() only returns one handle, unlike WaitMany and epoll which can >> > return > >> > many. The upside is a smaller API surface and possibly simpler >> > implementation. > >> > The negative is increased overhead (having to call Wait() many times) >> when >> > several handles are signalled. >> > > I think emulating MojoWaitMany here adds a little complexity to the API but >> > not > >> much. Maybe we could do something in between, where you explicitly pass >> the >> > max > >> number of signals you are willing to hear about for a single call. >> Passing 1 >> > as > >> that max would let us use the API as you have it defined now. >> > > What you describe is exactly what epoll does. It lets you wait for up to N > fds > to become ready. It's not a huge deal, but I thought it's worth bringing up > anyways. I'm not attached to either way. > > > > >> > 5. What happens when you call wait at the same time from multiple >> threads? >> I've >> > defined it, but I'm not 100% happy with it. >> > > I probably haven't put nearly enough thought into this, but why would >> anyone >> legitimately want to wait on the same handle from multiple threads? I'd be >> > happy > >> to go with your description as-is with the addendum that nobody should >> ever >> cause this to happen. >> > > So, it's unlikely we'll use this in Chrome. However, if we look at epoll, > there > are cases where it's useful to have multiple threads call wait on the same > set. > In epoll, it lets you have an efficient, multi-threaded IO loop thread-pool > (think EventManager in google3). On the flip side, epoll is much better at > supporting this because it's edge-triggered and has a one-shot wait > feature, > whereas Mojo's level-triggering would cause a thundering-herd effect > because > multiple threads would get woken up. Regardless, this still needs to be > defined > because Mojo should be thread-safe. We could cop-out and define it to > return > BUSY if you wait from multiple threads. > > > > 6. naming >> > > Seems fine to me, though the resulting name of WaitSetWait is a little >> unfortunate. We could throw around some other synonyms for WaitSet, e.g., >> WatchSet or something. >> > > I'd like to keep "wait" as part of it since the concept of waiting is a > well-defined action in Mojo. But WaitSetWait is... nasty. > > https://codereview.chromium.org/1429553002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes and yes, if the set is represented by a MojoHandle. Then the questions become: 1. What does signal state represent? Presumably it's the state of the wait set itself. But an argument could also be made that it refers to the handles in the set being waited on. For example, you can wait for just the readable handles or just the writeable handles. 2. Do you have a sole GetWokenHandle() always used in combination with Wait(), or provide a combined WaitAndGet() like I've done in this CL? On 30 October 2015 at 12:50, Darin Fisher <darin@chromium.org> wrote: > Can MojoWait be used to wait on a WaitSet? Could the signalled state be > queried from the WaitSet after MojoWait returns? > > -Darin > On Oct 28, 2015 11:36 PM, <amistry@chromium.org> wrote: > >> On 2015/10/26 23:50:25, Anand Mistry wrote: >>> > Hi all, >>> > >>> > This change is what I would consider and API for wait sets in Mojo. >>> It's >>> > analogous to epoll, but without all the options (and only >>> level-triggered). >>> > Rather than debating the API in a doc, I thought I'd put together this >>> >> change >> >>> > and kill two birds with one stone. Hopefully, it'll let me make >>> progress a >>> >> bit >> >>> > quicker than I've been doing so far. >>> > >>> > There are lots of questionable design choices which I'd like everyone >>> to >>> comment >>> > on. Specifically (but not limited to): >>> > 1. Re-use MojoHandle vs. a new handle type. In this change, I've gone >>> for >>> >> the >> >>> > latter. But I can certainly see the pros and cons of each. For reusing >>> > MojoHandle, on one hand, very few of the usual handle properties would >>> >> apply. >> >>> > You can't read/write or pass the handle. But you can close it, and >>> wait on >>> >> it, >> >>> > leading to interesting cases of nested waits (a wait set handle being >>> added >>> >> to >> >>> a >>> > wait set). It would reduce the API surface by one function (eliminating >>> > WaitSetClose()), but probably wouldn't affect implementation >>> complexity very >>> > much. >>> >> >> I'm a fan of having a single unified handle type. You also can't read or >>> write >>> shared buffers (and likewise you can't dupe message pipe endpoints), but >>> they >>> all use MojoHandle. >>> >> >> I'm kinda coming around to this as well. I like Darin's point that using >> MojoHandle would allow us to re-use some of the C++ bindings >> (ScopedHandle, and >> maybe more). >> >> >> > >>> > 2. Separate functions for add/remove. epoll() has a single function >>> that >>> >> does >> >>> > add/remove/modify. This didn't 'feel' like it fit into the current >>> Mojo API >>> > design, so I went with separate functions. There's no modify, since I >>> didn't >>> > think it was necessary. >>> >> >> Seems completely reasonable to have separate add/remove functions. IMHO >>> this >>> >> API >> >>> is easier to read and write than a unified function with operational >>> flags. >>> >> >> Thanks. Pretty much my thought as well. >> >> >> > >>> > 3. A handle can only be added to a wait set once. This is different >>> from >>> > WaitMany, where you can add a handle multiple times with different >>> signals. >>> > WaitSet is a stateful API whereas WaitMany is one-shot. You need to be >>> able >>> >> to >> >>> > add and remove waiters, and having keying by handle+signal seemed >>> >> problematic. >> >>> > Note, epoll also only allows an fd to be added once to the set. >>> >> >> It seems acceptable to enforce this constraint, but I wonder if it's >>> >> necessary. >> >>> It seems non-problematic to still key on handle alone, but treat >>> addition/removal as flag set union/subtraction. If a remove operation >>> results >>> >> in >> >>> 0 signal flags, the handle is removed from the wait set. >>> >> >> Hmmm. Implementation wise, this would be easy. But my fear is that this >> would >> lead to issues with semantics, and be difficult to use correctly. For >> instance, >> if you key by handle alone, which one do you remove when you call >> Remove()? I >> think the effect, as long as you have a single controller for the wait >> call, can >> be achieved by the caller instead of doing this inside the wait set >> primitive. >> >> >> > >>> > 4. Wait() only returns one handle, unlike WaitMany and epoll which can >>> >> return >> >>> > many. The upside is a smaller API surface and possibly simpler >>> >> implementation. >> >>> > The negative is increased overhead (having to call Wait() many times) >>> when >>> > several handles are signalled. >>> >> >> I think emulating MojoWaitMany here adds a little complexity to the API >>> but >>> >> not >> >>> much. Maybe we could do something in between, where you explicitly pass >>> the >>> >> max >> >>> number of signals you are willing to hear about for a single call. >>> Passing 1 >>> >> as >> >>> that max would let us use the API as you have it defined now. >>> >> >> What you describe is exactly what epoll does. It lets you wait for up to >> N fds >> to become ready. It's not a huge deal, but I thought it's worth bringing >> up >> anyways. I'm not attached to either way. >> >> >> > >>> > 5. What happens when you call wait at the same time from multiple >>> threads? >>> I've >>> > defined it, but I'm not 100% happy with it. >>> >> >> I probably haven't put nearly enough thought into this, but why would >>> anyone >>> legitimately want to wait on the same handle from multiple threads? I'd >>> be >>> >> happy >> >>> to go with your description as-is with the addendum that nobody should >>> ever >>> cause this to happen. >>> >> >> So, it's unlikely we'll use this in Chrome. However, if we look at epoll, >> there >> are cases where it's useful to have multiple threads call wait on the >> same set. >> In epoll, it lets you have an efficient, multi-threaded IO loop >> thread-pool >> (think EventManager in google3). On the flip side, epoll is much better at >> supporting this because it's edge-triggered and has a one-shot wait >> feature, >> whereas Mojo's level-triggering would cause a thundering-herd effect >> because >> multiple threads would get woken up. Regardless, this still needs to be >> defined >> because Mojo should be thread-safe. We could cop-out and define it to >> return >> BUSY if you wait from multiple threads. >> >> >> > 6. naming >>> >> >> Seems fine to me, though the resulting name of WaitSetWait is a little >>> unfortunate. We could throw around some other synonyms for WaitSet, e.g., >>> WatchSet or something. >>> >> >> I'd like to keep "wait" as part of it since the concept of waiting is a >> well-defined action in Mojo. But WaitSetWait is... nasty. >> >> https://codereview.chromium.org/1429553002/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/30 at 03:38:07, amistry wrote: > Yes and yes, if the set is represented by a MojoHandle. Then the questions > become: > 1. What does signal state represent? Presumably it's the state of the wait > set itself. But an argument could also be made that it refers to the > handles in the set being waited on. For example, you can wait for just the > readable handles or just the writeable handles. I think the first suggestion makes more sense. I don't know that there's much value in doing the second thing and it sounds slightly more complex? > 2. Do you have a sole GetWokenHandle() always used in combination with > Wait(), or provide a combined WaitAndGet() like I've done in this CL? I don't really like the idea of having a separate GetWokenHandle. Why don't we add another out arguemnt (mojoHandle* signaled_handle) to MojoWait. For single-handle waits this will be the input |handle| for consistency. For wait-set waits it will be the woken handle. > > On 30 October 2015 at 12:50, Darin Fisher <darin@chromium.org> wrote: > > > Can MojoWait be used to wait on a WaitSet? Could the signalled state be > > queried from the WaitSet after MojoWait returns? > > > > -Darin > > On Oct 28, 2015 11:36 PM, <amistry@chromium.org> wrote: > > > >> On 2015/10/26 23:50:25, Anand Mistry wrote: > >>> > Hi all, > >>> > > >>> > This change is what I would consider and API for wait sets in Mojo. > >>> It's > >>> > analogous to epoll, but without all the options (and only > >>> level-triggered). > >>> > Rather than debating the API in a doc, I thought I'd put together this > >>> > >> change > >> > >>> > and kill two birds with one stone. Hopefully, it'll let me make > >>> progress a > >>> > >> bit > >> > >>> > quicker than I've been doing so far. > >>> > > >>> > There are lots of questionable design choices which I'd like everyone > >>> to > >>> comment > >>> > on. Specifically (but not limited to): > >>> > 1. Re-use MojoHandle vs. a new handle type. In this change, I've gone > >>> for > >>> > >> the > >> > >>> > latter. But I can certainly see the pros and cons of each. For reusing > >>> > MojoHandle, on one hand, very few of the usual handle properties would > >>> > >> apply. > >> > >>> > You can't read/write or pass the handle. But you can close it, and > >>> wait on > >>> > >> it, > >> > >>> > leading to interesting cases of nested waits (a wait set handle being > >>> added > >>> > >> to > >> > >>> a > >>> > wait set). It would reduce the API surface by one function (eliminating > >>> > WaitSetClose()), but probably wouldn't affect implementation > >>> complexity very > >>> > much. > >>> > >> > >> I'm a fan of having a single unified handle type. You also can't read or > >>> write > >>> shared buffers (and likewise you can't dupe message pipe endpoints), but > >>> they > >>> all use MojoHandle. > >>> > >> > >> I'm kinda coming around to this as well. I like Darin's point that using > >> MojoHandle would allow us to re-use some of the C++ bindings > >> (ScopedHandle, and > >> maybe more). > >> > >> > >> > > >>> > 2. Separate functions for add/remove. epoll() has a single function > >>> that > >>> > >> does > >> > >>> > add/remove/modify. This didn't 'feel' like it fit into the current > >>> Mojo API > >>> > design, so I went with separate functions. There's no modify, since I > >>> didn't > >>> > think it was necessary. > >>> > >> > >> Seems completely reasonable to have separate add/remove functions. IMHO > >>> this > >>> > >> API > >> > >>> is easier to read and write than a unified function with operational > >>> flags. > >>> > >> > >> Thanks. Pretty much my thought as well. > >> > >> > >> > > >>> > 3. A handle can only be added to a wait set once. This is different > >>> from > >>> > WaitMany, where you can add a handle multiple times with different > >>> signals. > >>> > WaitSet is a stateful API whereas WaitMany is one-shot. You need to be > >>> able > >>> > >> to > >> > >>> > add and remove waiters, and having keying by handle+signal seemed > >>> > >> problematic. > >> > >>> > Note, epoll also only allows an fd to be added once to the set. > >>> > >> > >> It seems acceptable to enforce this constraint, but I wonder if it's > >>> > >> necessary. > >> > >>> It seems non-problematic to still key on handle alone, but treat > >>> addition/removal as flag set union/subtraction. If a remove operation > >>> results > >>> > >> in > >> > >>> 0 signal flags, the handle is removed from the wait set. > >>> > >> > >> Hmmm. Implementation wise, this would be easy. But my fear is that this > >> would > >> lead to issues with semantics, and be difficult to use correctly. For > >> instance, > >> if you key by handle alone, which one do you remove when you call > >> Remove()? I > >> think the effect, as long as you have a single controller for the wait > >> call, can > >> be achieved by the caller instead of doing this inside the wait set > >> primitive. > >> > >> > >> > > >>> > 4. Wait() only returns one handle, unlike WaitMany and epoll which can > >>> > >> return > >> > >>> > many. The upside is a smaller API surface and possibly simpler > >>> > >> implementation. > >> > >>> > The negative is increased overhead (having to call Wait() many times) > >>> when > >>> > several handles are signalled. > >>> > >> > >> I think emulating MojoWaitMany here adds a little complexity to the API > >>> but > >>> > >> not > >> > >>> much. Maybe we could do something in between, where you explicitly pass > >>> the > >>> > >> max > >> > >>> number of signals you are willing to hear about for a single call. > >>> Passing 1 > >>> > >> as > >> > >>> that max would let us use the API as you have it defined now. > >>> > >> > >> What you describe is exactly what epoll does. It lets you wait for up to > >> N fds > >> to become ready. It's not a huge deal, but I thought it's worth bringing > >> up > >> anyways. I'm not attached to either way. > >> > >> > >> > > >>> > 5. What happens when you call wait at the same time from multiple > >>> threads? > >>> I've > >>> > defined it, but I'm not 100% happy with it. > >>> > >> > >> I probably haven't put nearly enough thought into this, but why would > >>> anyone > >>> legitimately want to wait on the same handle from multiple threads? I'd > >>> be > >>> > >> happy > >> > >>> to go with your description as-is with the addendum that nobody should > >>> ever > >>> cause this to happen. > >>> > >> > >> So, it's unlikely we'll use this in Chrome. However, if we look at epoll, > >> there > >> are cases where it's useful to have multiple threads call wait on the > >> same set. > >> In epoll, it lets you have an efficient, multi-threaded IO loop > >> thread-pool > >> (think EventManager in google3). On the flip side, epoll is much better at > >> supporting this because it's edge-triggered and has a one-shot wait > >> feature, > >> whereas Mojo's level-triggering would cause a thundering-herd effect > >> because > >> multiple threads would get woken up. Regardless, this still needs to be > >> defined > >> because Mojo should be thread-safe. We could cop-out and define it to > >> return > >> BUSY if you wait from multiple threads. > >> > >> > >> > 6. naming > >>> > >> > >> Seems fine to me, though the resulting name of WaitSetWait is a little > >>> unfortunate. We could throw around some other synonyms for WaitSet, e.g., > >>> WatchSet or something. > >>> > >> > >> I'd like to keep "wait" as part of it since the concept of waiting is a > >> well-defined action in Mojo. But WaitSetWait is... nasty. > >> > >> https://codereview.chromium.org/1429553002/ > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On 2015/11/09 20:48:41, Ken Rockot wrote: > On 2015/10/30 at 03:38:07, amistry wrote: > > Yes and yes, if the set is represented by a MojoHandle. Then the questions > > become: > > 1. What does signal state represent? Presumably it's the state of the wait > > set itself. But an argument could also be made that it refers to the > > handles in the set being waited on. For example, you can wait for just the > > readable handles or just the writeable handles. > > I think the first suggestion makes more sense. I don't know that there's much > value in doing the second thing and it sounds slightly more complex? > > > 2. Do you have a sole GetWokenHandle() always used in combination with > > Wait(), or provide a combined WaitAndGet() like I've done in this CL? > > I don't really like the idea of having a separate GetWokenHandle. Why don't > we add another out arguemnt (mojoHandle* signaled_handle) to MojoWait. > > For single-handle waits this will be the input |handle| for consistency. > For wait-set waits it will be the woken handle. More paint for the shed: It seems a little awkward for *signaled_handle to sometimes be the handle passed to MojoWait and other times be a different handle. Another approach might be to optimize the API for wait sets. You could make WaitSet be the underlying primitive, and then just make MojoWait and MojoWaitMany be wrappers around the WaitSet API. This might lead us to develop a core System API entry point that is better optimized for WaitSet than trying to shoehorn the existing MojoWait to work with WaitSet handles. -Darin
On 2015/11/09 at 21:04:43, darin wrote: > On 2015/11/09 20:48:41, Ken Rockot wrote: > > On 2015/10/30 at 03:38:07, amistry wrote: > > > Yes and yes, if the set is represented by a MojoHandle. Then the questions > > > become: > > > 1. What does signal state represent? Presumably it's the state of the wait > > > set itself. But an argument could also be made that it refers to the > > > handles in the set being waited on. For example, you can wait for just the > > > readable handles or just the writeable handles. > > > > I think the first suggestion makes more sense. I don't know that there's much > > value in doing the second thing and it sounds slightly more complex? > > > > > 2. Do you have a sole GetWokenHandle() always used in combination with > > > Wait(), or provide a combined WaitAndGet() like I've done in this CL? > > > > I don't really like the idea of having a separate GetWokenHandle. Why don't > > we add another out arguemnt (mojoHandle* signaled_handle) to MojoWait. > > > > For single-handle waits this will be the input |handle| for consistency. > > For wait-set waits it will be the woken handle. > > More paint for the shed: It seems a little awkward for *signaled_handle > to sometimes be the handle passed to MojoWait and other times be a different > handle. Good point. In this sense it would be inconsistent, and that would be bad. > > Another approach might be to optimize the API for wait sets. You could > make WaitSet be the underlying primitive, and then just make MojoWait > and MojoWaitMany be wrappers around the WaitSet API. This might lead us > to develop a core System API entry point that is better optimized for > WaitSet than trying to shoehorn the existing MojoWait to work with > WaitSet handles. Or could we just change MojoWait to be that WaitSet-optimized entry point. It could only accept a WaitSet handle as input (to avoid the inconsistency you pointed out above), and we could get rid of MojoWaitMany entirely. Are there really many practical uses for a wait API that only waits on a single pipe handle? > > -Darin
On Mon, Nov 9, 2015 at 1:16 PM, <rockot@chromium.org> wrote: > On 2015/11/09 at 21:04:43, darin wrote: > >> On 2015/11/09 20:48:41, Ken Rockot wrote: >> > On 2015/10/30 at 03:38:07, amistry wrote: >> > > Yes and yes, if the set is represented by a MojoHandle. Then the >> questions >> > > become: >> > > 1. What does signal state represent? Presumably it's the state of the >> wait >> > > set itself. But an argument could also be made that it refers to the >> > > handles in the set being waited on. For example, you can wait for >> just the >> > > readable handles or just the writeable handles. >> > >> > I think the first suggestion makes more sense. I don't know that there's >> > much > >> > value in doing the second thing and it sounds slightly more complex? >> > >> > > 2. Do you have a sole GetWokenHandle() always used in combination with >> > > Wait(), or provide a combined WaitAndGet() like I've done in this CL? >> > >> > I don't really like the idea of having a separate GetWokenHandle. Why >> don't >> > we add another out arguemnt (mojoHandle* signaled_handle) to MojoWait. >> > >> > For single-handle waits this will be the input |handle| for consistency. >> > For wait-set waits it will be the woken handle. >> > > More paint for the shed: It seems a little awkward for *signaled_handle >> to sometimes be the handle passed to MojoWait and other times be a >> different >> handle. >> > > Good point. In this sense it would be inconsistent, and that would be bad. > > > Another approach might be to optimize the API for wait sets. You could >> make WaitSet be the underlying primitive, and then just make MojoWait >> and MojoWaitMany be wrappers around the WaitSet API. This might lead us >> to develop a core System API entry point that is better optimized for >> WaitSet than trying to shoehorn the existing MojoWait to work with >> WaitSet handles. >> > > Or could we just change MojoWait to be that WaitSet-optimized entry point. > It could only accept a WaitSet handle as input (to avoid the inconsistency > you > pointed out above), and we could get rid of MojoWaitMany entirely. > > Getting rid of MojoWaitMany makes sense. It could be kept around as veneer around the WaitSet API of course. > Are there really many practical uses for a wait API that only waits on a > single > pipe handle? > Sync calls? WaitForIncomingMethodCall Note: We might find that we should augment all of these calls with a second "shutdown" handle like we do in IPC::SyncChannel. That way, there is a convenient way to interrupt all sync calls. The current solution is to just count on the pipe being closed on the other end to interrupt the WaitForIncomingMethodCall method. The other use case of MojoWait is for probing the signaled state of a handle. This is done by specifying a deadline of 0. -Darin > > > -Darin >> > > > > https://codereview.chromium.org/1429553002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi all, I've updated this change based on the discussion, as well as my own experience implementing it. A few specific points I'd like to call out: 1. Change to making a wait set a MojoHandle. It occurs to me that the only common operation between MojoHandle types is transferability (other than closing). The functions to interact with each type is different, so I don't see any harm in introducing new functions to deal specifically with wait sets. Not being transferable I think is a reasonable compromise. The other thing to note is that because the implementation is based on |Dispatcher|s, there's some locking weirdness. 2. As a result of 1, waiting for the wait set is now done by MojoWait() and retrieving woken handles is a separate function. I agree with the arguments about not altering the existing MojoWait() function to special-case wait sets. There are also implementation details which should be addressed (MojoWait() is implemented using a WaitMany(), which does too many unnecessary allocations), but those don't affect the API surface. 3. MojoGetReadyHandle() (naming, ick!) returns a single handle. I've thought about the overhead in the case of multiple ready handles, but I don't think it's going to be a major issue. Right now, MojoWaitMany() as used by by MessagePumpMojo, only wakes one handle at a time. It's likely that only a small number of handles will be woken at any given point in time. One exception is when a process dies and all message pipes to that process have their peer closed. But even then... 4. MojoWait() may result in spurious wake-ups. This is a consequence of the thread-safety of adding and removing handles. I thought it was good to spell this out explicitly. 5. Also due to threading, MojoGetReadyHandle() may return an invalid handle.
Makes sense to me. I like your bikeshed color. (I didn't do a detailed review.) On Nov 12, 2015 4:21 PM, <amistry@chromium.org> wrote: > Hi all, > > I've updated this change based on the discussion, as well as my own > experience > implementing it. > > A few specific points I'd like to call out: > 1. Change to making a wait set a MojoHandle. It occurs to me that the only > common operation between MojoHandle types is transferability (other than > closing). The functions to interact with each type is different, so I > don't see > any harm in introducing new functions to deal specifically with wait sets. > Not > being transferable I think is a reasonable compromise. > > The other thing to note is that because the implementation is based on > |Dispatcher|s, there's some locking weirdness. > > 2. As a result of 1, waiting for the wait set is now done by MojoWait() and > retrieving woken handles is a separate function. I agree with the arguments > about not altering the existing MojoWait() function to special-case wait > sets. > There are also implementation details which should be addressed > (MojoWait() is > implemented using a WaitMany(), which does too many unnecessary > allocations), > but those don't affect the API surface. > > 3. MojoGetReadyHandle() (naming, ick!) returns a single handle. I've > thought > about the overhead in the case of multiple ready handles, but I don't > think it's > going to be a major issue. Right now, MojoWaitMany() as used by by > MessagePumpMojo, only wakes one handle at a time. It's likely that only a > small > number of handles will be woken at any given point in time. One exception > is > when a process dies and all message pipes to that process have their peer > closed. But even then... > > 4. MojoWait() may result in spurious wake-ups. This is a consequence of the > thread-safety of adding and removing handles. I thought it was good to > spell > this out explicitly. > > 5. Also due to threading, MojoGetReadyHandle() may return an invalid > handle. > > https://codereview.chromium.org/1429553002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
reviewers: Ping! Despite the lack of implementation here, I still want to submit this.
Sorry for the delayed response. lgtm but you'll need to rebase and adjust all the mojo/public references to the new location https://codereview.chromium.org/1429553002/diff/20001/third_party/mojo/src/mo... File third_party/mojo/src/mojo/public/c/system/BUILD.gn (right): https://codereview.chromium.org/1429553002/diff/20001/third_party/mojo/src/mo... third_party/mojo/src/mojo/public/c/system/BUILD.gn:21: "wait_set.h", Might as well update GYP too (third_party/mojo/mojo_public.gyp:mojo_system_headers).
Description was changed from ========== API definition for WaitSet. ========== to ========== API definition for WaitSet. BUG=556865 ==========
https://codereview.chromium.org/1429553002/diff/20001/third_party/mojo/src/mo... File third_party/mojo/src/mojo/public/c/system/BUILD.gn (right): https://codereview.chromium.org/1429553002/diff/20001/third_party/mojo/src/mo... third_party/mojo/src/mojo/public/c/system/BUILD.gn:21: "wait_set.h", On 2015/11/17 01:50:23, Ken Rockot wrote: > Might as well update GYP too > (third_party/mojo/mojo_public.gyp:mojo_system_headers). Done.
jam, yzshen, sammc: Ping! Please review for submit.
https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:35: // Add a wait on |handle| to |wait_set_handle|. Nit: please use "Adds" because the omitted subject is "this function". (here and elsewhere) https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:101: // subsequent calls to |MojoGetReadyHandle()| will return the same handle. I wonder how the wait set will determine the order to return ready handles. (Maybe it is implementation related?) (1) For example, assume {a, b} are two handles that are ready in the wait set. I call MojoGetReadyHandle() and it returns |a|. The comment sounds like if I don't do something with |a| (say, draining the data or removing it from the set), subsequent calls will return |a| (instead of |b|) for sure. (2) The other question is about starvation: assume {a, b} are two handles that are ready in the wait set. I call MojoGetReadyHandle() and it returns |a|. I drain the data of |a|, process the data and then call MojoGetReadyHandle() again. In the mean time, there is new data available for |a|, so |a| is ready again. If this pattern repeats, is it possible that |b| is never returned by MojoGetReadyHandle() and "starved"? It seems nice to add some comments to clarify. https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:105: MojoHandle *handle, // Out. style nit: '*' should be adjacent to the type instead of the value. (here and elsewhere)
https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:35: // Add a wait on |handle| to |wait_set_handle|. On 2015/11/18 23:50:58, yzshen1 wrote: > Nit: please use "Adds" because the omitted subject is "this function". (here and > elsewhere) Done? Sorry, grammar has never been my strong point. https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:101: // subsequent calls to |MojoGetReadyHandle()| will return the same handle. Added a comment, and explanation inline. On 2015/11/18 23:50:58, yzshen1 wrote: > I wonder how the wait set will determine the order to return ready handles. > (Maybe it is implementation related?) > > (1) For example, assume {a, b} are two handles that are ready in the wait set. I > call MojoGetReadyHandle() and it returns |a|. The comment sounds like if I don't > do something with |a| (say, draining the data or removing it from the set), > subsequent calls will return |a| (instead of |b|) for sure. I picked this wording because it represents a worst-case scenario that the user must handle. It's not strictly correct because the implementation could round-robin, or randomly pick. But this isn't guaranteed by the API. I chose not to have any guarantees here in order to allow an efficient implementation, similar to how mutexes generally aren't fair. > > (2) The other question is about starvation: assume {a, b} are two handles that > are ready in the wait set. I call MojoGetReadyHandle() and it returns |a|. I > drain the data of |a|, process the data and then call MojoGetReadyHandle() > again. In the mean time, there is new data available for |a|, so |a| is ready > again. If this pattern repeats, is it possible that |b| is never returned by > MojoGetReadyHandle() and "starved"? According to the API, yes (but in my current implementation, no, because I round-robin). I would argue that if you run into this situation, you have bigger problems. I think that this behaviour is reasonable and the notion of fairness here should be handled at a higher level. The wait set has no notion of what the best way to handle this case is. It might be that you want efficiency, and repeatedly handling the same handle is most efficient. It could also be that you want some other metric of fairness, such as distributing equal bandwidth between renderers as not to starve any particular page. Encoding any notion of fairness won't capture every case, and puts restrictions on the implementation. That said, I'm willing to reconsider fairness as part of the API. Note that epoll doesn't guarantee any notion of fairness. And the specific situation you called out would be handled by an epoll user by using edge-triggering and draining the ready queue each time before handling the each fd. > > It seems nice to add some comments to clarify. > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:105: MojoHandle *handle, // Out. On 2015/11/18 23:50:58, yzshen1 wrote: > style nit: '*' should be adjacent to the type instead of the value. (here and > elsewhere) Done.
https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:101: // subsequent calls to |MojoGetReadyHandle()| will return the same handle. On 2015/11/19 04:13:45, Anand Mistry wrote: > Added a comment, and explanation inline. > > On 2015/11/18 23:50:58, yzshen1 wrote: > > I wonder how the wait set will determine the order to return ready handles. > > (Maybe it is implementation related?) > > > > (1) For example, assume {a, b} are two handles that are ready in the wait set. > I > > call MojoGetReadyHandle() and it returns |a|. The comment sounds like if I > don't > > do something with |a| (say, draining the data or removing it from the set), > > subsequent calls will return |a| (instead of |b|) for sure. > > I picked this wording because it represents a worst-case scenario that the user > must handle. It's not strictly correct because the implementation could > round-robin, or randomly pick. But this isn't guaranteed by the API. I chose not > to have any guarantees here in order to allow an efficient implementation, > similar to how mutexes generally aren't fair. > > > > > (2) The other question is about starvation: assume {a, b} are two handles that > > are ready in the wait set. I call MojoGetReadyHandle() and it returns |a|. I > > drain the data of |a|, process the data and then call MojoGetReadyHandle() > > again. In the mean time, there is new data available for |a|, so |a| is ready > > again. If this pattern repeats, is it possible that |b| is never returned by > > MojoGetReadyHandle() and "starved"? > > According to the API, yes (but in my current implementation, no, because I > round-robin). I would argue that if you run into this situation, you have bigger > problems. I think that this behaviour is reasonable and the notion of fairness > here should be handled at a higher level. The wait set has no notion of what the > best way to handle this case is. It might be that you want efficiency, and > repeatedly handling the same handle is most efficient. It could also be that you > want some other metric of fairness, such as distributing equal bandwidth between > renderers as not to starve any particular page. Encoding any notion of fairness > won't capture every case, and puts restrictions on the implementation. > > That said, I'm willing to reconsider fairness as part of the API. Note that > epoll doesn't guarantee any notion of fairness. And the specific situation you > called out would be handled by an epoll user by using edge-triggering and > draining the ready queue each time before handling the each fd. > > > > > It seems nice to add some comments to clarify. > > > Thanks for the detailed reply! I totally agree that "fairness" shouldn't be handled by this API because it really depends on the user scenarios. I just think it is useful to explicitly comment about it. (Your updated comment looks good.) That being said, if the user wants to make scheduling decision himself, this API seems a little inconvenient for the user to learn "the whole picture", i.e., all the handles that are currently ready. It seems the user needs to: call MojoGetReadyHandle() and then MojoRemoveWaiter() to remove the ready handle returned; repeat this process until no ready handle. And then add all those handles back to the wait set (if he wants to continue watching). Is my understanding correct?
On 2015/11/19 06:52:25, yzshen1 wrote: > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > File mojo/public/c/system/wait_set.h (right): > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > mojo/public/c/system/wait_set.h:101: // subsequent calls to > |MojoGetReadyHandle()| will return the same handle. > On 2015/11/19 04:13:45, Anand Mistry wrote: > > Added a comment, and explanation inline. > > > > On 2015/11/18 23:50:58, yzshen1 wrote: > > > I wonder how the wait set will determine the order to return ready handles. > > > (Maybe it is implementation related?) > > > > > > (1) For example, assume {a, b} are two handles that are ready in the wait > set. > > I > > > call MojoGetReadyHandle() and it returns |a|. The comment sounds like if I > > don't > > > do something with |a| (say, draining the data or removing it from the set), > > > subsequent calls will return |a| (instead of |b|) for sure. > > > > I picked this wording because it represents a worst-case scenario that the > user > > must handle. It's not strictly correct because the implementation could > > round-robin, or randomly pick. But this isn't guaranteed by the API. I chose > not > > to have any guarantees here in order to allow an efficient implementation, > > similar to how mutexes generally aren't fair. > > > > > > > > (2) The other question is about starvation: assume {a, b} are two handles > that > > > are ready in the wait set. I call MojoGetReadyHandle() and it returns |a|. I > > > drain the data of |a|, process the data and then call MojoGetReadyHandle() > > > again. In the mean time, there is new data available for |a|, so |a| is > ready > > > again. If this pattern repeats, is it possible that |b| is never returned by > > > MojoGetReadyHandle() and "starved"? > > > > According to the API, yes (but in my current implementation, no, because I > > round-robin). I would argue that if you run into this situation, you have > bigger > > problems. I think that this behaviour is reasonable and the notion of fairness > > here should be handled at a higher level. The wait set has no notion of what > the > > best way to handle this case is. It might be that you want efficiency, and > > repeatedly handling the same handle is most efficient. It could also be that > you > > want some other metric of fairness, such as distributing equal bandwidth > between > > renderers as not to starve any particular page. Encoding any notion of > fairness > > won't capture every case, and puts restrictions on the implementation. > > > > That said, I'm willing to reconsider fairness as part of the API. Note that > > epoll doesn't guarantee any notion of fairness. And the specific situation you > > called out would be handled by an epoll user by using edge-triggering and > > draining the ready queue each time before handling the each fd. > > > > > > > > It seems nice to add some comments to clarify. > > > > > > > Thanks for the detailed reply! > > I totally agree that "fairness" shouldn't be handled by this API because it > really depends on the user scenarios. I just think it is useful to explicitly > comment about it. (Your updated comment looks good.) > > That being said, if the user wants to make scheduling decision himself, this API > seems a little inconvenient for the user to learn "the whole picture", i.e., all > the handles that are currently ready. I'd say that if you're interacting with this low-level API instead of using something like AsyncWaiter or HandleWatcher, you should be prepared to learn the whole picture. > > It seems the user needs to: call MojoGetReadyHandle() and then > MojoRemoveWaiter() to remove the ready handle returned; repeat this process > until no ready handle. And then add all those handles back to the wait set (if > he wants to continue watching). > > Is my understanding correct? Yes. However, two points: 1. This is basically how WaitMany functions internally, except it's much worse. And MessagePumpMojo already works like this. 2. The door to do something smarter is open. You could optimise your message loop for the case where a handler will act on the ready handle (i.e. drain a message pipe) and detect the case where it doesn't based on some heuristic. That way, most of the time, you don't have to pay any bookkeeping cost.
On 2015/11/19 07:51:26, Anand Mistry wrote: > On 2015/11/19 06:52:25, yzshen1 wrote: > > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > > File mojo/public/c/system/wait_set.h (right): > > > > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > > mojo/public/c/system/wait_set.h:101: // subsequent calls to > > |MojoGetReadyHandle()| will return the same handle. > > On 2015/11/19 04:13:45, Anand Mistry wrote: > > > Added a comment, and explanation inline. > > > > > > On 2015/11/18 23:50:58, yzshen1 wrote: > > > > I wonder how the wait set will determine the order to return ready > handles. > > > > (Maybe it is implementation related?) > > > > > > > > (1) For example, assume {a, b} are two handles that are ready in the wait > > set. > > > I > > > > call MojoGetReadyHandle() and it returns |a|. The comment sounds like if I > > > don't > > > > do something with |a| (say, draining the data or removing it from the > set), > > > > subsequent calls will return |a| (instead of |b|) for sure. > > > > > > I picked this wording because it represents a worst-case scenario that the > > user > > > must handle. It's not strictly correct because the implementation could > > > round-robin, or randomly pick. But this isn't guaranteed by the API. I chose > > not > > > to have any guarantees here in order to allow an efficient implementation, > > > similar to how mutexes generally aren't fair. > > > > > > > > > > > (2) The other question is about starvation: assume {a, b} are two handles > > that > > > > are ready in the wait set. I call MojoGetReadyHandle() and it returns |a|. > I > > > > drain the data of |a|, process the data and then call MojoGetReadyHandle() > > > > again. In the mean time, there is new data available for |a|, so |a| is > > ready > > > > again. If this pattern repeats, is it possible that |b| is never returned > by > > > > MojoGetReadyHandle() and "starved"? > > > > > > According to the API, yes (but in my current implementation, no, because I > > > round-robin). I would argue that if you run into this situation, you have > > bigger > > > problems. I think that this behaviour is reasonable and the notion of > fairness > > > here should be handled at a higher level. The wait set has no notion of what > > the > > > best way to handle this case is. It might be that you want efficiency, and > > > repeatedly handling the same handle is most efficient. It could also be that > > you > > > want some other metric of fairness, such as distributing equal bandwidth > > between > > > renderers as not to starve any particular page. Encoding any notion of > > fairness > > > won't capture every case, and puts restrictions on the implementation. > > > > > > That said, I'm willing to reconsider fairness as part of the API. Note that > > > epoll doesn't guarantee any notion of fairness. And the specific situation > you > > > called out would be handled by an epoll user by using edge-triggering and > > > draining the ready queue each time before handling the each fd. > > > > > > > > > > > It seems nice to add some comments to clarify. > > > > > > > > > > > Thanks for the detailed reply! > > > > I totally agree that "fairness" shouldn't be handled by this API because it > > really depends on the user scenarios. I just think it is useful to explicitly > > comment about it. (Your updated comment looks good.) > > > > That being said, if the user wants to make scheduling decision himself, this > API > > seems a little inconvenient for the user to learn "the whole picture", i.e., > all > > the handles that are currently ready. > > I'd say that if you're interacting with this low-level API instead of using > something like AsyncWaiter or HandleWatcher, you should be prepared to learn the > whole picture. Exactly. That is what I was trying to say: users of the wait set probably want to learn the whole picture and make scheduling decision themselves. However, MojoGetReadyHandle() is a little inconvenient for that purpose. At the first glance, it seems the API has made the scheduling decision for its users, because it returns a single ready handle that it picks with unspecified, impl-dependent algorithm. I have seen your comment about "returning a single handle should be okay" in reply #15. But I didn't see why this approach is better than returning an array of all ready handles. Is it because of performance? > > > > > It seems the user needs to: call MojoGetReadyHandle() and then > > MojoRemoveWaiter() to remove the ready handle returned; repeat this process > > until no ready handle. And then add all those handles back to the wait set (if > > he wants to continue watching). > > > > Is my understanding correct? > > Yes. However, two points: > 1. This is basically how WaitMany functions internally, except it's much worse. > And MessagePumpMojo already works like this. Right. However, I think the main purpose is to look for ways to make improvements, not following existing work. Right? > 2. The door to do something smarter is open. You could optimise your message > loop for the case where a handler will act on the ready handle (i.e. drain a > message pipe) and detect the case where it doesn't based on some heuristic. That > way, most of the time, you don't have to pay any bookkeeping cost. Would you please tell me more about the idea? I am not sure I understand the part of "detect the case where it doesn't based on some heuristic". (Sorry for raising questions after the CL has gone through many rounds of reviews. I just want to make sure I have understood the considerations behind the design.)
On 2015/11/19 17:05:42, yzshen1 wrote: > On 2015/11/19 07:51:26, Anand Mistry wrote: > > On 2015/11/19 06:52:25, yzshen1 wrote: > > > > > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > > > File mojo/public/c/system/wait_set.h (right): > > > > > > > > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > > > mojo/public/c/system/wait_set.h:101: // subsequent calls to > > > |MojoGetReadyHandle()| will return the same handle. > > > On 2015/11/19 04:13:45, Anand Mistry wrote: > > > > Added a comment, and explanation inline. > > > > > > > > On 2015/11/18 23:50:58, yzshen1 wrote: > > > > > I wonder how the wait set will determine the order to return ready > > handles. > > > > > (Maybe it is implementation related?) > > > > > > > > > > (1) For example, assume {a, b} are two handles that are ready in the > wait > > > set. > > > > I > > > > > call MojoGetReadyHandle() and it returns |a|. The comment sounds like if > I > > > > don't > > > > > do something with |a| (say, draining the data or removing it from the > > set), > > > > > subsequent calls will return |a| (instead of |b|) for sure. > > > > > > > > I picked this wording because it represents a worst-case scenario that the > > > user > > > > must handle. It's not strictly correct because the implementation could > > > > round-robin, or randomly pick. But this isn't guaranteed by the API. I > chose > > > not > > > > to have any guarantees here in order to allow an efficient implementation, > > > > similar to how mutexes generally aren't fair. > > > > > > > > > > > > > > (2) The other question is about starvation: assume {a, b} are two > handles > > > that > > > > > are ready in the wait set. I call MojoGetReadyHandle() and it returns > |a|. > > I > > > > > drain the data of |a|, process the data and then call > MojoGetReadyHandle() > > > > > again. In the mean time, there is new data available for |a|, so |a| is > > > ready > > > > > again. If this pattern repeats, is it possible that |b| is never > returned > > by > > > > > MojoGetReadyHandle() and "starved"? > > > > > > > > According to the API, yes (but in my current implementation, no, because I > > > > round-robin). I would argue that if you run into this situation, you have > > > bigger > > > > problems. I think that this behaviour is reasonable and the notion of > > fairness > > > > here should be handled at a higher level. The wait set has no notion of > what > > > the > > > > best way to handle this case is. It might be that you want efficiency, and > > > > repeatedly handling the same handle is most efficient. It could also be > that > > > you > > > > want some other metric of fairness, such as distributing equal bandwidth > > > between > > > > renderers as not to starve any particular page. Encoding any notion of > > > fairness > > > > won't capture every case, and puts restrictions on the implementation. > > > > > > > > That said, I'm willing to reconsider fairness as part of the API. Note > that > > > > epoll doesn't guarantee any notion of fairness. And the specific situation > > you > > > > called out would be handled by an epoll user by using edge-triggering and > > > > draining the ready queue each time before handling the each fd. > > > > > > > > > > > > > > It seems nice to add some comments to clarify. > > > > > > > > > > > > > > > Thanks for the detailed reply! > > > > > > I totally agree that "fairness" shouldn't be handled by this API because it > > > really depends on the user scenarios. I just think it is useful to > explicitly > > > comment about it. (Your updated comment looks good.) > > > > > > That being said, if the user wants to make scheduling decision himself, this > > API > > > seems a little inconvenient for the user to learn "the whole picture", i.e., > > all > > > the handles that are currently ready. > > > > I'd say that if you're interacting with this low-level API instead of using > > something like AsyncWaiter or HandleWatcher, you should be prepared to learn > the > > whole picture. > > Exactly. That is what I was trying to say: users of the wait set probably want > to learn the whole picture and make scheduling decision themselves. However, > MojoGetReadyHandle() is a little inconvenient for that purpose. At the first > glance, it seems the API has made the scheduling decision for its users, because > it returns a single ready handle that it picks with unspecified, impl-dependent > algorithm. Hmm, I see your point. > I have seen your comment about "returning a single handle should be okay" in > reply #15. But I didn't see why this approach is better than returning an array > of all ready handles. Is it because of performance? No, not performance. I didn't mention this before, but I felt that if this returned multiple handles, there would be a mental disconnect between the API and the expectations of the user. Specifically, if you asked for 4 handles and this API returned 2, the user might assume that they need to wait. But due to implementation weirdness, this is not a correct assumption. Note that this isn't a problem, it's just unnecessary work. So really, it's just me. I can change it if you feel it's worth it. > > > > > > > > > It seems the user needs to: call MojoGetReadyHandle() and then > > > MojoRemoveWaiter() to remove the ready handle returned; repeat this process > > > until no ready handle. And then add all those handles back to the wait set > (if > > > he wants to continue watching). > > > > > > Is my understanding correct? > > > > Yes. However, two points: > > 1. This is basically how WaitMany functions internally, except it's much > worse. > > And MessagePumpMojo already works like this. > > Right. However, I think the main purpose is to look for ways to make > improvements, not following existing work. Right? Yup. I'm just pointing out that it's not worse than the current state, and probably not a big problem in practice. > > > 2. The door to do something smarter is open. You could optimise your message > > loop for the case where a handler will act on the ready handle (i.e. drain a > > message pipe) and detect the case where it doesn't based on some heuristic. > That > > way, most of the time, you don't have to pay any bookkeeping cost. > > Would you please tell me more about the idea? I am not sure I understand the > part of "detect the case where it doesn't based on some heuristic". So, the idea is that in the common case, only one or two handles will be woken up. In that case, you could pull the ready handles one by one, and run the handler right after reading it using GetReadyHandle(). Right now, the primary user of MessagePumpMojo, HandleWatcher, always removes the handle from the message pump in the handler (https://code.google.com/p/chromium/codesearch#chromium/src/mojo/message_pump/...). A more efficient user would drain the (for example) message pipe in the message pump handler and leave the handle in the message pump. Your situation could be detected by counting the number of ready handles that are handled, and if it exceeds some threshold (say 2), then get and remove all the ready handles before servicing them. Hmm, maybe I should just implement this to show you. It would be better than trying to explain. I'll upload a CL next week to demo. > > > (Sorry for raising questions after the CL has gone through many rounds of > reviews. I just want to make sure I have understood the considerations behind > the design.) Please, continue to comment. I want to get this right.
On Thu, Nov 19, 2015 at 9:16 PM, <amistry@chromium.org> wrote: > On 2015/11/19 17:05:42, yzshen1 wrote: > >> On 2015/11/19 07:51:26, Anand Mistry wrote: >> > On 2015/11/19 06:52:25, yzshen1 wrote: >> > > >> > >> > > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > >> > > File mojo/public/c/system/wait_set.h (right): >> > > >> > > >> > >> > > > https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... > >> > > mojo/public/c/system/wait_set.h:101: // subsequent calls to >> > > |MojoGetReadyHandle()| will return the same handle. >> > > On 2015/11/19 04:13:45, Anand Mistry wrote: >> > > > Added a comment, and explanation inline. >> > > > >> > > > On 2015/11/18 23:50:58, yzshen1 wrote: >> > > > > I wonder how the wait set will determine the order to return ready >> > handles. >> > > > > (Maybe it is implementation related?) >> > > > > >> > > > > (1) For example, assume {a, b} are two handles that are ready in >> the >> wait >> > > set. >> > > > I >> > > > > call MojoGetReadyHandle() and it returns |a|. The comment sounds >> like >> > if > >> I >> > > > don't >> > > > > do something with |a| (say, draining the data or removing it from >> the >> > set), >> > > > > subsequent calls will return |a| (instead of |b|) for sure. >> > > > >> > > > I picked this wording because it represents a worst-case scenario >> that >> > the > >> > > user >> > > > must handle. It's not strictly correct because the implementation >> could >> > > > round-robin, or randomly pick. But this isn't guaranteed by the >> API. I >> chose >> > > not >> > > > to have any guarantees here in order to allow an efficient >> > implementation, > >> > > > similar to how mutexes generally aren't fair. >> > > > >> > > > > >> > > > > (2) The other question is about starvation: assume {a, b} are two >> handles >> > > that >> > > > > are ready in the wait set. I call MojoGetReadyHandle() and it >> returns >> |a|. >> > I >> > > > > drain the data of |a|, process the data and then call >> MojoGetReadyHandle() >> > > > > again. In the mean time, there is new data available for |a|, so >> |a| >> > is > >> > > ready >> > > > > again. If this pattern repeats, is it possible that |b| is never >> returned >> > by >> > > > > MojoGetReadyHandle() and "starved"? >> > > > >> > > > According to the API, yes (but in my current implementation, no, >> because >> > I > >> > > > round-robin). I would argue that if you run into this situation, you >> > have > >> > > bigger >> > > > problems. I think that this behaviour is reasonable and the notion >> of >> > fairness >> > > > here should be handled at a higher level. The wait set has no >> notion of >> what >> > > the >> > > > best way to handle this case is. It might be that you want >> efficiency, >> > and > >> > > > repeatedly handling the same handle is most efficient. It could >> also be >> that >> > > you >> > > > want some other metric of fairness, such as distributing equal >> bandwidth >> > > between >> > > > renderers as not to starve any particular page. Encoding any notion >> of >> > > fairness >> > > > won't capture every case, and puts restrictions on the >> implementation. >> > > > >> > > > That said, I'm willing to reconsider fairness as part of the API. >> Note >> that >> > > > epoll doesn't guarantee any notion of fairness. And the specific >> > situation > >> > you >> > > > called out would be handled by an epoll user by using >> edge-triggering >> > and > >> > > > draining the ready queue each time before handling the each fd. >> > > > >> > > > > >> > > > > It seems nice to add some comments to clarify. >> > > > > >> > > > >> > > >> > > Thanks for the detailed reply! >> > > >> > > I totally agree that "fairness" shouldn't be handled by this API >> because >> > it > >> > > really depends on the user scenarios. I just think it is useful to >> explicitly >> > > comment about it. (Your updated comment looks good.) >> > > >> > > That being said, if the user wants to make scheduling decision >> himself, >> > this > >> > API >> > > seems a little inconvenient for the user to learn "the whole picture", >> > i.e., > >> > all >> > > the handles that are currently ready. >> > >> > I'd say that if you're interacting with this low-level API instead of >> using >> > something like AsyncWaiter or HandleWatcher, you should be prepared to >> learn >> the >> > whole picture. >> > > Exactly. That is what I was trying to say: users of the wait set probably >> want >> to learn the whole picture and make scheduling decision themselves. >> However, >> MojoGetReadyHandle() is a little inconvenient for that purpose. At the >> first >> glance, it seems the API has made the scheduling decision for its users, >> > because > >> it returns a single ready handle that it picks with unspecified, >> > impl-dependent > >> algorithm. >> > > Hmm, I see your point. > > I have seen your comment about "returning a single handle should be okay" >> in >> reply #15. But I didn't see why this approach is better than returning an >> > array > >> of all ready handles. Is it because of performance? >> > > No, not performance. I didn't mention this before, but I felt that if this > returned > multiple handles, there would be a mental disconnect between the API and > the > expectations of the user. Specifically, if you asked for 4 handles and > this API > returned 2, the user might assume that they need to wait. But due to > implementation > weirdness, this is not a correct assumption. Note that this isn't a > problem, > it's > just unnecessary work. > > So really, it's just me. I can change it if you feel it's worth it. Thanks for the reply! I feel that it would be more flexible if we support returning multiple handles, and likely (hopefully?) won't complicate the implementation. I imagine something like this: (The names, etc. haven't been carefully considered. Just to show my thoughts.) struct ReadyHandleInfo { MojoHandle handle; MojoHandleSignalsState signals_state; }; // |count| inputs the size of the |ready_handles| buffer; it outputs how many handles were written into |ready_handles|. // If |count| inputs 0, the function doesn't touch |ready_handles| but outputs the total number of ready handles in |count|. So if you want, you can call this function once to measure how big a buffer you should allocate, and then call a second time with the buffer. MojoResult MojoGetReadyHandles( MojoHandle wait_set_handle, int32_t count, // Inout param. ReadyHandleInfo* ready_handles); // Out param. The current function definition is equivalent to setting |count| to 1. > > > > > >> > > >> > > It seems the user needs to: call MojoGetReadyHandle() and then >> > > MojoRemoveWaiter() to remove the ready handle returned; repeat this >> > process > >> > > until no ready handle. And then add all those handles back to the >> wait set >> (if >> > > he wants to continue watching). >> > > >> > > Is my understanding correct? >> > >> > Yes. However, two points: >> > 1. This is basically how WaitMany functions internally, except it's much >> worse. >> > And MessagePumpMojo already works like this. >> > > Right. However, I think the main purpose is to look for ways to make >> improvements, not following existing work. Right? >> > > Yup. I'm just pointing out that it's not worse than the current state, and > probably > not a big problem in practice. Agree that it is not worse than the current state. :) > > > > > 2. The door to do something smarter is open. You could optimise your >> message >> > loop for the case where a handler will act on the ready handle (i.e. >> drain a >> > message pipe) and detect the case where it doesn't based on some >> heuristic. >> That >> > way, most of the time, you don't have to pay any bookkeeping cost. >> > > Would you please tell me more about the idea? I am not sure I understand >> the >> part of "detect the case where it doesn't based on some heuristic". >> > > So, the idea is that in the common case, only one or two handles will be > woken > up. > In that case, you could pull the ready handles one by one, and run the > handler > right > after reading it using GetReadyHandle(). Right now, the primary user of > MessagePumpMojo, > HandleWatcher, always removes the handle from the message pump in the > handler > ( > https://code.google.com/p/chromium/codesearch#chromium/src/mojo/message_pump/... > ). > A more efficient user would drain the (for example) message pipe in the > message > pump > handler and leave the handle in the message pump. Your situation could be > detected > by counting the number of ready handles that are handled, and if it > exceeds some > threshold (say 2), then get and remove all the ready handles before > servicing > them. > > Hmm, maybe I should just implement this to show you. It would be better > than > trying to > explain. I'll upload a CL next week to demo. Thanks! It would be nice to see real code but you don't have to if it is too much trouble. Maybe a little bit pseudo code is sufficient to help people understand the idea. > > > > (Sorry for raising questions after the CL has gone through many rounds of >> reviews. I just want to make sure I have understood the considerations >> behind >> the design.) >> > > Please, continue to comment. I want to get this right. > > https://codereview.chromium.org/1429553002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 20 November 2015 at 18:15, Yuzhu Shen <yzshen@chromium.org> wrote: > > > On Thu, Nov 19, 2015 at 9:16 PM, <amistry@chromium.org> wrote: > >> On 2015/11/19 17:05:42, yzshen1 wrote: >> >>> On 2015/11/19 07:51:26, Anand Mistry wrote: >>> > On 2015/11/19 06:52:25, yzshen1 wrote: >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... >> >>> > > File mojo/public/c/system/wait_set.h (right): >>> > > >>> > > >>> > >>> >> >> >> https://codereview.chromium.org/1429553002/diff/60001/mojo/public/c/system/wa... >> >>> > > mojo/public/c/system/wait_set.h:101: // subsequent calls to >>> > > |MojoGetReadyHandle()| will return the same handle. >>> > > On 2015/11/19 04:13:45, Anand Mistry wrote: >>> > > > Added a comment, and explanation inline. >>> > > > >>> > > > On 2015/11/18 23:50:58, yzshen1 wrote: >>> > > > > I wonder how the wait set will determine the order to return >>> ready >>> > handles. >>> > > > > (Maybe it is implementation related?) >>> > > > > >>> > > > > (1) For example, assume {a, b} are two handles that are ready in >>> the >>> wait >>> > > set. >>> > > > I >>> > > > > call MojoGetReadyHandle() and it returns |a|. The comment sounds >>> like >>> >> if >> >>> I >>> > > > don't >>> > > > > do something with |a| (say, draining the data or removing it >>> from the >>> > set), >>> > > > > subsequent calls will return |a| (instead of |b|) for sure. >>> > > > >>> > > > I picked this wording because it represents a worst-case scenario >>> that >>> >> the >> >>> > > user >>> > > > must handle. It's not strictly correct because the implementation >>> could >>> > > > round-robin, or randomly pick. But this isn't guaranteed by the >>> API. I >>> chose >>> > > not >>> > > > to have any guarantees here in order to allow an efficient >>> >> implementation, >> >>> > > > similar to how mutexes generally aren't fair. >>> > > > >>> > > > > >>> > > > > (2) The other question is about starvation: assume {a, b} are two >>> handles >>> > > that >>> > > > > are ready in the wait set. I call MojoGetReadyHandle() and it >>> returns >>> |a|. >>> > I >>> > > > > drain the data of |a|, process the data and then call >>> MojoGetReadyHandle() >>> > > > > again. In the mean time, there is new data available for |a|, so >>> |a| >>> >> is >> >>> > > ready >>> > > > > again. If this pattern repeats, is it possible that |b| is never >>> returned >>> > by >>> > > > > MojoGetReadyHandle() and "starved"? >>> > > > >>> > > > According to the API, yes (but in my current implementation, no, >>> because >>> >> I >> >>> > > > round-robin). I would argue that if you run into this situation, >>> you >>> >> have >> >>> > > bigger >>> > > > problems. I think that this behaviour is reasonable and the notion >>> of >>> > fairness >>> > > > here should be handled at a higher level. The wait set has no >>> notion of >>> what >>> > > the >>> > > > best way to handle this case is. It might be that you want >>> efficiency, >>> >> and >> >>> > > > repeatedly handling the same handle is most efficient. It could >>> also be >>> that >>> > > you >>> > > > want some other metric of fairness, such as distributing equal >>> bandwidth >>> > > between >>> > > > renderers as not to starve any particular page. Encoding any >>> notion of >>> > > fairness >>> > > > won't capture every case, and puts restrictions on the >>> implementation. >>> > > > >>> > > > That said, I'm willing to reconsider fairness as part of the API. >>> Note >>> that >>> > > > epoll doesn't guarantee any notion of fairness. And the specific >>> >> situation >> >>> > you >>> > > > called out would be handled by an epoll user by using >>> edge-triggering >>> >> and >> >>> > > > draining the ready queue each time before handling the each fd. >>> > > > >>> > > > > >>> > > > > It seems nice to add some comments to clarify. >>> > > > > >>> > > > >>> > > >>> > > Thanks for the detailed reply! >>> > > >>> > > I totally agree that "fairness" shouldn't be handled by this API >>> because >>> >> it >> >>> > > really depends on the user scenarios. I just think it is useful to >>> explicitly >>> > > comment about it. (Your updated comment looks good.) >>> > > >>> > > That being said, if the user wants to make scheduling decision >>> himself, >>> >> this >> >>> > API >>> > > seems a little inconvenient for the user to learn "the whole >>> picture", >>> >> i.e., >> >>> > all >>> > > the handles that are currently ready. >>> > >>> > I'd say that if you're interacting with this low-level API instead of >>> using >>> > something like AsyncWaiter or HandleWatcher, you should be prepared to >>> learn >>> the >>> > whole picture. >>> >> >> Exactly. That is what I was trying to say: users of the wait set probably >>> want >>> to learn the whole picture and make scheduling decision themselves. >>> However, >>> MojoGetReadyHandle() is a little inconvenient for that purpose. At the >>> first >>> glance, it seems the API has made the scheduling decision for its users, >>> >> because >> >>> it returns a single ready handle that it picks with unspecified, >>> >> impl-dependent >> >>> algorithm. >>> >> >> Hmm, I see your point. >> >> I have seen your comment about "returning a single handle should be okay" >>> in >>> reply #15. But I didn't see why this approach is better than returning an >>> >> array >> >>> of all ready handles. Is it because of performance? >>> >> >> No, not performance. I didn't mention this before, but I felt that if this >> returned >> multiple handles, there would be a mental disconnect between the API and >> the >> expectations of the user. Specifically, if you asked for 4 handles and >> this API >> returned 2, the user might assume that they need to wait. But due to >> implementation >> weirdness, this is not a correct assumption. Note that this isn't a >> problem, >> it's >> just unnecessary work. >> >> So really, it's just me. I can change it if you feel it's worth it. > > > Thanks for the reply! I feel that it would be more flexible if we support > returning multiple handles, and likely (hopefully?) won't complicate the > implementation. > > I imagine something like this: (The names, etc. haven't been carefully > considered. Just to show my thoughts.) > > struct ReadyHandleInfo { > MojoHandle handle; > MojoHandleSignalsState signals_state; > }; > > // |count| inputs the size of the |ready_handles| buffer; it outputs how > many handles were written into |ready_handles|. > // If |count| inputs 0, the function doesn't touch |ready_handles| but > outputs the total number of ready handles in |count|. So if you want, you > can call this function once to measure how big a buffer you should > allocate, and then call a second time with the buffer. > > MojoResult MojoGetReadyHandles( > MojoHandle wait_set_handle, > int32_t count, // Inout param. > ReadyHandleInfo* ready_handles); // Out param. > > The current function definition is equivalent to setting |count| to 1. > I'll give it a shot and see what it's like. > >> >> >> > >>> > > >>> > > It seems the user needs to: call MojoGetReadyHandle() and then >>> > > MojoRemoveWaiter() to remove the ready handle returned; repeat this >>> >> process >> >>> > > until no ready handle. And then add all those handles back to the >>> wait set >>> (if >>> > > he wants to continue watching). >>> > > >>> > > Is my understanding correct? >>> > >>> > Yes. However, two points: >>> > 1. This is basically how WaitMany functions internally, except it's >>> much >>> worse. >>> > And MessagePumpMojo already works like this. >>> >> >> Right. However, I think the main purpose is to look for ways to make >>> improvements, not following existing work. Right? >>> >> >> Yup. I'm just pointing out that it's not worse than the current state, and >> probably >> not a big problem in practice. > > > Agree that it is not worse than the current state. :) > >> >> >> >> > 2. The door to do something smarter is open. You could optimise your >>> message >>> > loop for the case where a handler will act on the ready handle (i.e. >>> drain a >>> > message pipe) and detect the case where it doesn't based on some >>> heuristic. >>> That >>> > way, most of the time, you don't have to pay any bookkeeping cost. >>> >> >> Would you please tell me more about the idea? I am not sure I understand >>> the >>> part of "detect the case where it doesn't based on some heuristic". >>> >> >> So, the idea is that in the common case, only one or two handles will be >> woken >> up. >> In that case, you could pull the ready handles one by one, and run the >> handler >> right >> after reading it using GetReadyHandle(). Right now, the primary user of >> MessagePumpMojo, >> HandleWatcher, always removes the handle from the message pump in the >> handler >> ( >> https://code.google.com/p/chromium/codesearch#chromium/src/mojo/message_pump/... >> ). >> A more efficient user would drain the (for example) message pipe in the >> message >> pump >> handler and leave the handle in the message pump. Your situation could be >> detected >> by counting the number of ready handles that are handled, and if it >> exceeds some >> threshold (say 2), then get and remove all the ready handles before >> servicing >> them. >> >> Hmm, maybe I should just implement this to show you. It would be better >> than >> trying to >> explain. I'll upload a CL next week to demo. > > > Thanks! It would be nice to see real code but you don't have to if it is > too much trouble. Maybe a little bit pseudo code is sufficient to help > people understand the idea. > So, the MessagePumpMojo change for Wait set is in https://codereview.chromium.org/1467953002/ (not ready for review). In it, I have a loop which pulls off N (where N == 3) ready handles. If it pulls that many, which should be relatively uncommon, you could fall back to the Get/Remove combination until all ready handles are processed, then add back the ones still being waited on. That would avoid starving handles, but keep the efficient behaviour for the common case. > > >> >> >> >> (Sorry for raising questions after the CL has gone through many rounds of >>> reviews. I just want to make sure I have understood the considerations >>> behind >>> the design.) >>> >> >> Please, continue to comment. I want to get this right. >> >> https://codereview.chromium.org/1429553002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:48: // It is safe to add a handle to a wait set while performing a wait on another Is it specified what happens when this is done? It seems a bit weird to have this after Returns. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:64: // It is safe to remove a handle from a wait set while performing a wait on Ditto. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:70: // Retreives a ready from |wait_set_handle|. A handle is ready if: Retrieves a ready handle from |wait_set_handle|. Please make it clear that any of the three items below being true causes a handle to be ready. It isn't immediately obvious. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:72: // - It becomes known that no signal for the handle will ever be satisfied. "It" refers to something different here compared to the other two items.
https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:50: MOJO_SYSTEM_EXPORT MojoResult MojoAddWaiter( naming nit (bikeshedding): I'm not sure the term "Waiter" makes that much sense here. A handle is not a waiter. MojoAddHandleToWaitSet would be clearer, but since we have MojoGetReadyHandle maybe we should just go with MojoAddHandle. We don't have an object-oriented naming convention for Mojo API since we don't expect a large API.
I'm still addressing the comments, but I've implemented this in the new EDK and run the page cycler against mandoline before and after waitset (newer run is waitset): https://googledrive.com/host/0B3QQn2XrgBSQYnVGbjBhakVOYmc It looks better, but I think there may be too much noise to tell.
> > I imagine something like this: (The names, etc. haven't been carefully > considered. Just to show my thoughts.) > > struct ReadyHandleInfo { > MojoHandle handle; > MojoHandleSignalsState signals_state; > }; > > // |count| inputs the size of the |ready_handles| buffer; it outputs how > many handles were written into |ready_handles|. > // If |count| inputs 0, the function doesn't touch |ready_handles| but > outputs the total number of ready handles in |count|. So if you want, you > can call this function once to measure how big a buffer you should > allocate, and then call a second time with the buffer. > > MojoResult MojoGetReadyHandles( > MojoHandle wait_set_handle, > int32_t count, // Inout param. > ReadyHandleInfo* ready_handles); // Out param. > > The current function definition is equivalent to setting |count| to 1. > So, I went ahead and implemented this to see what it's like. First, some hard numbers. With the oop-proxy-resolver enabled, I did a session restore of 14 random pages (loaded from slashdot) and recorded the number of ready handles in a local histogram (input count set at 100): Histogram: MessagePumpMojo.ReadyHandles recorded 3628 samples, average = 1.1 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (3420 = 94.3%) {0.0%} 2 ---O (152 = 4.2%) {94.3%} 3 -O (33 = 0.9%) {98.5%} 4 O (12 = 0.3%) {99.4%} 5 O (6 = 0.2%) {99.7%} 6 O (2 = 0.1%) {99.9%} 7 O (2 = 0.1%) {99.9%} 8 O (0 = 0.0%) {100.0%} 9 O (1 = 0.0%) {100.0%} Granted, the use of Mojo right now is quite small. So, I also loaded up mandoline with theverge.com and got the following from a single load + some scrolling: [1202/142147:ERROR:message_pump_mojo.cc(279)] mojo:desktop_ui : Histogram: MessagePumpMojo.ReadyHandles recorded 330 samples, average = 1.1 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (316 = 95.8%) {0.0%} 2 ---O (13 = 3.9%) {95.8%} 3 ... 6 O (1 = 0.3%) {99.7%} 7 ... [20831:20831:1202/142147:ERROR:message_pump_mojo.cc(279)] http://www.theverge.com/ : Histogram: MessagePumpMojo.ReadyHandles recorded 4730 samples, average = 1.1 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (4464 = 94.4%) {0.0%} 2 ---O (214 = 4.5%) {94.4%} 3 O (17 = 0.4%) {98.9%} 4 O (14 = 0.3%) {99.3%} 5 O (10 = 0.2%) {99.6%} 6 O (4 = 0.1%) {99.8%} 7 O (2 = 0.0%) {99.9%} 8 O (0 = 0.0%) {99.9%} 9 O (2 = 0.0%) {99.9%} 10 O (0 = 0.0%) {99.9%} 11 O (1 = 0.0%) {99.9%} 12 O (1 = 0.0%) {100.0%} 13 ... 15 O (1 = 0.0%) {100.0%} 16 ... [20849:20849:1202/142148:ERROR:message_pump_mojo.cc(279)] http://tpc.googlesyndication.com/safeframe/1-0-2/html/container.html : Histogram: MessagePumpMojo.ReadyHandles recorded 1344 samples, average = 1.0 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (1340 = 99.7%) {0.0%} 2 O (1 = 0.1%) {99.7%} 3 O (2 = 0.1%) {99.8%} 4 O (0 = 0.0%) {99.9%} 5 O (1 = 0.1%) {99.9%} 6 ... [20893:20893:1202/142148:ERROR:message_pump_mojo.cc(279)] https://s-static.ak.facebook.com/connect/xd_arbiter/TlA_zCeMkxl.js#channel=f1... : Histogram: MessagePumpMojo.ReadyHandles recorded 1095 samples, average = 1.0 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (1092 = 99.7%) {0.0%} 2 O (2 = 0.2%) {99.7%} 3 ... 5 O (1 = 0.1%) {99.9%} 6 ... [20831:20840:1202/142148:ERROR:message_pump_mojo.cc(279)] : Histogram: MessagePumpMojo.ReadyHandles recorded 4780 samples, average = 1.1 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (4509 = 94.3%) {0.0%} 2 ---O (217 = 4.5%) {94.3%} 3 O (17 = 0.4%) {98.9%} 4 O (16 = 0.3%) {99.2%} 5 O (10 = 0.2%) {99.6%} 6 O (4 = 0.1%) {99.8%} 7 O (2 = 0.0%) {99.9%} 8 O (0 = 0.0%) {99.9%} 9 O (2 = 0.0%) {99.9%} 10 O (0 = 0.0%) {99.9%} 11 O (1 = 0.0%) {99.9%} 12 O (1 = 0.0%) {100.0%} 13 ... 15 O (1 = 0.0%) {100.0%} 16 ... [20810:20824:1202/142148:ERROR:message_pump_mojo.cc(279)] mojo://filesystem/ : Histogram: MessagePumpMojo.ReadyHandles recorded 958 samples, average = 1.1 0 O (0 = 0.0%) 1 ------------------------------------------------------------------------O (908 = 94.8%) {0.0%} 2 ----O (50 = 5.2%) {94.8%} 3 ... The vast majority of the time, the number of ready handles is small, with the average being around 1. I bring this up to try and quantify the benefits of returning multiple handles. Implementation complexity is a non-issue in my opinion. It's small, and not worth considering. Implementation side-effects are a slight concern. By necessity, you need to do at least one array alloc/free + element_construction_cost if you are to return multiple handles. This is because the API allows an arbitrary number of handle to be returned, and so the implementation needs to do some variable-length bookkeeping internally, even though only one handle might be returned. I can avoid the allocations in the case where the number of handles requested is small (<16) by using StackContainer<>s, but you're still paying some cost. Even a single site like the verge generates thousands of message loops iterations, so I'd like to keep the cost as small as possible. The API surface increases from 3 arguments, to 5. You need both a |*count| and a |*results| since each handle has their own result. And for consistency with other Mojo core APIs, these are separate arguments. I'm quite opposed to the effect of returning the number of ready handles if the input count is 0, because: 1. It's only a hint, and potentially not a very good one. The actual value may be wildly off when you get around to retrieving the handles. 2. It implies you want to do an allocation, which is probably a good idea to avoid in the places you intend to use this. 3. It's special casing the meaning of the function depending on the arguments passed. My position on this is mostly neutral, slightly leaning towards only returning a single handle because of the (slight) runtime cost. But if you're willing to make an executive decision, I'll follow it.
Thanks for getting the numbers and giving detailed explanation of your idea! As you said, the current use of Mojo is small. Even with Mandoline, only the most basic functionality is implemented. The vast majority of features are not supported yet. Eventually the number of ready handles at any given time may be bigger. But we don't know for sure at the moment so I don't think I can make a strong argument about that. :) What I am not super satisfied is that the API is a little inconsistent conceptually: it watches multiple handles, but it doesn't have a convenient way to report multiple ready handles. The only way to do it (without relying on unspecified implementation details) is to get a ready handle, and then remove it from the set, and get the next ready handle... Another idea is to make GetReadyHandle() "iterates" over the ready handles. It also seems more consistent to me. (I understand that the exact meaning of "iterate" here may need some thoughts because handle signal state may change at any time.) I think I have fully made my point. I will leave this decision to you and other reviewers. Thanks!
On 2015/12/02 05:25:52, yzshen1 wrote: > Thanks for getting the numbers and giving detailed explanation of your idea! > > As you said, the current use of Mojo is small. Even with Mandoline, only the > most basic functionality is implemented. The vast majority of features are not > supported yet. > Eventually the number of ready handles at any given time may be bigger. But we > don't know for sure at the moment so I don't think I can make a strong argument > about that. :) > > What I am not super satisfied is that the API is a little inconsistent > conceptually: it watches multiple handles, but it doesn't have a convenient way > to report multiple ready handles. The only way to do it (without relying on > unspecified implementation details) is to get a ready handle, and then remove it > from the set, and get the next ready handle... > > Another idea is to make GetReadyHandle() "iterates" over the ready handles. It > also seems more consistent to me. (I understand that the exact meaning of > "iterate" here may need some thoughts because handle signal state may change at > any time.) > > I think I have fully made my point. I will leave this decision to you and other > reviewers. Thanks! LGTM
On 2015/12/02 05:25:52, yzshen1 wrote: > Thanks for getting the numbers and giving detailed explanation of your idea! > > As you said, the current use of Mojo is small. Even with Mandoline, only the > most basic functionality is implemented. The vast majority of features are not > supported yet. > Eventually the number of ready handles at any given time may be bigger. But we > don't know for sure at the moment so I don't think I can make a strong argument > about that. :) > > What I am not super satisfied is that the API is a little inconsistent > conceptually: it watches multiple handles, but it doesn't have a convenient way > to report multiple ready handles. The only way to do it (without relying on > unspecified implementation details) is to get a ready handle, and then remove it > from the set, and get the next ready handle... Thank you. I think the conceptual consistency of the API is a good rationale, given all the implementation and performance issues are fairly equivalent. I'll make the change. > Another idea is to make GetReadyHandle() "iterates" over the ready handles. It > also seems more consistent to me. (I understand that the exact meaning of > "iterate" here may need some thoughts because handle signal state may change at > any time.) > > I think I have fully made my point. I will leave this decision to you and other > reviewers. Thanks!
Ok, I think there's been enough bikesheading, so I'm calling it. If there are no significant objections, I'm submitting this. So speak now or forever hold your peace. Where "forever" is defined as "until the next cl". https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... File mojo/public/c/system/wait_set.h (right): https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:48: // It is safe to add a handle to a wait set while performing a wait on another On 2015/11/23 06:51:57, Sam McNally wrote: > Is it specified what happens when this is done? I thought that would be obvious. If the other thread is waiting and the handle is ready, it would wake up that other thread. Otherwise, it would just get added to the set and when woken, would terminate the wait. > It seems a bit weird to have this after Returns. It seemed to be the convention to put clarifying notes after the returns. But other than TODOs, only the MojoWait() definition does this, so I've moved it to before. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:50: MOJO_SYSTEM_EXPORT MojoResult MojoAddWaiter( On 2015/11/23 22:19:01, darin (slow to review) wrote: > naming nit (bikeshedding): I'm not sure the term "Waiter" makes that much sense > here. A handle is not a waiter. MojoAddHandleToWaitSet would be clearer, but > since we have MojoGetReadyHandle maybe we should just go with MojoAddHandle. We > don't have an object-oriented naming convention for Mojo API since we don't > expect a large API. Done. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:64: // It is safe to remove a handle from a wait set while performing a wait on On 2015/11/23 06:51:56, Sam McNally wrote: > Ditto. Done. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:70: // Retreives a ready from |wait_set_handle|. A handle is ready if: On 2015/11/23 06:51:56, Sam McNally wrote: > Retrieves a ready handle from |wait_set_handle|. > > Please make it clear that any of the three items below being true causes a > handle to be ready. It isn't immediately obvious. Done. https://codereview.chromium.org/1429553002/diff/80001/mojo/public/c/system/wa... mojo/public/c/system/wait_set.h:72: // - It becomes known that no signal for the handle will ever be satisfied. On 2015/11/23 06:51:57, Sam McNally wrote: > "It" refers to something different here compared to the other two items. Done... I think...
The CQ bit was checked by amistry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429553002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429553002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amistry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/1429553002/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429553002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429553002/160001
Message was sent while issue was closed.
Description was changed from ========== API definition for WaitSet. BUG=556865 ========== to ========== API definition for WaitSet. BUG=556865 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== API definition for WaitSet. BUG=556865 ========== to ========== API definition for WaitSet. BUG=556865 Committed: https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de Cr-Commit-Position: refs/heads/master@{#363914} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de Cr-Commit-Position: refs/heads/master@{#363914} |