cc: Use max_bytes_per_copy_operation setting to determine one-copy flush interval.

cc: Use max_bytes_per_copy_operation setting to determine one-copy flush interval.

max_bytes_per_copy_operation provides a mechanism to split one-copy
work into smaller units to prevent the GPU service from being
blocked for too long. The flush interval groups a number of
copy operations into one from the GPU service POV so it makes
sense to use the same limit to determine how frequently to
issue flush tasks.

This keeps the flush interval at 4 copy operations when using a
512x512 tile size by using a 4MB default value for
max_bytes_per_copy_operation.

BUG=460288
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/53880ba4e2ae5b4e360f5ae84707600c0dff5fa5
Cr-Commit-Position: refs/heads/master@{#332896}

[reveman, 2015-05-28 00:35:31]

reveman@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2015-06-01 18:35:52]

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:357:
bytes_scheduled_since_last_flush_ += rows_to_copy * bytes_per_row;
So for big tiles (that are split up into several copies) this would cause us to
possibly post several tasks for the one tile (one per full copy)?

https://codereview.chromium.org/1148413007/diff/1/content/renderer/gpu/render...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1148413007/diff/1/content/renderer/gpu/render...
content/renderer/gpu/render_widget_compositor.cc:181: size_t
GetMaxBytesPerCopyOperation() {
Leave a comment here explaining the rationale for 4MB as the number.

[vmpstr, 2015-06-02 23:23:28]

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:362: if
(bytes_scheduled_since_last_flush_ >= max_bytes_per_copy_operation_) {
I think you need to DCHECK that max_bytes_per_copy_operation_ is less than
std::numeric_limits<size_t>::max(), since then we would never flush?

Either that or change the layer tree settings default to something smaller.

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.h:130: size_t
bytes_scheduled_since_last_flush_;
Can you make this int? I'm going through the cc code fixing size_t -> int
truncations in a few places, and this makes it difficult to deal with.

https://codereview.chromium.org/1148413007/diff/1/content/renderer/gpu/render...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1148413007/diff/1/content/renderer/gpu/render...
content/renderer/gpu/render_widget_compositor.cc:181: size_t
GetMaxBytesPerCopyOperation() {
On 2015/06/01 18:35:52, vmpstr wrote:
> Leave a comment here explaining the rationale for 4MB as the number.

Can you also make this int?

[reveman, 2015-06-04 01:19:51]

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:357:
bytes_scheduled_since_last_flush_ += rows_to_copy * bytes_per_row;
On 2015/06/01 at 18:35:52, vmpstr wrote:
> So for big tiles (that are split up into several copies) this would cause us
to possibly post several tasks for the one tile (one per full copy)?

Yup, that's the behavior we want.

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.cc:362: if
(bytes_scheduled_since_last_flush_ >= max_bytes_per_copy_operation_) {
On 2015/06/02 at 23:23:27, vmpstr wrote:
> I think you need to DCHECK that max_bytes_per_copy_operation_ is less than
std::numeric_limits<size_t>::max(), since then we would never flush?
> 
> Either that or change the layer tree settings default to something smaller.

A flush here is not required for correct behavior. numeric_limits<size_t>::max()
should work fine. This flush is just to reduce the size of copy operation
batches sent to the GPU process.

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1148413007/diff/1/cc/raster/one_copy_tile_tas...
cc/raster/one_copy_tile_task_worker_pool.h:130: size_t
bytes_scheduled_since_last_flush_;
On 2015/06/02 at 23:23:27, vmpstr wrote:
> Can you make this int? I'm going through the cc code fixing size_t -> int
truncations in a few places, and this makes it difficult to deal with.

Done.

https://codereview.chromium.org/1148413007/diff/1/content/renderer/gpu/render...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1148413007/diff/1/content/renderer/gpu/render...
content/renderer/gpu/render_widget_compositor.cc:181: size_t
GetMaxBytesPerCopyOperation() {
On 2015/06/02 at 23:23:27, vmpstr wrote:
> On 2015/06/01 18:35:52, vmpstr wrote:
> > Leave a comment here explaining the rationale for 4MB as the number.

Done.

> 
> Can you also make this int?

Done.

[vmpstr, 2015-06-04 01:26:44]

lgtm

https://codereview.chromium.org/1148413007/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1148413007/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:181: size_t
GetMaxBytesPerCopyOperation() {
nit: int this too please

[reveman, 2015-06-04 02:16:29]

reveman@chromium.org changed reviewers:
+ avi@chromium.org

[reveman, 2015-06-04 02:16:30]

+avi for content/

https://codereview.chromium.org/1148413007/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/1148413007/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:181: size_t
GetMaxBytesPerCopyOperation() {
On 2015/06/04 at 01:26:44, vmpstr wrote:
> nit: int this too please

Done.

[Avi (use Gerrit), 2015-06-04 19:34:22]

lgtm

stampity stamp

[reveman, 2015-06-04 19:35:35]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-06-04 19:35:36]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1148413007/#ps40001
(title: "int")

[commit-bot: I haz the power, 2015-06-04 19:36:22]

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

[commit-bot: I haz the power, 2015-06-04 20:11:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-04 20:11:58]

Patchset 3 (id:??) landed as
https://crrev.com/53880ba4e2ae5b4e360f5ae84707600c0dff5fa5
Cr-Commit-Position: refs/heads/master@{#332896}