Performance tuning of HandleWatcher

Performance tuning of HandleWatcher

Couple of tweaks:
. make WatcherThreadManager maintain a queue of requests and process
  them at once.
. Makes State not unnecessarily cancel requests if it was told the
  handle is ready.

BUG=none
TEST=none
R=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291436

[darin (slow to review), 2014-08-21 23:55:12]

Is it a very common pattern that we start watching a handle and then immediately
stop watching it?

https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:74: // Cancels a previously schedule request to
start a watch.
nit: schedule -> scheduled

https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:347: if (!got_ready_)
isn't there a race condition here? what if StartWatching got far enough into
doing its work but not quite to the point of running the callback?

also, it seems like it would be cleaner if every call to StartWatching had to be
paired with a call to StopWatching, rather than trying to observe cases where
StopWatching may not be necessary.

[sky, 2014-08-22 00:03:51]

I don't think it's a common pattern anymore that we start/stop repeatedly. I
could certainly see multiple HandleWatchers getting created before the
background thread wakes up.

https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
File mojo/common/handle_watcher.cc (right):

https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:74: // Cancels a previously schedule request to
start a watch.
On 2014/08/21 23:55:12, darin wrote:
> nit: schedule -> scheduled

Done.

https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
mojo/common/handle_watcher.cc:347: if (!got_ready_)
On 2014/08/21 23:55:12, darin wrote:
> isn't there a race condition here? what if StartWatching got far enough into
> doing its work but not quite to the point of running the callback?

Then got_ready_ would be false, right?

> also, it seems like it would be cleaner if every call to StartWatching had to
be
> paired with a call to StopWatching, rather than trying to observe cases where
> StopWatching may not be necessary.

That's effectively what got_ready_ is doing. I don't think it makes sense to
track that in WatcherBackend as it would mean an additional thread hop is
necessary every time State is destroyed. I think it's also nice that
WatcherBackend (like HandleWatcher) can assume once it's notified the caller
that it removes internal state.

[darin (slow to review), 2014-08-22 00:06:55]

On 2014/08/22 00:03:51, sky wrote:
> I don't think it's a common pattern anymore that we start/stop repeatedly. I
> could certainly see multiple HandleWatchers getting created before the
> background thread wakes up.
> 
>
https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
> File mojo/common/handle_watcher.cc (right):
> 
>
https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
> mojo/common/handle_watcher.cc:74: // Cancels a previously schedule request to
> start a watch.
> On 2014/08/21 23:55:12, darin wrote:
> > nit: schedule -> scheduled
> 
> Done.
> 
>
https://codereview.chromium.org/480293004/diff/20001/mojo/common/handle_watch...
> mojo/common/handle_watcher.cc:347: if (!got_ready_)
> On 2014/08/21 23:55:12, darin wrote:
> > isn't there a race condition here? what if StartWatching got far enough into
> > doing its work but not quite to the point of running the callback?
> 
> Then got_ready_ would be false, right?

Right, and we would fail to call StopWatching. No one would ever call
StopWatching.
Then, we could end up in a situation where the handle is closed but the
background
thread is still stuck inside MojoWait, right?


> 
> > also, it seems like it would be cleaner if every call to StartWatching had
to
> be
> > paired with a call to StopWatching, rather than trying to observe cases
where
> > StopWatching may not be necessary.
> 
> That's effectively what got_ready_ is doing. I don't think it makes sense to
> track that in WatcherBackend as it would mean an additional thread hop is
> necessary every time State is destroyed. I think it's also nice that
> WatcherBackend (like HandleWatcher) can assume once it's notified the caller
> that it removes internal state.

[darin (slow to review), 2014-08-22 00:10:37]

LGTM w/ the additional comment we discussed.

[sky, 2014-08-22 00:14:26]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-08-22 00:20:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/480293004/60001

[commit-bot: I haz the power, 2014-08-22 01:20:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 02:19:05]

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

[commit-bot: I haz the power, 2014-08-22 02:19:06]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sky, 2014-08-22 02:35:13]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-08-22 02:36:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/480293004/60001

[commit-bot: I haz the power, 2014-08-22 03:30:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 04:26:55]

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

[commit-bot: I haz the power, 2014-08-22 04:26:56]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sky, 2014-08-22 14:00:37]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-08-22 14:01:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/480293004/60001

[commit-bot: I haz the power, 2014-08-22 14:55:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 15:53:00]

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

[commit-bot: I haz the power, 2014-08-22 15:53:01]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sky, 2014-08-22 16:17:55]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-08-22 16:18:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sky@chromium.org/480293004/60001

[commit-bot: I haz the power, 2014-08-22 17:11:58]

Committed patchset #4 (60001) as 291436