scheduler: Add support for tracing scheduler state

scheduler: Add support for tracing scheduler state

This patch adds code for recording the interesting parts of the
TaskQueueManager, RendererSchedulerImpl and RenderSchedulerSelector
classes into a trace. The main bits of state that are captured:

 - All tasks in the incoming and work queues.
 - Number of tasks in the work queue as a time series.
 - The work queue which was selected for execution.
 - Scheduler priority both as a time series and snapshots.
 - Selector priorities as snapshots.

Sample trace:
https://drive.google.com/file/d/0ByyxMXB38gLDUTFpRzhldmI2Q2c/view?usp=sharing

BUG=391005
Committed: https://crrev.com/c6a4ab056d73a8bc454b6c4fb2591459a7dcd32a
Cr-Commit-Position: refs/heads/master@{#303642}

[Sami, 2014-10-27 16:18:50]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, petrcermak@chromium.org, picksi@chromium.org,
rmcilroy@chromium.org

[rmcilroy, 2014-10-27 17:51:29]

Looks great, there will be lots of useful info when this lands :).  A couple of
comments, the main one being whether we should be calling
TaskQueueManager::QueueAsValueInto from RendererSchedulerSelector.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:234: (Now() -
base::TimeTicks()).InMicroseconds() / 1000.0L);
Maybe pass in the value of "Now()" from it's use in updatePolicy so it is not
out-of-sync with respect to the values we used to make the policy update
decision?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_selector.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:38:
queues_[i].work_queue = work_queues[i];
Maybe set a default "unknown" name to avoid segfaults if you forget to set the
names (or do the trace functions handle this already)?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:156: }
Could we encapsulate this tracing code in a function call?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:160: } else if
(priority >= kNormalPriority) {
This is a bit confusing since ">= normal" sounds like higher or above priority. 
Maybe just "else if (priority != kControlPriority)?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:173:
TaskQueueManager::QueueAsValueInto(*queue.work_queue, state.get());
I'm not sure we should be serializing the taskQueueManager queues here.  It
doesn't capture the incoming tasks so I'm not sure if it would be completely
clear.  Would it be better to do this tracing in TaskQueueManager, and have that
class keep the names of the queues instead of the selector?  I think this would
be a clearer separation (you could add AsValueInto(...) to the TaskQueueSelector
interface and have the TaskQueueManager call that to serialize the priority
state when building a Trace Event after calling SelectWorkQueueToService.  WDYT?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:202: for
(QueuePriority priority = kHighPriority; priority < kQueuePriorityCount;
Should we not expose the control priority here?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_selector.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:90: void
DidSelectQueue(size_t selected_queue, QueuePriority priority);
nit - comment when to call this.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.h:83: static void
TaskAsValueInto(const base::PendingTask& task,
nit - private

[alexclarke, 2014-10-27 17:58:10]

alexclarke@google.com changed reviewers:
+ alexclarke@google.com

[alexclarke, 2014-10-27 17:58:11]

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:89: last_input_time_ =
Now();
I know last_input_time_ is in the json blob, but it would be nice to have some
tracing to mark when this function gets called.

Maybe there was a plan to do that in the trace viewer as a post process?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.h:125: mutable base::Lock
incoming_signals_lock_;
Hmm I thought that AssertAcquired was const:

https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

[picksi1, 2014-10-28 11:01:40]

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:47: }
Does it still make sense to use a "blink.scheduler" flag here?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:237: (last_input_time_ -
base::TimeTicks()).InMicroseconds() / 1000.0L);
Would InMillisecondsF() do the same here?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_selector.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:156: }
Does this all boil away in release builds? I worry about impacting
performance...

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:160: } else if
(priority >= kNormalPriority) {
Or should this be a case statement (following the pattern used by
RendererSchedulerSelector() below?)

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_selector.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:67: 
"AsValueInto" means nothing to me! Is there some idiom I am unaware of?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:91: 
Possibly the ship has sailed, but it should the selected_queue index be a
typedef (e.g. "QueueIndex") or something other than a size_t?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:106: };
Does this structure need to hold a work_queue pointer, as this adds an extra
indirection to every queue use? Could we just keep the name here as a parallel
data structure to the original task queue list?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:338:
state->SetInteger("sequence_num", task.sequence_num);
do we need the underscores in the strings here?

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:344: state->SetDouble(
ditto: InMillisecondsF?

[Sami, 2014-10-28 12:57:48]

Thanks for the comments all! All addressed.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:47: }
On 2014/10/28 11:01:40, picksi1 wrote:
> Does it still make sense to use a "blink.scheduler" flag here?

You're probably right, let's go for renderer.scheduler. Too bad the name in
TaskQueueManager can't easily be parameterized.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:89: last_input_time_ =
Now();
On 2014/10/27 17:58:11, alexclarke wrote:
> I know last_input_time_ is in the json blob, but it would be nice to have some
> tracing to mark when this function gets called.
> 
> Maybe there was a plan to do that in the trace viewer as a post process?

Good idea, I'll just add tracing here.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:234: (Now() -
base::TimeTicks()).InMicroseconds() / 1000.0L);
On 2014/10/27 17:51:28, rmcilroy wrote:
> Maybe pass in the value of "Now()" from it's use in updatePolicy so it is not
> out-of-sync with respect to the values we used to make the policy update
> decision?

Done, although it's a little messy since I want to avoid reading the clock
unnecessarily.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.cc:237: (last_input_time_ -
base::TimeTicks()).InMicroseconds() / 1000.0L);
On 2014/10/28 11:01:40, picksi1 wrote:
> Would InMillisecondsF() do the same here?

Yes, that's much cleaner. I wonder why cc -- where I lifted this from -- didn't
use that.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_impl.h:125: mutable base::Lock
incoming_signals_lock_;
On 2014/10/27 17:58:11, alexclarke wrote:
> Hmm I thought that AssertAcquired was const:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

Oops, I added this since I was taking the lock earlier but don't need to do that
anymore => removed.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_selector.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:38:
queues_[i].work_queue = work_queues[i];
On 2014/10/27 17:51:28, rmcilroy wrote:
> Maybe set a default "unknown" name to avoid segfaults if you forget to set the
> names (or do the trace functions handle this already)?

I'm only tracing the counter value if the queue has a valid name.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:156: }
On 2014/10/27 17:51:28, rmcilroy wrote:
> Could we encapsulate this tracing code in a function call?

I've now moved this to TQM and it's much cleaner there. Thanks for the
suggestion!

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:156: }
On 2014/10/28 11:01:40, picksi1 wrote:
> Does this all boil away in release builds? I worry about impacting
> performance...

Tracing is always compiled in -- having separate builds would just be too
cumbersome. It is very efficient when it is disabled at run time -- the only
cost you pay is a non-atomic read per trace event.

With the new patch this is more efficient since we don't iterate over the task
queues until we actually know we're tracing.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:160: } else if
(priority >= kNormalPriority) {
On 2014/10/27 17:51:28, rmcilroy wrote:
> This is a bit confusing since ">= normal" sounds like higher or above
priority. 
> Maybe just "else if (priority != kControlPriority)?

Agreed, done.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:160: } else if
(priority >= kNormalPriority) {
On 2014/10/28 11:01:40, picksi1 wrote:
> Or should this be a case statement (following the pattern used by
> RendererSchedulerSelector() below?)

I suppose a switch case would be more future proof. Done.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:173:
TaskQueueManager::QueueAsValueInto(*queue.work_queue, state.get());
On 2014/10/27 17:51:28, rmcilroy wrote:
> I'm not sure we should be serializing the taskQueueManager queues here.  It
> doesn't capture the incoming tasks so I'm not sure if it would be completely
> clear.  Would it be better to do this tracing in TaskQueueManager, and have
that
> class keep the names of the queues instead of the selector?  I think this
would
> be a clearer separation (you could add AsValueInto(...) to the
TaskQueueSelector
> interface and have the TaskQueueManager call that to serialize the priority
> state when building a Trace Event after calling SelectWorkQueueToService. 
WDYT?

Yeah, I think I agree with you. Doing it this way is a little backwards. I've
now moved everything to the TQM. The only downside is that we can't easily
parameterize the category used by TQM for tracing, which will complicate things
a little if we ever want to move TQM to base.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.cc:202: for
(QueuePriority priority = kHighPriority; priority < kQueuePriorityCount;
On 2014/10/27 17:51:28, rmcilroy wrote:
> Should we not expose the control priority here?

Ack, bad copy & paste. Thanks.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
File content/renderer/scheduler/renderer_scheduler_selector.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:67: 
On 2014/10/28 11:01:40, picksi1 wrote:
> "AsValueInto" means nothing to me! Is there some idiom I am unaware of?

Agreed that it isn't very descriptive, but it's pretty common tracing vernacular
so I'll follow the pattern:
https://code.google.com/p/chromium/codesearch#search/&q=asvalueinto&sq=packag...

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:90: void
DidSelectQueue(size_t selected_queue, QueuePriority priority);
On 2014/10/27 17:51:28, rmcilroy wrote:
> nit - comment when to call this.

Done.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:91: 
On 2014/10/28 11:01:40, picksi1 wrote:
> Possibly the ship has sailed, but it should the selected_queue index be a
> typedef (e.g. "QueueIndex") or something other than a size_t?

I think we went back and forth with this a couple of times but ended up with
size_t since it is commonly used with indices. Unfortunately a typedef doesn't
really buy you much in terms of type correctness because it will get implicitly
converted to and from a size_t.

(btw, typedefs are now discouraged for new code; "using" is the new hotness.)

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/r...
content/renderer/scheduler/renderer_scheduler_selector.h:106: };
On 2014/10/28 11:01:40, picksi1 wrote:
> Does this structure need to hold a work_queue pointer, as this adds an extra
> indirection to every queue use? Could we just keep the name here as a parallel
> data structure to the original task queue list?

Yes, the work queues are not owned by this class so we need to hold a pointer.
It was the same before this patch. There really shouldn't be extra indirection
here -- just the element stride changes. I don't like having two parallel arrays
because you have to make sure their sizes are in sync.

Anyway, this is all academic because this struct is no more :)

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:338:
state->SetInteger("sequence_num", task.sequence_num);
On 2014/10/28 11:01:40, picksi1 wrote:
> do we need the underscores in the strings here?

As opposed to camelCase? I think underscores are closer to the json flavor.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:344: state->SetDouble(
On 2014/10/28 11:01:40, picksi1 wrote:
> ditto: InMillisecondsF?

Done.

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.h:83: static void
TaskAsValueInto(const base::PendingTask& task,
On 2014/10/27 17:51:28, rmcilroy wrote:
> nit - private

Done.

[picksi1, 2014-10-28 13:38:59]

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:338:
state->SetInteger("sequence_num", task.sequence_num);
As opposed to spaces! I assume this string ends in TraceViewer so only needs to
be human readable (unless there are parsing issues along the way).

[Sami, 2014-10-28 13:42:00]

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/681793003/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:338:
state->SetInteger("sequence_num", task.sequence_num);
On 2014/10/28 13:38:59, picksi1 wrote:
> As opposed to spaces! I assume this string ends in TraceViewer so only needs
to
> be human readable (unless there are parsing issues along the way).
> 

You could have spaces here per the json spec, but I think we generally prefer
hacker_style. One of the ideas was to write custom visualization for this data
in Trace Viewer, so this isn't necessarily just for humans.

[rmcilroy, 2014-11-07 18:21:57]

This will need rebasing, but lgtm, thanks!

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:80:
"RendererSchedulerImpl::WillBeginFrame");
Would it be worth passing the args values (frame_time, interval) here?

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_task_queue_selector.cc (right):

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_task_queue_selector.cc:135: }
indentation

[Sami, 2014-11-10 14:35:00]

skyostil@chromium.org changed reviewers:
+ sievers@chromium.org

[Sami, 2014-11-10 14:35:01]

Thanks Ross. I fixed your comments and did some small reorganization.

Daniel, could you approve this for real?

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:80:
"RendererSchedulerImpl::WillBeginFrame");
On 2014/11/07 18:21:57, rmcilroy wrote:
> Would it be worth passing the args values (frame_time, interval) here?

Good idea. Looks like |args| already knows how to serialize itself to the trace.

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_task_queue_selector.cc (right):

https://codereview.chromium.org/681793003/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_task_queue_selector.cc:135: }
On 2014/11/07 18:21:57, rmcilroy wrote:
> indentation

Whoops, fixed :)

[no sievers, 2014-11-10 23:15:27]

lgtm

https://codereview.chromium.org/681793003/diff/140001/content/renderer/schedu...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/681793003/diff/140001/content/renderer/schedu...
content/renderer/scheduler/renderer_scheduler_impl.cc:291:
state->SetBoolean("has_task_queue_manager", task_queue_manager_.get());
Do you really care about this? The only call site for AsValueLocked() above has
an early-out for !task_queue_manager_.get.

[Sami, 2014-11-11 11:11:44]

On 2014/11/10 23:15:27, sievers wrote:
> lgtm
> 
>
https://codereview.chromium.org/681793003/diff/140001/content/renderer/schedu...
> File content/renderer/scheduler/renderer_scheduler_impl.cc (right):
> 
>
https://codereview.chromium.org/681793003/diff/140001/content/renderer/schedu...
> content/renderer/scheduler/renderer_scheduler_impl.cc:291:
> state->SetBoolean("has_task_queue_manager", task_queue_manager_.get());
> Do you really care about this? The only call site for AsValueLocked() above
has
> an early-out for !task_queue_manager_.get.

Yeah, I guess it is a little superfluous. Removed.

[Sami, 2014-11-11 11:11:57]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-11 11:12:45]

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

[commit-bot: I haz the power, 2014-11-11 11:15:29]

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

[commit-bot: I haz the power, 2014-11-11 11:15:30]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2014-11-11 12:15:42]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-11 12:16:57]

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

[commit-bot: I haz the power, 2014-11-11 12:33:50]

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

[commit-bot: I haz the power, 2014-11-11 12:33:51]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sami, 2014-11-11 12:44:07]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-11 12:45:09]

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

[commit-bot: I haz the power, 2014-11-11 14:25:50]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-11-11 14:26:27]

Patchset 11 (id:??) landed as
https://crrev.com/c6a4ab056d73a8bc454b6c4fb2591459a7dcd32a
Cr-Commit-Position: refs/heads/master@{#303642}