API definition for WaitSet.

API definition for WaitSet.

BUG=556865
Committed: https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de
Cr-Commit-Position: refs/heads/master@{#363914}

[Anand Mistry (off Chromium), 2015-10-26 23:50:25]

amistry@chromium.org changed reviewers:
+ jam@chromium.org, rockot@chromium.org, sammc@chromium.org, yzshen@chromium.org

[Anand Mistry (off Chromium), 2015-10-26 23:50:25]

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.

[Anand Mistry (off Chromium), 2015-10-26 23:55:25]

Oh yeah, and problem #6, naming.

[darin (slow to review), 2015-10-26 23:57:30]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2015-10-26 23:57:30]

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.

[darin (slow to review), 2015-10-26 23:58:40]

Actually, if you do reuse MojoHandle, then you can reuse MojoClose and
ScopedHandle. Hmmm...

[Ken Rockot(use gerrit already), 2015-10-28 21:18:03]

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.

[Anand Mistry (off Chromium), 2015-10-29 06:36:25]

> 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.

[darin (slow to review), 2015-10-30 01:50:09]

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.

[Anand Mistry (off Chromium), 2015-10-30 03:38:07]

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.

[Ken Rockot(use gerrit already), 2015-11-09 20:48:41]

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.
>

[darin (slow to review), 2015-11-09 21:04:43]

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

[Ken Rockot(use gerrit already), 2015-11-09 21:16:45]

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

[darin (slow to review), 2015-11-09 21:47:00]

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.

[Anand Mistry (off Chromium), 2015-11-13 00:21:30]

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.

[darin (slow to review), 2015-11-13 05:12:51]

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.

[Anand Mistry (off Chromium), 2015-11-17 01:42:15]

reviewers: Ping! Despite the lack of implementation here, I still want to submit
this.

[Ken Rockot(use gerrit already), 2015-11-17 01:50:24]

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).

[Anand Mistry (off Chromium), 2015-11-17 02:55:20]

Description was changed from

==========
API definition for WaitSet.
==========

to

==========
API definition for WaitSet.

BUG=556865
==========

[Anand Mistry (off Chromium), 2015-11-17 03:11:32]

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.

[Anand Mistry (off Chromium), 2015-11-18 22:25:55]

jam, yzshen, sammc: Ping! Please review for submit.

[yzshen1, 2015-11-18 23:50:58]

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)

[Anand Mistry (off Chromium), 2015-11-19 04:13:45]

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.

[yzshen1, 2015-11-19 06:52:25]

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?

[Anand Mistry (off Chromium), 2015-11-19 07:51:26]

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.

[yzshen1, 2015-11-19 17:05:42]

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.)

[Anand Mistry (off Chromium), 2015-11-20 05:16:57]

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.

[yzshen1, 2015-11-20 07:16:05]

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.

[Anand Mistry (off Chromium), 2015-11-23 05:42:03]

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.

[Sam McNally, 2015-11-23 06:51:57]

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.

[darin (slow to review), 2015-11-23 22:19:01]

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.

[Anand Mistry (off Chromium), 2015-11-25 03:32:30]

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.

[Anand Mistry (off Chromium), 2015-12-02 03:47:40]

> 
> 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.

[yzshen1, 2015-12-02 05:25:52]

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!

[yzshen1, 2015-12-02 05:33:35]

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

[Anand Mistry (off Chromium), 2015-12-02 06:38:02]

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!

[Anand Mistry (off Chromium), 2015-12-03 00:47:13]

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...

[Anand Mistry (off Chromium), 2015-12-09 00:02:51]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 00:14:48]

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

[commit-bot: I haz the power, 2015-12-09 01:42:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 01:42:46]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2015-12-09 02:02:32]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-12-09 02:02:32]

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.")

[commit-bot: I haz the power, 2015-12-09 02:05:57]

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

[commit-bot: I haz the power, 2015-12-09 02:16:54]

Description was changed from

==========
API definition for WaitSet.

BUG=556865
==========

to

==========
API definition for WaitSet.

BUG=556865
==========

[commit-bot: I haz the power, 2015-12-09 02:16:55]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-09 02:18:07]

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}
==========

[commit-bot: I haz the power, 2015-12-09 02:18:09]

Patchset 9 (id:??) landed as
https://crrev.com/81f7f78077b60aef4ce6ea27d1202852952830de
Cr-Commit-Position: refs/heads/master@{#363914}