Adding TryPostTask to the message loop

Adding TryPostTask to the message loop

This is to allow code in certain high priority threads
to use the task/message loop facility. For example the
audio threads that currently use sentinel booleans and
recurring timers.

TryPostTask will fail if it is going contend for
the lock. The caller must handle that case.

Note that the current tests do not test the contended
mode. I don't see a way to test that case without
introducing runtime penalty for all of chrome.

BUG=none
TEST=included

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

[cpu_(ooo_6.6-7.5), 2013-04-20 18:39:50]

as per our conversation.

[jar (doing other things), 2013-04-21 15:29:34]

Personal preference is that I'd rather keep as much locking logic in the
AutoLock class (and away from callers), since that is the essential reason for
having it.

I made an alternate suggestion for an API below.  WDYT?

https://codereview.chromium.org/14387002/diff/4001/base/message_loop.cc
File base/message_loop.cc (right):

https://codereview.chromium.org/14387002/diff/4001/base/message_loop.cc#newco...
base/message_loop.cc:613: AutoLock locked(incoming_queue_lock_,
AutoLock::NoAcquire());
Interesting API.  Did you consider having a second AutoLock constructor
supporting something like:
AutoLock locked(incomming_queue_lock, use_try_lock);
if (use_try_lock && locked.try_failed())
  return false;

You could even assert (in the AutoLock class) that if they pass in a try lock,
then they *must* test it via the accessor prior to destruction.

https://codereview.chromium.org/14387002/diff/4001/base/synchronization/lock.h
File base/synchronization/lock.h (right):

https://codereview.chromium.org/14387002/diff/4001/base/synchronization/lock....
base/synchronization/lock.h:106: }
If you stick with this API... I'd suggest:

lock_.AssertAcquired();

I guess it is validated at destruction time, but it is good to document the
intent, and catch the problem ASAP.

[cpu_(ooo_6.6-7.5), 2013-04-22 23:30:59]

https://codereview.chromium.org/14387002/diff/4001/base/message_loop.cc
File base/message_loop.cc (right):

https://codereview.chromium.org/14387002/diff/4001/base/message_loop.cc#newco...
base/message_loop.cc:613: AutoLock locked(incoming_queue_lock_,
AutoLock::NoAcquire());
On 2013/04/21 15:29:35, jar wrote:
> Interesting API.  Did you consider having a second AutoLock constructor
> supporting something like:
> AutoLock locked(incomming_queue_lock, use_try_lock);
> if (use_try_lock && locked.try_failed())
>   return false;
> 
> You could even assert (in the AutoLock class) that if they pass in a try lock,
> then they *must* test it via the accessor prior to destruction.

Yeah, the main drawback is having to add state to the autolock class, so
everybody using AutoLock pays but possibly only one user in the entire chrome
codebase uses the state.

That and there is a movement away from booleans on the interface to more
descriptive types, which also allows static type dispatch as we don't evaluate
use_try_lock again inside the autolock ctor.

I realize it looks a bit ugly but I feel the ugliness is contained in a very
small piece of code.

[cpu_(ooo_6.6-7.5), 2013-04-23 17:47:34]

forgot about the lock_.AssertAcquired(); sounds good. I'll add it.

[cpu_(ooo_6.6-7.5), 2013-04-24 16:48:59]

assert added PTAL.

[miu, 2013-04-26 07:27:00]

lgtm

Thanks for doing this, cpu!  :)  It'll feel great to rip out the extra
copy-and-poll scheme I've been using for silence detection.  Also, there are
other efforts across Chrome audio/video internals that should benefit from this.

[cpu_(ooo_6.6-7.5), 2013-04-26 20:02:15]

jar, ping

[jar (doing other things), 2013-04-26 21:59:43]

I still think this would be better done in the auto_lock class using state there
(as I argued, the cost of a bool in a stack frame is no big deal... especially
when the frame is often near the extreme of the stack.

Here are some nits.... but I want to ponder how to do this better (in
auto_lock).

https://codereview.chromium.org/14387002/diff/4001/base/message_loop.cc
File base/message_loop.cc (right):

https://codereview.chromium.org/14387002/diff/4001/base/message_loop.cc#newco...
base/message_loop.cc:281: const tracked_objects::Location& from_here, const
Closure& task) {
nit: one arg per line if you need to wrap.

You could also fix line 273 where this was perchance copied from.

https://codereview.chromium.org/14387002/diff/4001/base/message_loop_unittest.cc
File base/message_loop_unittest.cc (right):

https://codereview.chromium.org/14387002/diff/4001/base/message_loop_unittest...
base/message_loop_unittest.cc:97:
EXPECT_TRUE(MessageLoop::current()->TryPostTask(FROM_HERE, Bind(
You should probably add a test that holds the lock ahead of time, and expects a
false return from a try post.

I wonder if anyone else even uses try()?

https://codereview.chromium.org/14387002/diff/4001/base/synchronization/lock.h
File base/synchronization/lock.h (right):

https://codereview.chromium.org/14387002/diff/4001/base/synchronization/lock....
base/synchronization/lock.h:99: struct NoAcquire {};
I have a habit (religion?) of avoiding a negative in a flag, and perhaps this
would be better as:
AlreadyAcquired()

[jar (doing other things), 2013-04-29 17:27:18]

I still think it is an ugly approach, but at least it is not that big.

LGTM

p.s., If this does not get used.... I'd argue for removing it.

https://codereview.chromium.org/14387002/diff/18001/base/message_loop.h
File base/message_loop.h (right):

https://codereview.chromium.org/14387002/diff/18001/base/message_loop.h#newco...
base/message_loop.h:452: // return value.
Reading the whole comment, starting on line 443, perhaps you should re-write
from there (since line 443 is optionally untrue now).  Better might be:

This function attempts to add pending task to our incoming_queue_. The append
can only possibly fail when |use_try_lock| is true.

When |use_try_lock| is true, then this call will avoid blocking if the related
lock is already held, and will in that case (when the lock is contended) fail to
perform the append, and will return false.

If the call succeeds to append to the queue, then this call will return true.

In all cases, the caller retains ownership of |pending_task|, but this function
will reset the value of pending_task->task.  This is needed to ensure that the
posting call stack does not retain pending_task->task beyond this function call.

[cpu_(ooo_6.6-7.5), 2013-04-30 05:26:04]

I agree, it it does not get used by the m29 I'll remove it.

https://codereview.chromium.org/14387002/diff/18001/base/message_loop.h
File base/message_loop.h (right):

https://codereview.chromium.org/14387002/diff/18001/base/message_loop.h#newco...
base/message_loop.h:452: // return value.
On 2013/04/29 17:27:18, jar wrote:
> Reading the whole comment, starting on line 443, perhaps you should re-write
> from there (since line 443 is optionally untrue now).  Better might be:
> 
> This function attempts to add pending task to our incoming_queue_. The append
> can only possibly fail when |use_try_lock| is true.
> 
> When |use_try_lock| is true, then this call will avoid blocking if the related
> lock is already held, and will in that case (when the lock is contended) fail
to
> perform the append, and will return false.
> 
> If the call succeeds to append to the queue, then this call will return true.
> 
> In all cases, the caller retains ownership of |pending_task|, but this
function
> will reset the value of pending_task->task.  This is needed to ensure that the
> posting call stack does not retain pending_task->task beyond this function
call.

Done.

[commit-bot: I haz the power, 2013-04-30 05:26:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cpu@chromium.org/14387002/23001

[commit-bot: I haz the power, 2013-04-30 08:54:18]

Change committed as 197315