gpu: GPU service scheduler.

gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

The scheduler maintains a priority queue of sequences ordered by
sequence priority and then order number. Priority inversion because of
sync token dependencies is handled by a simple priority ceiling scheme.
Sequences which are expected to release sync tokens dependencies are
assigned a priority (HIGH) that's higher than any client specified
priorities. However, browser view context sequences can have an even
higher priority (REAL_TIME).

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2814843002
Cr-Commit-Position: refs/heads/master@{#471187}
Committed: https://chromium.googlesource.com/chromium/src/+/e7e36ccdaa3fcf63b330bc956eeeb73abf7c39ea

[sunnyps, 2017-04-11 19:01:32]

Description was changed from

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
==========

to

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sunnyps, 2017-04-11 19:41:37]

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

[commit-bot: I haz the power, 2017-04-11 19:42:27]

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

[commit-bot: I haz the power, 2017-04-11 19:48:03]

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

[commit-bot: I haz the power, 2017-04-11 19:48:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[sunnyps, 2017-04-11 19:51:07]

Description was changed from

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

The scheduler maintains a priority queue of sequences ordered by
sequence priority and then order number. Priority inversion because of
sync token dependencies is handled by a simple priority ceiling scheme.
Sequences which are expected to release sync tokens dependencies are
assigned a priority (HIGH) that's higher than any client specified
priorities. However, browser view context sequences can have an even
higher priority (REAL_TIME).

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sunnyps, 2017-04-11 19:59:51]

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

[commit-bot: I haz the power, 2017-04-11 20:00:28]

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

[sunnyps, 2017-04-11 20:02:22]

This is not fully done yet but is very close to the final state.

PTAL at the overall design, API, naming, etc.

This has few tests right now. I'm working on adding more.

[commit-bot: I haz the power, 2017-04-11 20:29:56]

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

[commit-bot: I haz the power, 2017-04-11 20:29:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunnyps, 2017-04-14 20:33:25]

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

[commit-bot: I haz the power, 2017-04-14 20:33:48]

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

[sunnyps, 2017-04-14 20:35:01]

sunnyps@chromium.org changed reviewers:
+ vmiura@chromium.org

[sunnyps, 2017-04-14 20:35:02]

vmiura@ PTAL

[sunnyps, 2017-05-02 02:02:03]

Description was changed from

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

The scheduler maintains a priority queue of sequences ordered by
sequence priority and then order number. Priority inversion because of
sync token dependencies is handled by a simple priority ceiling scheme.
Sequences which are expected to release sync tokens dependencies are
assigned a priority (HIGH) that's higher than any client specified
priorities. However, browser view context sequences can have an even
higher priority (REAL_TIME).

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

The scheduler maintains a priority queue of sequences ordered by
sequence priority and then order number. Priority inversion because of
sync token dependencies is handled by a simple priority ceiling scheme.
Sequences which are expected to release sync tokens dependencies are
assigned a priority (HIGH) that's higher than any client specified
priorities. However, browser view context sequences can have an even
higher priority (REAL_TIME).

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[vmiura, 2017-05-04 21:27:37]

I'm still reviewing the Scheduler implementation.  A few comments for now.

https://codereview.chromium.org/2814843002/diff/80001/gpu/ipc/service/gpu_cha...
File gpu/ipc/service/gpu_channel.h (right):

https://codereview.chromium.org/2814843002/diff/80001/gpu/ipc/service/gpu_cha...
gpu/ipc/service/gpu_channel.h:291: //   process. To guarantee fairness, we must
wait a minimum amount of time
Is this preemption description still valid?

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16032:
ExitCommandProcessingEarly();
Is there a particular reason to exit early after a release?

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:304: sequence->Destroy();
Is it necessary to defer the Destroy?  Is it due to sequences_ having ownership
of the sequence lifetime?

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:458: scheduling_queue_ =
SchedulingQueue(states.begin(), states.end());
For efficiency, it would be better to use the std::priority_queue move
constructor form.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.h:123: mutable base::Lock lock_;
The lock should be declared before the variables it protects.

[piman, 2017-05-09 00:48:39]

Ditto, still reviewing the scheduler code, but here's feedback on the rest,
mostly nits.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
File gpu/command_buffer/client/context_support.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
gpu/command_buffer/client/context_support.h:29: virtual void
EnsureWorkVisibleAsync() = 0;
nit: please provide documentation. In particular, why having this instead of
just calling Flush? Is it for the thread-safety?

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gpu_control.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
gpu/command_buffer/client/gpu_control.h:72: virtual void
EnsureWorkVisibleAsync() = 0;
nit: please provide documentation here.
Not too sure about the naming. What it does really is flush the ordering
barriers, right? As is it sounds you would expect it to be like
EnsureWorkVisible but with a callback instead of synchronous.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/com...
File gpu/command_buffer/common/gpu_stream_constants.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/com...
gpu/command_buffer/common/gpu_stream_constants.h:18: GPU_STREAM_WORKER = 1,
I'm not excited about defining here a notion that belongs to the higher levels
(worker). For example we may decide in the future that we want a separate stream
for Pepper or WebGL or video, but those are concepts that only exist in content/

I would prefer keeping that enum in content/, if we want a static allocation of
stream ids. Otherwise the previous dynamic allocation doesn't seem all that
bothersome?

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:776: Send(reply);
nit: if channel_ is nullptr, Send won't do anything, so you can skip this (sync
messages should be unblock on the client side if this happens).

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.h:234: std::map<int32_t, SequenceId>
stream_sequences_;
nit: flat_map? This will likely be small and more often looked up than changed?

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.h:339: std::map<int32_t, SequenceId>
route_sequences_;
nit: flat_map too? Likely small ish, and more often looked up than changed?

[piman, 2017-05-10 00:38:50]

Some more nits. Overall this is nice though, close to landing I think.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:106: // the command buffer is
descheduled/sc
nit: "/sc" typo or missing end of comment?

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:244: running_state_ = IDLE;
nit: could be premature optimization, but in Scheduler::RunNextTask, we call
FinishTask, then if (IsRunnable()) SetScheduled()
Would it make sense to combine it here, and set the state to SCHEDULED directly
if IsRunnable() ? That way we avoid calling UpdateSchedulingState twice. Little
things, not sure if they matter.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:292: std::unique_ptr<Sequence> sequence
=
nit: it's ok to use auto when using MakeUnique (that's more or less the point of
MakeUnique).

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:449: std::vector<SchedulingState>
states;
nit: states.reserve(sequences_.count()); to avoid reallocations during the loop?

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:458: scheduling_queue_ =
SchedulingQueue(states.begin(), states.end());
On 2017/05/04 21:27:37, vmiura wrote:
> For efficiency, it would be better to use the std::priority_queue move
> constructor form.

I'd even argue, if we access underlying container (scheduling_queue_.c), we can
save allocations, e.g. by reusing the underlying vector here (rather that create
a new one in |states|) and calling std::make_heap.
It's a bit more code, but could make things a bit faster.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.h:110: std::map<SequenceId,
std::unique_ptr<Sequence>> sequences_;
nit: flat_map may work well here.

[sunnyps, 2017-05-10 23:15:16]

Addressed all comments and removed the temporary EnsureWorkVisibleAsync patch.

PTAL

https://codereview.chromium.org/2814843002/diff/80001/gpu/ipc/service/gpu_cha...
File gpu/ipc/service/gpu_channel.h (right):

https://codereview.chromium.org/2814843002/diff/80001/gpu/ipc/service/gpu_cha...
gpu/ipc/service/gpu_channel.h:291: //   process. To guarantee fairness, we must
wait a minimum amount of time
On 2017/05/04 21:27:37, vmiura wrote:
> Is this preemption description still valid?

I moved the filter and message queue to gpu_channel.cc and updated the comments.
The filter hasn't been responsible for preemption for some time. However, the
comment still applies to message queue.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
File gpu/command_buffer/client/context_support.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
gpu/command_buffer/client/context_support.h:29: virtual void
EnsureWorkVisibleAsync() = 0;
On 2017/05/09 00:48:38, piman wrote:
> nit: please provide documentation. In particular, why having this instead of
> just calling Flush? Is it for the thread-safety?

Sorry, I will split this out into a separate CL. I only included this because I
wanted to run telemetry trybots which doesn't play well with dependent patchsets
AFAIK.

I want to add a thread-safe Flush that flushes the last ordering barrier for the
worker stream. Not 100% sure how to expose it though.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
File gpu/command_buffer/client/gpu_control.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/cli...
gpu/command_buffer/client/gpu_control.h:72: virtual void
EnsureWorkVisibleAsync() = 0;
On 2017/05/09 00:48:38, piman wrote:
> nit: please provide documentation here.
> Not too sure about the naming. What it does really is flush the ordering
> barriers, right? As is it sounds you would expect it to be like
> EnsureWorkVisible but with a callback instead of synchronous.

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/com...
File gpu/command_buffer/common/gpu_stream_constants.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/com...
gpu/command_buffer/common/gpu_stream_constants.h:18: GPU_STREAM_WORKER = 1,
On 2017/05/09 00:48:38, piman wrote:
> I'm not excited about defining here a notion that belongs to the higher levels
> (worker). For example we may decide in the future that we want a separate
stream
> for Pepper or WebGL or video, but those are concepts that only exist in
content/
> 
> I would prefer keeping that enum in content/, if we want a static allocation
of
> stream ids. Otherwise the previous dynamic allocation doesn't seem all that
> bothersome?

The service side implementation of stream lifetimes is simpler for static
allocation of streams. The service doesn't need to specifically know the stream
ids so I've moved those to content/common/gpu_stream_constants.h.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16032:
ExitCommandProcessingEarly();
On 2017/05/04 21:27:37, vmiura wrote:
> Is there a particular reason to exit early after a release?

Releasing can unblock a higher priority context so we need to check with the
scheduler. Added a comment about this.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:106: // the command buffer is
descheduled/sc
On 2017/05/10 00:38:50, piman wrote:
> nit: "/sc" typo or missing end of comment?

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:244: running_state_ = IDLE;
On 2017/05/10 00:38:50, piman wrote:
> nit: could be premature optimization, but in Scheduler::RunNextTask, we call
> FinishTask, then if (IsRunnable()) SetScheduled()
> Would it make sense to combine it here, and set the state to SCHEDULED
directly
> if IsRunnable() ? That way we avoid calling UpdateSchedulingState twice.
Little
> things, not sure if they matter.

I kept it this way because I wanted to be explicit that SCHEDULED means that the
sequence is in the scheduling queue.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:292: std::unique_ptr<Sequence> sequence
=
On 2017/05/10 00:38:50, piman wrote:
> nit: it's ok to use auto when using MakeUnique (that's more or less the point
of
> MakeUnique).

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:304: sequence->Destroy();
On 2017/05/04 21:27:37, vmiura wrote:
> Is it necessary to defer the Destroy?  Is it due to sequences_ having
ownership
> of the sequence lifetime?

This isn't necessary as long as destroying the stream won't have nasty
side-effects. I was going to run pending tasks on destruction but after
discussing with piman@ we decided that's not necessary if we're destroying
streams when we destroy the channel (no need to reply to sync messages). I've
removed the two-stage destruction for now.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:449: std::vector<SchedulingState>
states;
On 2017/05/10 00:38:50, piman wrote:
> nit: states.reserve(sequences_.count()); to avoid reallocations during the
loop?

Not needed any more (see below).

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:458: scheduling_queue_ =
SchedulingQueue(states.begin(), states.end());
On 2017/05/10 00:38:50, piman wrote:
> On 2017/05/04 21:27:37, vmiura wrote:
> > For efficiency, it would be better to use the std::priority_queue move
> > constructor form.
> 
> I'd even argue, if we access underlying container (scheduling_queue_.c), we
can
> save allocations, e.g. by reusing the underlying vector here (rather that
create
> a new one in |states|) and calling std::make_heap.
> It's a bit more code, but could make things a bit faster.

Changed to use make_heap, etc.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.h:110: std::map<SequenceId,
std::unique_ptr<Sequence>> sequences_;
On 2017/05/10 00:38:50, piman wrote:
> nit: flat_map may work well here.

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.h:123: mutable base::Lock lock_;
On 2017/05/04 21:27:37, vmiura wrote:
> The lock should be declared before the variables it protects.

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:776: Send(reply);
On 2017/05/09 00:48:38, piman wrote:
> nit: if channel_ is nullptr, Send won't do anything, so you can skip this
(sync
> messages should be unblock on the client side if this happens).

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.h (right):

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.h:234: std::map<int32_t, SequenceId>
stream_sequences_;
On 2017/05/09 00:48:38, piman wrote:
> nit: flat_map? This will likely be small and more often looked up than
changed?

Done.

https://codereview.chromium.org/2814843002/diff/240001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.h:339: std::map<int32_t, SequenceId>
route_sequences_;
On 2017/05/09 00:48:39, piman wrote:
> nit: flat_map too? Likely small ish, and more often looked up than changed?

Done.

[piman, 2017-05-10 23:35:24]

lgtm

[sunnyps, 2017-05-10 23:53:34]

sunnyps@chromium.org changed reviewers:
+ dcheng@chromium.org, sadrul@chromium.org

[sunnyps, 2017-05-10 23:53:35]

+dcheng@ for OWNERS review of gpu/ipc changes
+sadrul@ for OWNERS review of services/ui/.../gpu changes

[dcheng, 2017-05-11 03:31:47]

ipc lgtm

https://codereview.chromium.org/2814843002/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.cc (right):

https://codereview.chromium.org/2814843002/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:151: : sequence_id_(sequence_id),
priority_(priority), order_data_(order_data) {}
Nit: std::move(order_data) since it's passed by value anyway

[sadrul, 2017-05-11 04:05:09]

lgtm

https://codereview.chromium.org/2814843002/diff/300001/services/ui/public/cpp...
File services/ui/public/cpp/gpu/gpu.cc (right):

https://codereview.chromium.org/2814843002/diff/300001/services/ui/public/cpp...
services/ui/public/cpp/gpu/gpu.cc:67: bool support_locking = false;
Can these remain constexpr (or const)?

[vmiura, 2017-05-11 18:49:45]

lgtm4

[vmiura, 2017-05-11 18:50:04]

lgtm

[sunnyps, 2017-05-11 20:58:27]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2017-05-11 20:58:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
vmiura@chromium.org, dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2814843002/#ps320001
(title: "nits")

[sunnyps, 2017-05-11 20:58:55]

Thanks everyone!

https://codereview.chromium.org/2814843002/diff/300001/gpu/command_buffer/ser...
File gpu/command_buffer/service/scheduler.cc (right):

https://codereview.chromium.org/2814843002/diff/300001/gpu/command_buffer/ser...
gpu/command_buffer/service/scheduler.cc:151: : sequence_id_(sequence_id),
priority_(priority), order_data_(order_data) {}
On 2017/05/11 03:31:47, dcheng wrote:
> Nit: std::move(order_data) since it's passed by value anyway

Done.

https://codereview.chromium.org/2814843002/diff/300001/services/ui/public/cpp...
File services/ui/public/cpp/gpu/gpu.cc (right):

https://codereview.chromium.org/2814843002/diff/300001/services/ui/public/cpp...
services/ui/public/cpp/gpu/gpu.cc:67: bool support_locking = false;
On 2017/05/11 04:05:09, sadrul wrote:
> Can these remain constexpr (or const)?

Done.

[commit-bot: I haz the power, 2017-05-11 20:59:25]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-11 22:16:22]

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

[commit-bot: I haz the power, 2017-05-11 22:16:24]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sunnyps, 2017-05-11 23:23:22]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2017-05-11 23:23:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
vmiura@chromium.org, dcheng@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2814843002/#ps340001
(title: "fix test dcheck failures")

[commit-bot: I haz the power, 2017-05-11 23:24:52]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-12 00:32:34]

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

[commit-bot: I haz the power, 2017-05-12 00:32:36]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunnyps, 2017-05-12 01:13:10]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2017-05-12 01:14:02]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-12 02:26:31]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1494551590228190,
"parent_rev": "47778d3b6f6e5da0a698689d9c4c3e7e3cf0f51e", "commit_rev":
"e7e36ccdaa3fcf63b330bc956eeeb73abf7c39ea"}

[commit-bot: I haz the power, 2017-05-12 02:26:42]

Description was changed from

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

The scheduler maintains a priority queue of sequences ordered by
sequence priority and then order number. Priority inversion because of
sync token dependencies is handled by a simple priority ceiling scheme.
Sequences which are expected to release sync tokens dependencies are
assigned a priority (HIGH) that's higher than any client specified
priorities. However, browser view context sequences can have an even
higher priority (REAL_TIME).

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gpu: GPU service scheduler.

The GPU service scheduler runs tasks (closures) containing command
buffer work. Tasks are enqueued in sequences which represent indepdent
streams of execution. A task in one sequence may wait on a sync token
fence that's released on another sequence. Tasks can specify their sync
token fences beforehand so that the scheduler won't run a sequence only
to disable it due to a sync token wait. Tasks can check if they should
yield so that a higher priority task may run. If a task decides to
yield, it can continue execution by supplying a new closure which will
run before any other scheduled tasks in that sequence.

The scheduler maintains a priority queue of sequences ordered by
sequence priority and then order number. Priority inversion because of
sync token dependencies is handled by a simple priority ceiling scheme.
Sequences which are expected to release sync tokens dependencies are
assigned a priority (HIGH) that's higher than any client specified
priorities. However, browser view context sequences can have an even
higher priority (REAL_TIME).

GPU channel IPC messages are run as tasks in the scheduler. Each client
side stream is mapped to a sequence. The client is expected to use a
small number of streams to partition its command buffers so there is no
tracking of stream lifetimes in the service. The message filter posts
messages as tasks. AsyncFlush is the only message for which sync token
dependencies are given to the scheduler.

The initial implementation uses the GPU main thread to run scheduled
tasks. Future implementations, for example, could use a worker pool
with task traits to specify which thread a task should run on.

R=piman@chromium.org
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2814843002
Cr-Commit-Position: refs/heads/master@{#471187}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7e36ccdaa3fcf63b330bc956eee...
==========

[commit-bot: I haz the power, 2017-05-12 02:26:44]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/e7e36ccdaa3fcf63b330bc956eee...

[Justin Donnelly, 2017-05-12 16:43:44]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/2881813002/ by jdonnelly@chromium.org.

The reason for reverting is: Suspected of breaking the Win7 Tests (dbg)(1)
builder:

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...

Sample stack trace:

[6424:6796:0512/085713.197:FATAL:gpu_channel.cc(122)] Check failed:
channel_messages_.empty(). 
Backtrace:
	base::debug::StackTrace::StackTrace [0x100AD557+55]
	base::debug::StackTrace::StackTrace [0x100AD1F1+17]
	logging::LogMessage::~LogMessage [0x1010103E+94]
	gpu::GpuChannelMessageQueue::~GpuChannelMessageQueue [0x0AA6A1DE+126]
	gpu::GpuChannelMessageQueue::`scalar deleting destructor' [0x0AA6D13F+15]
	base::RefCountedThreadSafe<gpu::GpuChannelMessageQueue,base::DefaultRefCountedThreadSafeTraits<gpu::GpuChannelMessageQueue>
>::DeleteInternal [0x0AA6E4A2+34]
	base::DefaultRefCountedThreadSafeTraits<gpu::GpuChannelMessageQueue>::Destruct
[0x0AA6E7BC+12]
	base::RefCountedThreadSafe<gpu::GpuChannelMessageQueue,base::DefaultRefCountedThreadSafeTraits<gpu::GpuChannelMessageQueue>
>::Release [0x0AA731D0+32]
	scoped_refptr<gpu::GpuChannelMessageQueue>::Release [0x0AA7323B+11]
	scoped_refptr<gpu::GpuChannelMessageQueue>::~scoped_refptr<gpu::GpuChannelMessageQueue>
[0x0AA69A5A+26]
	gpu::GpuChannelMessageFilter::~GpuChannelMessageFilter [0x0AA6A0F7+151]
	gpu::GpuChannelMessageFilter::`vector deleting destructor' [0x0AA6CC4D+77]
	base::RefCountedThreadSafe<IPC::MessageFilter,base::DefaultRefCountedThreadSafeTraits<IPC::MessageFilter>
>::DeleteInternal [0x0B154D27+39]
	base::DefaultRefCountedThreadSafeTraits<IPC::MessageFilter>::Destruct
[0x0B154F3C+12]
	base::RefCountedThreadSafe<IPC::MessageFilter,base::DefaultRefCountedThreadSafeTraits<IPC::MessageFilter>
>::Release [0x0B1565DA+58]
	scoped_refptr<IPC::MessageFilter>::Release [0x0B15662E+14]
	scoped_refptr<IPC::MessageFilter>::~scoped_refptr<IPC::MessageFilter>
[0x0B152A7A+26]
	scoped_refptr<IPC::MessageFilter>::`scalar deleting destructor' [0x0B15411F+15]
	std::allocator<scoped_refptr<IPC::MessageFilter>
>::destroy<scoped_refptr<IPC::MessageFilter> > [0x0B151261+17]
	std::allocator_traits<std::allocator<scoped_refptr<IPC::MessageFilter> >
>::destroy<scoped_refptr<IPC::MessageFilter> > [0x0B15127F+15]
	std::_Wrap_alloc<std::allocator<scoped_refptr<IPC::MessageFilter> >
>::destroy<scoped_refptr<IPC::MessageFilter> > [0x0B151234+20]
	std::_Destroy_range1<std::allocator<scoped_refptr<IPC::MessageFilter>
>,scoped_refptr<IPC::MessageFilter> *> [0x0B15074B+43]
	std::_Destroy_range<std::allocator<scoped_refptr<IPC::MessageFilter>
>,scoped_refptr<IPC::MessageFilter> *> [0x0B1507AA+26]
	std::vector<scoped_refptr<IPC::MessageFilter>,std::allocator<scoped_refptr<IPC::MessageFilter>
> >::_Destroy [0x0B15740D+29]
	std::vector<scoped_refptr<IPC::MessageFilter>,std::allocator<scoped_refptr<IPC::MessageFilter>
> >::clear [0x0B15878E+46]
	IPC::ChannelProxy::Context::OnChannelClosed [0x0B1558EB+395]
	IPC::SyncChannel::SyncContext::OnChannelClosed [0x0B190416+38]
	??$Invoke@ABV?$scoped_refptr@VContext@ChannelProxy@IPC@@@@$$V@?$FunctorTraits@P8Context@ChannelProxy@IPC@@AEXXZX@internal@base@@SAXP8Context@ChannelProxy@IPC@@AEXXZABV?$scoped_refptr@VContext@ChannelProxy@IPC@@@@@Z
[0x0B14F978+24]
	base::internal::InvokeHelper<0,void>::MakeItSo<void (__thiscall
IPC::ChannelProxy::Context::*const
&)(void),scoped_refptr<IPC::ChannelProxy::Context> const &> [0x0B1500B8+40]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
IPC::ChannelProxy::Context::*)(void),scoped_refptr<IPC::ChannelProxy::Context>
>,void __cdecl(void)>::RunImpl<void (__thiscall
IPC::ChannelProxy::Context::*const &)(void),std::tuple<scoped [0x0B1504A4+52]
	base::internal::Invoker<base::internal::BindState<void (__thiscall
IPC::ChannelProxy::Context::*)(void),scoped_refptr<IPC::ChannelProxy::Context>
>,void __cdecl(void)>::Run [0x0B156904+36]
	base::Callback<void __cdecl(void),0,0>::Run [0x10049A45+53]
	base::debug::TaskAnnotator::RunTask [0x100B444C+476]
	base::MessageLoop::RunTask [0x1012F94C+620]
	base::MessageLoop::DeferOrRunPendingTask [0x1012DE4B+43]
	base::MessageLoop::DoWork [0x1012E44F+239]
	base::MessagePumpForIO::DoRunLoop [0x10136211+33]
	base::MessagePumpWin::Run [0x101372FB+123]
	base::MessageLoop::RunHandler [0x1012F5C5+293]
	base::RunLoop::Run [0x101EDB26+166]
	base::Thread::Run [0x10289D01+273]
	base::Thread::ThreadMain [0x1028AE4F+863]
	base::PlatformThread::Sleep [0x102659EC+380]
	BaseThreadInitThunk [0x76A9338A+18]
	RtlInitializeExceptionChain [0x77A39902+99]
	RtlInitializeExceptionChain [0x77A398D5+54].