Avoid holding |incoming_queue_lock_| while waking up the message loop.

Avoid holding |incoming_queue_lock_| while waking up the message loop.

When constraining chrome to a limited number of cores (read: 2), it
appears that the write syscall that wakes up the message loop thread
may take a significant amount of time (>80ms) despite being a non-blocking
write. During that time, if |incoming_queue_lock_| is being held, any other
thread that tried to post a task to that message queue will be blocked in
trying to acquire that lock.

Committed: https://crrev.com/7fa6701bc5183bd5a73203d4fe1309f75ccfd5b4
Cr-Commit-Position: refs/heads/master@{#398458}

[Anand Mistry (off Chromium), 2016-05-19 01:58:58]

amistry@chromium.org changed reviewers:
+ danakj@chromium.org

[Anand Mistry (off Chromium), 2016-05-19 01:58:58]

This is the primary motivation for doing
https://codereview.chromium.org/1988563002/

[danakj, 2016-05-19 20:32:11]

I just got this in my inbox I think overnight. Sorry for being slow I see you
sent this a while ago, will read things.

[danakj, 2016-05-19 21:27:43]

What would change if you just used two base::Locks instead of this new
construct?

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.h:81: // task a being posted.
is being

[danakj, 2016-05-19 21:28:37]

Also, is there a way to make this diff smaller? I feel like you moved some code
around and tried to clean things up a bit - which is good. But I'd prefer if you
did that in a separate patch, or maybe an additional patch set once we agree on
the technical changes. Changing MessageLoop is scary and I don't know the code
well yet.

[danakj, 2016-05-19 22:29:37]

Ah.. ok I think I've stared at this enough to answer my previous questions.

Using a 2nd lock would be insufficient because you need to hold it both around
ScheduleWork() and around the incoming_queue_lock block. So you really do need a
RW lock.

I do have a few nits and implementation questions. This seems like a good change
to me.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:131: AutoReadLock
lock(message_loop_lock_);
nit: name "hold_message_loop"

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:132: DCHECK(message_loop_);
This method is called from the MessageLoop which is what lets you DCHECK this
right? So why bother holding the read lock at all? It can't go away inside here.
You could just document this fact?

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:133: message_loop_->ScheduleWork();
How come this doesn't set message_loop_scheduled_ = true anymore?

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:147: // Ensures |message_loop_| isn't
null'd while running.
"nulled". tho it's more like ensures the message loop isn't destroyed right? cuz
once it's null here it will also no longer exist.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:148: AutoReadLock
ml_lock(message_loop_lock_);
nit: name this variable "hold_message_loop"

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:157: AutoLock
locked(incoming_queue_lock_);
nit: name this variable "hold"

[Anand Mistry (off Chromium), 2016-05-20 04:45:38]

I've removed one piece of refactoring, but the rest is necessary due to locking.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:131: AutoReadLock
lock(message_loop_lock_);
On 2016/05/19 22:29:37, danakj wrote:
> nit: name "hold_message_loop"

Removed as per comment below.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:132: DCHECK(message_loop_);
On 2016/05/19 22:29:37, danakj wrote:
> This method is called from the MessageLoop which is what lets you DCHECK this
> right? So why bother holding the read lock at all? It can't go away inside
here.
> You could just document this fact?

Done. Thinking about it a bit more, calling ScheduleWork() here is completely
unnecessary because this function is called before the MessageLoop starts
running, and MessageLoop checks for tasks before sleeping. But I'm keeping it,
to be on the safe side.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:133: message_loop_->ScheduleWork();
On 2016/05/19 22:29:37, danakj wrote:
> How come this doesn't set message_loop_scheduled_ = true anymore?

It's not necessary. The worst that can happen is a spurious wake-up on startup.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:147: // Ensures |message_loop_| isn't
null'd while running.
On 2016/05/19 22:29:37, danakj wrote:
> "nulled". tho it's more like ensures the message loop isn't destroyed right?
cuz
> once it's null here it will also no longer exist.

yeha, changed to destroyed. The nulling happens in MessageLoop's destructor, so
they're essentially the same.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:148: AutoReadLock
ml_lock(message_loop_lock_);
On 2016/05/19 22:29:37, danakj wrote:
> nit: name this variable "hold_message_loop"

Done.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.cc:157: AutoLock
locked(incoming_queue_lock_);
On 2016/05/19 22:29:37, danakj wrote:
> nit: name this variable "hold"

Done.

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/1991623002/diff/20001/base/message_loop/incom...
base/message_loop/incoming_task_queue.h:81: // task a being posted.
On 2016/05/19 21:27:43, danakj wrote:
> is being

Done.

[Anand Mistry (off Chromium), 2016-06-06 12:18:00]

Ping!

[danakj, 2016-06-06 16:28:34]

LGTM

[Anand Mistry (off Chromium), 2016-06-08 00:18:20]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-06-08 00:19:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991623002/80001

[commit-bot: I haz the power, 2016-06-08 01:58:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-08 01:59:51]

Description was changed from

==========
Avoid holding |incoming_queue_lock_| while waking up the message loop.

When constraining chrome to a limited number of cores (read: 2), it
appears that the write syscall that wakes up the message loop thread
may take a significant amount of time (>80ms) despite being a non-blocking
write. During that time, if |incoming_queue_lock_| is being held, any other
thread that tried to post a task to that message queue will be blocked in
trying to acquire that lock.
==========

to

==========
Avoid holding |incoming_queue_lock_| while waking up the message loop.

When constraining chrome to a limited number of cores (read: 2), it
appears that the write syscall that wakes up the message loop thread
may take a significant amount of time (>80ms) despite being a non-blocking
write. During that time, if |incoming_queue_lock_| is being held, any other
thread that tried to post a task to that message queue will be blocked in
trying to acquire that lock.

Committed: https://crrev.com/7fa6701bc5183bd5a73203d4fe1309f75ccfd5b4
Cr-Commit-Position: refs/heads/master@{#398458}
==========

[commit-bot: I haz the power, 2016-06-08 01:59:52]

Patchset 5 (id:??) landed as
https://crrev.com/7fa6701bc5183bd5a73203d4fe1309f75ccfd5b4
Cr-Commit-Position: refs/heads/master@{#398458}