Mojo EDK: Circulate MojoArmWatcher outputs to avoid starvation

Mojo EDK: Circulate MojoArmWatcher outputs to avoid starvation

Changes the implementation of MojoArmWatcher slightly so that
the set of ready handles is circulated across consecutive calls,
avoiding potential handle starvation by callers requesting a
small fixed number of outputs each time.

Also correctly iterates over only the ready handles in
WatcherDispatcher::Arm(), rather than less efficiently iterating
over all watched handles. (Ooops!)

BUG=700171
R=yzshen@chromium.org

Review-Url: https://codereview.chromium.org/2754083003
Cr-Commit-Position: refs/heads/master@{#457557}
Committed: https://chromium.googlesource.com/chromium/src/+/9d95f9e5de8e1e95beaaead0bbaad3fbfdc84e15

[Ken Rockot(use gerrit already), 2017-03-16 17:44:10]

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

[commit-bot: I haz the power, 2017-03-16 17:44:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-03-16 18:24:04]

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

[commit-bot: I haz the power, 2017-03-16 18:24:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-03-16 18:59:01]

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
mojo/edk/system/watcher_dispatcher.cc:228: if (!watch->ready())
[A question to help my own understanding]
Why do we need this check? Because we don't guarantee the user that when the
notification arrives, the signals are still accurate, is it okay to deliver the
state without checking?

If that works, this code could be simplified by setting the correct
max_ready_contexts and removing first_ready_iter/exhausted, I think.

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
File mojo/edk/system/watcher_dispatcher.h (right):

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
mojo/edk/system/watcher_dispatcher.h:90: Watch* last_watch_to_block_arming_ =
nullptr;
Please comment that this pointer may not refer to a valid object and therefore
should never be de-refed. Or maybe change it to uintptr_t.

[Ken Rockot(use gerrit already), 2017-03-16 19:20:11]

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

[commit-bot: I haz the power, 2017-03-16 19:20:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-03-16 19:21:11]

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
mojo/edk/system/watcher_dispatcher.cc:228: if (!watch->ready())
On 2017/03/16 at 18:59:01, yzshen1 wrote:
> [A question to help my own understanding]
> Why do we need this check? Because we don't guarantee the user that when the
notification arrives, the signals are still accurate, is it okay to deliver the
state without checking?
> 
> If that works, this code could be simplified by setting the correct
max_ready_contexts and removing first_ready_iter/exhausted, I think.

Very good point! This test was only here because the iteration was happening
over all handles. It is guaranteed that the set of ready_watches_ is equal to
the set of watched_handles_ values for which ready() is true, so this is now
unnecessary. This simplifies things quite a bit.

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
File mojo/edk/system/watcher_dispatcher.h (right):

https://codereview.chromium.org/2754083003/diff/20001/mojo/edk/system/watcher...
mojo/edk/system/watcher_dispatcher.h:90: Watch* last_watch_to_block_arming_ =
nullptr;
On 2017/03/16 at 18:59:01, yzshen1 wrote:
> Please comment that this pointer may not refer to a valid object and therefore
should never be de-refed. Or maybe change it to uintptr_t.

Done

[commit-bot: I haz the power, 2017-03-16 20:43:46]

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

[commit-bot: I haz the power, 2017-03-16 20:43:47]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-03-16 21:06:17]

LGTM with one nit.

Thanks!

https://codereview.chromium.org/2754083003/diff/40001/mojo/edk/system/watcher...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2754083003/diff/40001/mojo/edk/system/watcher...
mojo/edk/system/watcher_dispatcher.cc:219: last_watch_to_block_arming_ = watch;
nit: it might further simplify the code by:
============================
Calculate |last_ready_iter| from |last_watch_to_block_arming_|.

for (...) {
  Calculate next and assign it to |last_read_iter|.
  Assign to output param.
}

Update |last_watch_to_block_arming_| from |last_read_iter|.

[Ken Rockot(use gerrit already), 2017-03-16 21:17:35]

https://codereview.chromium.org/2754083003/diff/40001/mojo/edk/system/watcher...
File mojo/edk/system/watcher_dispatcher.cc (right):

https://codereview.chromium.org/2754083003/diff/40001/mojo/edk/system/watcher...
mojo/edk/system/watcher_dispatcher.cc:219: last_watch_to_block_arming_ = watch;
On 2017/03/16 at 21:06:17, yzshen1 wrote:
> nit: it might further simplify the code by:
> ============================
> Calculate |last_ready_iter| from |last_watch_to_block_arming_|.
> 
> for (...) {
>   Calculate next and assign it to |last_read_iter|.
>   Assign to output param.
> }
> 
> Update |last_watch_to_block_arming_| from |last_read_iter|.

It seems like a simplification at first, but in practice I think it's kind of a
wash either way.

The issue is that last_watch_to_block_arming_ may be invalid, and in that case
we don't really have a valid notion of a "last ready" iterator.

We could in that case initialize it to end() before entering the loop, but then
the loop logic gets gnarly. You could start the loop by resetting to begin() if
at end(), but normally you want to start the loop by iterating (and potentially
wrapping around) - so what you end up with would look something like:

for (...) {
  if (iter == end()) {
    iter = begin()
  } else {
    ++iter;
    if (iter == end())
      iter = begin();
  }
}

or

you could default to begin() when there's no last_watch:

if (last_watch) { ... }
else iter = begin();

for (...) {
  ++iter;
  if (iter == end())
    iter = begin();
  // ...
}

which looks much nicer, but can incorrectly skip the first entry in the
collection, so it's not correct.

For now I will land as-is and maybe we can talk about a cleaner way to do this
if I've missed something. Thanks!

[Ken Rockot(use gerrit already), 2017-03-16 21:17:45]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2017-03-16 21:18:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-03-16 21:21:35]

On 2017/03/16 21:17:35, Ken Rockot wrote:
>
https://codereview.chromium.org/2754083003/diff/40001/mojo/edk/system/watcher...
> File mojo/edk/system/watcher_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2754083003/diff/40001/mojo/edk/system/watcher...
> mojo/edk/system/watcher_dispatcher.cc:219: last_watch_to_block_arming_ =
watch;
> On 2017/03/16 at 21:06:17, yzshen1 wrote:
> > nit: it might further simplify the code by:
> > ============================
> > Calculate |last_ready_iter| from |last_watch_to_block_arming_|.
> > 
> > for (...) {
> >   Calculate next and assign it to |last_read_iter|.
> >   Assign to output param.
> > }
> > 
> > Update |last_watch_to_block_arming_| from |last_read_iter|.
> 
> It seems like a simplification at first, but in practice I think it's kind of
a
> wash either way.
> 
> The issue is that last_watch_to_block_arming_ may be invalid, and in that case
> we don't really have a valid notion of a "last ready" iterator.
> 
> We could in that case initialize it to end() before entering the loop, but
then
> the loop logic gets gnarly. You could start the loop by resetting to begin()
if
> at end(), but normally you want to start the loop by iterating (and
potentially
> wrapping around) - so what you end up with would look something like:
> 
> for (...) {
>   if (iter == end()) {
>     iter = begin()
>   } else {
>     ++iter;
>     if (iter == end())
>       iter = begin();
>   }
> }
> 
> or
> 
> you could default to begin() when there's no last_watch:
> 
> if (last_watch) { ... }
> else iter = begin();
> 
> for (...) {
>   ++iter;
>   if (iter == end())
>     iter = begin();
>   // ...
> }
> 
> which looks much nicer, but can incorrectly skip the first entry in the
> collection, so it's not correct.
> 
> For now I will land as-is and maybe we can talk about a cleaner way to do this
> if I've missed something. Thanks!

Thanks for the explanation! You are right. LGTM++

[commit-bot: I haz the power, 2017-03-16 21:23:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489699065283730,
"parent_rev": "0e43eca719311f3a873d5c8bc644a77465450e21", "commit_rev":
"9d95f9e5de8e1e95beaaead0bbaad3fbfdc84e15"}

[commit-bot: I haz the power, 2017-03-16 21:24:32]

Description was changed from

==========
Mojo EDK: Circulate MojoArmWatcher outputs to avoid starvation

Changes the implementation of MojoArmWatcher slightly so that
the set of ready handles is circulated across consecutive calls,
avoiding potential handle starvation by callers requesting a
small fixed number of outputs each time.

Also correctly iterates over only the ready handles in
WatcherDispatcher::Arm(), rather than less efficiently iterating
over all watched handles. (Ooops!)

BUG=700171
R=yzshen@chromium.org
==========

to

==========
Mojo EDK: Circulate MojoArmWatcher outputs to avoid starvation

Changes the implementation of MojoArmWatcher slightly so that
the set of ready handles is circulated across consecutive calls,
avoiding potential handle starvation by callers requesting a
small fixed number of outputs each time.

Also correctly iterates over only the ready handles in
WatcherDispatcher::Arm(), rather than less efficiently iterating
over all watched handles. (Ooops!)

BUG=700171
R=yzshen@chromium.org

Review-Url: https://codereview.chromium.org/2754083003
Cr-Commit-Position: refs/heads/master@{#457557}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d95f9e5de8e1e95beaaead0bbaa...
==========

[commit-bot: I haz the power, 2017-03-16 21:24:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9d95f9e5de8e1e95beaaead0bbaa...