Re-land: cc: Use worker context for one-copy tile initialization.

Re-land: cc: Use worker context for one-copy tile initialization.

This moves management of staging resources to
OneCopyTileTaskWorkerPool class. This makes it possible
to use a worker context to issue and detect when copy
operations complete.

BUG=490295
TBR=jamesr@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/91d23278a61f82e03bf4bac2a5c509579a57edd0
Cr-Commit-Position: refs/heads/master@{#344490}

[reveman, 2015-07-15 20:10:47]

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

[reveman, 2015-07-16 21:54:14]

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

[reveman, 2015-07-16 21:54:16]

+vmiura for worker context related changes

[vmpstr, 2015-07-16 22:25:26]

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:85:
usleep(kCheckForQueryResultAvailableTickRateMs * 1000);
PlatformThread::Sleep?

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:90:
UMA_HISTOGRAM_CUSTOM_COUNTS("Renderer4.CopyTextureLatency", time_elapsed_us,
CopyTextureLatencyUS (we usually the time units, I think...)

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:329: void
OneCopyTileTaskWorkerPool::PlaybackAndScheduleCopyOnWorkerThread(
Can you break this function up into smaller logical parts? Like

buffer = FindStagingBufferForContentId
if (!buffer)
  buffer = FindFreeStagingBufferForSize
if (!buffer)
  buffer = AllocateNewStagingBuffer

type of thing... and internally it would use helpers as well.

It would just make reading this easier, I think. Wdyt?

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:387: WaitForQueryResult(gl,
busy_buffers_.front()->query_id);
Are we waiting for the first one, because they are guaranteed to complete in
order? (As opposed to waiting for the busy buffer that we want directly?)

Also I'm curious why waiting for Nth buffer is better than doing a full raster?

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:396:
std::sort(free_buffers_.begin(), free_buffers_.end(),
Should we have some sort of a dirty flag to ensure we don't sort unnecessarily?
wdyt?

[reveman, 2015-07-22 22:39:15]

Latest patch includes memory tracing support and significant improvements to
memory usage by releasing staging buffers that have not been used for some
amount of time.

Patch also adds support for RGBA_4444 as that's needed to not increase memory
usage on some low-end devices when switching from async uploads.

PTAL.

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:85:
usleep(kCheckForQueryResultAvailableTickRateMs * 1000);
On 2015/07/16 at 22:25:26, vmpstr wrote:
> PlatformThread::Sleep?

Done.

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:90:
UMA_HISTOGRAM_CUSTOM_COUNTS("Renderer4.CopyTextureLatency", time_elapsed_us,
On 2015/07/16 at 22:25:26, vmpstr wrote:
> CopyTextureLatencyUS (we usually the time units, I think...)

This is the same as before but I removed it from latest patch as it's not
documented and not really useful imo.

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:329: void
OneCopyTileTaskWorkerPool::PlaybackAndScheduleCopyOnWorkerThread(
On 2015/07/16 at 22:25:26, vmpstr wrote:
> Can you break this function up into smaller logical parts? Like
> 
> buffer = FindStagingBufferForContentId
> if (!buffer)
>   buffer = FindFreeStagingBufferForSize
> if (!buffer)
>   buffer = AllocateNewStagingBuffer
> 
> type of thing... and internally it would use helpers as well.
> 
> It would just make reading this easier, I think. Wdyt?

This has been completely refactored in latest patch. Ptal and let me know if you
think some helper functions are still useful.

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:387: WaitForQueryResult(gl,
busy_buffers_.front()->query_id);
On 2015/07/16 at 22:25:26, vmpstr wrote:
> Are we waiting for the first one, because they are guaranteed to complete in
order? (As opposed to waiting for the busy buffer that we want directly?)
> 
> Also I'm curious why waiting for Nth buffer is better than doing a full
raster?

The idea is that it would provide more predictable results. There's a race
between when a tile gets invalidated and the GPU is done using it which is very
device dependent. On some devices we might end up using partial updates for some
specific UI element but not on another. By waiting, we'd make this more
predictable.

Anyhow, I removed this change from latest patch. I think it's better to do this
as a follow up.

https://codereview.chromium.org/1230203007/diff/80001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.cc:396:
std::sort(free_buffers_.begin(), free_buffers_.end(),
On 2015/07/16 at 22:25:26, vmpstr wrote:
> Should we have some sort of a dirty flag to ensure we don't sort
unnecessarily? wdyt?

removed this code form latest patch.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:198: : kMaxDefaultStagingBuffers),
Note: this reduction in staging buffer usage is supposed to match the logic in
render_widget.cc where we set the mapped_memory_reclaim_limit. I'm a bit less
aggressive here as we're much better at releasing staging buffers when they are
no longer in use. Some logic for low-end devices is still needed to not increase
spikes in memory usage when switching from async uploads.

[vmpstr, 2015-07-22 23:38:53]

Comments and questions (just for cc/raster/one_copy_tile_task_worker_pool.cc,
the rest seems straight forward)

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:85: void
WaitForQueryResult(gpu::gles2::GLES2Interface* gl, unsigned query_id) {
This should probably return a bool?

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:98:
gl->GetQueryObjectuivEXT(query_id, GL_QUERY_RESULT_EXT, &result);
Is this safe to call if we run out of attempts? If so, what's the behavior?

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:198: : kMaxDefaultStagingBuffers),
On 2015/07/22 22:39:15, reveman wrote:
> Note: this reduction in staging buffer usage is supposed to match the logic in
> render_widget.cc where we set the mapped_memory_reclaim_limit. I'm a bit less
> aggressive here as we're much better at releasing staging buffers when they
are
> no longer in use. Some logic for low-end devices is still needed to not
increase
> spikes in memory usage when switching from async uploads.

This should probably be a setting in layer_tree_settings that we can pass to
OneCopyTileTaskWorkerPool at construction time and set to an appropriate value
for low end devices in render_widget_compositor. Wdyt?

I'd like to avoid IsLowEndDevice checks in cc

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:329: return
resource_provider_->memory_efficient_texture_format();
Why the change here?

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:372: {
I think each of these { } can easily be a separate function :P

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:388: // Wait for number of non-free
buffers to become less than the limit.
what's the relationship between busy_buffers_ buffers_ and free_buffers_? Like
non-free buffer is a busy_buffer? or not necessarily?

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:395: WaitForQueryResult(gl,
busy_buffers_.front()->query_id);
If we ran out of attempts, can we still consider this a free buffer?

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:471: void* data = NULL;
nullptr

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:639: GetUsageTimeForLRUBuffer() +
staging_buffer_expiration_delay_;
Can you make a comment to what this time is...

UsageTime + staging_buffer_expiration - current_time... I guess that's when the
next buffer is going to expire (time ticks from now)?

[vmiura, 2015-07-23 01:39:39]

Worker context changes lgtm.

Could you also update
OutputSurface::SetWorkerContextShouldAggressivelyFreeResources() to use
ScopedContextGL?

[reveman, 2015-07-23 05:35:05]

On 2015/07/23 at 01:39:39, vmiura wrote:
> Worker context changes lgtm.
> 
> Could you also update
OutputSurface::SetWorkerContextShouldAggressivelyFreeResources() to use
ScopedContextGL?

Done.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:85: void
WaitForQueryResult(gpu::gles2::GLES2Interface* gl, unsigned query_id) {
On 2015/07/22 at 23:38:52, vmpstr wrote:
> This should probably return a bool?

Not sure what that would be for. This function can't fail.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:98:
gl->GetQueryObjectuivEXT(query_id, GL_QUERY_RESULT_EXT, &result);
On 2015/07/22 at 23:38:52, vmpstr wrote:
> Is this safe to call if we run out of attempts? If so, what's the behavior?

Yes, it's always safe to query the result but there's no guarantee that the
GLES2Implementation will do anything smarter than calling glFinish and we'd like
to avoid that if possible. If checking has failed too many times we just query
the result and let GLES2Implementation handle it as that allows us to detect
driver bugs.

The plan is to improve this code by using gl->SignalQuery and allow the GPU
service to notify the child process as soon as the query has passed. This will
also allow the GPU service to poll for query results less frequently as it will
know if a compositor is blocking on a query result.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:198: : kMaxDefaultStagingBuffers),
On 2015/07/22 at 23:38:52, vmpstr wrote:
> On 2015/07/22 22:39:15, reveman wrote:
> > Note: this reduction in staging buffer usage is supposed to match the logic
in
> > render_widget.cc where we set the mapped_memory_reclaim_limit. I'm a bit
less
> > aggressive here as we're much better at releasing staging buffers when they
are
> > no longer in use. Some logic for low-end devices is still needed to not
increase
> > spikes in memory usage when switching from async uploads.
> 
> This should probably be a setting in layer_tree_settings that we can pass to
OneCopyTileTaskWorkerPool at construction time and set to an appropriate value
for low end devices in render_widget_compositor. Wdyt?
> 
> I'd like to avoid IsLowEndDevice checks in cc

Done.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:329: return
resource_provider_->memory_efficient_texture_format();
On 2015/07/22 at 23:38:52, vmpstr wrote:
> Why the change here?

This was preventing us from using RGBA_4444 on some low-end devices, which is
the main reason for why we had a memory regression when switching to one-copy on
some Android devices. I can move this change to a separate patch if you like? I
ended up including it here as I've been running tests to verify all memory usage
improvements this patch provides and this is an important part of that.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:388: // Wait for number of non-free
buffers to become less than the limit.
On 2015/07/22 at 23:38:52, vmpstr wrote:
> what's the relationship between busy_buffers_ buffers_ and free_buffers_? Like
non-free buffer is a busy_buffer? or not necessarily?

A buffer can be in one of three states. 1) free, 2) used for raster, 3) busy.
|buffers_| contains all buffers independent of state. Ownership of the buffer is
either in |free_buffers_|, in |busy_buffers_| or the local variable in this
function.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:395: WaitForQueryResult(gl,
busy_buffers_.front()->query_id);
On 2015/07/22 at 23:38:52, vmpstr wrote:
> If we ran out of attempts, can we still consider this a free buffer?

The attempts are just an internal detail to avoid a call to glFinish.
WaitForQueryResult does what the name describes, it waits for the query result
and will not return until it's available.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:471: void* data = NULL;
On 2015/07/22 at 23:38:52, vmpstr wrote:
> nullptr

Done.

https://codereview.chromium.org/1230203007/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:639: GetUsageTimeForLRUBuffer() +
staging_buffer_expiration_delay_;
On 2015/07/22 at 23:38:52, vmpstr wrote:
> Can you make a comment to what this time is...
> 
> UsageTime + staging_buffer_expiration - current_time... I guess that's when
the next buffer is going to expire (time ticks from now)?

Yes, lru buffer usage time + expiration delay is next time it makes sense to try
and reduce memory usage. We schedule a call to ReduceMemoryUsage at this time.
Comment added.

[vmpstr, 2015-07-23 17:51:07]

cc lgtm

https://codereview.chromium.org/1230203007/diff/120001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/120001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:527: {
nit: You don't really need to scope it, since the function ends after this scope
ends.

https://codereview.chromium.org/1230203007/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/1230203007/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1312: if (output_surface_ &&
global_tile_state_.hard_memory_limit_in_bytes == 0) {
nit: This is a no-op change? :P

[reveman, 2015-07-23 18:40:39]

reveman@chromium.org changed reviewers:
+ piman@chromium.org, primiano@chromium.org

[reveman, 2015-07-23 18:40:40]

+piman for content/
+primiano for memory-infra related changes in
cc/raster/one_copy_tile_task_worker_pool.cc

https://codereview.chromium.org/1230203007/diff/120001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/120001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:527: {
On 2015/07/23 at 17:51:07, vmpstr wrote:
> nit: You don't really need to scope it, since the function ends after this
scope ends.

Done.

https://codereview.chromium.org/1230203007/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/1230203007/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1312: if (output_surface_ &&
global_tile_state_.hard_memory_limit_in_bytes == 0) {
On 2015/07/23 at 17:51:07, vmpstr wrote:
> nit: This is a no-op change? :P

Good catch. This was a leftover from some changes here in an early version of
the patch. Removed it from latest patch.

[reveman, 2015-07-23 18:41:31]

On 2015/07/23 at 18:40:40, reveman wrote:
> +piman for content/

and ui/compositor/

[Primiano Tucci (use gerrit), 2015-07-23 19:01:20]

OnMemoryDump & co stuff LGTM

[piman, 2015-07-23 20:44:52]

https://codereview.chromium.org/1230203007/diff/140001/cc/output/context_prov...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1230203007/diff/140001/cc/output/context_prov...
cc/output/context_provider.h:30: class ScopedContextGL {
nit: I like this class but kinda hate the name. Maybe add a Lock somewhere?

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:269:
BROWSER_WORKER_CONTEXT));
Can you move this into the else block below, so that we don't try to do this if
we go software?

Separately, is there a way for us to share this context across compositors?
Since compositors typically don't go "invisible", we never really trim memory
for them (shm, command buffers etc.), so it would be good if we could share the
context to keep the memory footprint in check (avoid scaling with the number of
windows).

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:37:
nullptr,
Any reason we don't pass the worker context here?

[reveman, 2015-07-24 17:49:15]

In addition to addressing review feedback I made some minor memory-infra related
changes in latest patch, used a dump name that makes it easier to understand
what the memory is used for and added a free_size attribute that makes it easy
to see how much of the memory is used vs in the free list.

PTAL

https://codereview.chromium.org/1230203007/diff/140001/cc/output/context_prov...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/1230203007/diff/140001/cc/output/context_prov...
cc/output/context_provider.h:30: class ScopedContextGL {
On 2015/07/23 at 20:44:52, piman (Very slow to review) wrote:
> nit: I like this class but kinda hate the name. Maybe add a Lock somewhere?

Changed the name to ScopedContextLock.

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:269:
BROWSER_WORKER_CONTEXT));
On 2015/07/23 at 20:44:52, piman (Very slow to review) wrote:
> Can you move this into the else block below, so that we don't try to do this
if we go software?
> 
> Separately, is there a way for us to share this context across compositors?
Since compositors typically don't go "invisible", we never really trim memory
for them (shm, command buffers etc.), so it would be good if we could share the
context to keep the memory footprint in check (avoid scaling with the number of
windows).

Done. Added a shared worker context provider to latest patch.

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1230203007/diff/140001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:37:
nullptr,
On 2015/07/23 at 20:44:52, piman (Very slow to review) wrote:
> Any reason we don't pass the worker context here?

Added a worker context here in latest patch.

[piman, 2015-07-24 17:52:19]

lgtm

[reveman, 2015-07-24 20:08:13]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-07-24 20:08:13]

The patchset sent to the CQ was uploaded after l-g-t-m from vmiura@chromium.org,
vmpstr@chromium.org, primiano@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-07-24 20:09:21]

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

[commit-bot: I haz the power, 2015-07-24 20:31:08]

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

[commit-bot: I haz the power, 2015-07-24 20:31:09]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[reveman, 2015-07-24 22:53:54]

https://codereview.chromium.org/1230203007/diff/200001/cc/output/output_surfa...
File cc/output/output_surface.cc (left):

https://codereview.chromium.org/1230203007/diff/200001/cc/output/output_surfa...
cc/output/output_surface.cc:143: &OutputSurface::DidLoseOutputSurface,
base::Unretained(this)));
I had to remove this to be able to share worker contexts between compositors. Is
it enough that we get the callback for the main context?

[piman, 2015-07-24 23:00:02]

lgtm

[piman, 2015-08-04 18:44:54]

https://codereview.chromium.org/1230203007/diff/280001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/280001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:413:
resource_provider_->output_surface()->worker_context_provider();
Mmh, we're accessing the ResourceProvider & OutputSurface from the wrong thread.
What guarantee do we have over lifetime and thread safety of those? Also, should
we keep a reference to the ContextProvider?

[reveman, 2015-08-04 21:56:42]

https://codereview.chromium.org/1230203007/diff/280001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1230203007/diff/280001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:413:
resource_provider_->output_surface()->worker_context_provider();
On 2015/08/04 at 18:44:54, piman (Very slow to review) wrote:
> Mmh, we're accessing the ResourceProvider & OutputSurface from the wrong
thread. What guarantee do we have over lifetime and thread safety of those?
Also, should we keep a reference to the ContextProvider?

This consistent with GpuTileTaskWorkerPool and safe afaict. The RP and
OutputSurface are guaranteed to be alive for the lifetime of the
*TileTaskWorkerPool instance and LTHI seem to get that right. It might still be
nice to take a reference to the ContextProvider here and in
GpuTileTaskWorkerPool to make this less fragile. I prefer to do that as part of
a separate patch though.

[reveman, 2015-08-05 01:29:37]

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

[commit-bot: I haz the power, 2015-08-05 01:31:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/320001

[commit-bot: I haz the power, 2015-08-05 04:19:34]

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

[commit-bot: I haz the power, 2015-08-05 04:19:34]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[reveman, 2015-08-05 20:54:16]

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

[commit-bot: I haz the power, 2015-08-05 20:55:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/340001

[commit-bot: I haz the power, 2015-08-05 21:55:31]

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

[commit-bot: I haz the power, 2015-08-05 21:55:32]

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

[reveman, 2015-08-06 18:07:10]

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

[commit-bot: I haz the power, 2015-08-06 18:08:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/360001

[commit-bot: I haz the power, 2015-08-06 18:11:58]

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

[commit-bot: I haz the power, 2015-08-06 18:11:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[reveman, 2015-08-06 19:21:01]

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

[commit-bot: I haz the power, 2015-08-06 19:21:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/380001

[commit-bot: I haz the power, 2015-08-06 21:35:44]

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

[commit-bot: I haz the power, 2015-08-06 21:35:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[piman, 2015-08-06 21:44:11]

lgtm

[reveman, 2015-08-06 21:54:16]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-06 21:54:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, vmpstr@chromium.org, vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps380001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-06 21:55:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/380001

[commit-bot: I haz the power, 2015-08-06 23:58:31]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-08-06 23:59:21]

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

[Ken Russell (switch to Gerrit), 2015-08-07 04:30:01]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/1273163004/ by kbr@chromium.org.

The reason for reverting is: Suspected of colliding with another patch and
causing Issue 517719..

[reveman, 2015-08-08 20:33:06]

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

[commit-bot: I haz the power, 2015-08-08 20:33:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/400001

[commit-bot: I haz the power, 2015-08-08 21:12:29]

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

[commit-bot: I haz the power, 2015-08-08 21:12:30]

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

[reveman, 2015-08-09 09:10:59]

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

[commit-bot: I haz the power, 2015-08-09 09:11:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/420001

[commit-bot: I haz the power, 2015-08-09 09:50:31]

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

[commit-bot: I haz the power, 2015-08-09 09:50:32]

Dry run: This issue passed the CQ dry run.

[reveman, 2015-08-09 20:08:04]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-09 20:08:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, piman@chromium.org, vmpstr@chromium.org,
vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps420001
(title: "minor cleanup to image binding code")

[commit-bot: I haz the power, 2015-08-09 20:08:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/420001

[commit-bot: I haz the power, 2015-08-09 20:12:26]

Committed patchset #22 (id:420001)

[commit-bot: I haz the power, 2015-08-09 20:13:07]

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

[Ken Russell (switch to Gerrit), 2015-08-09 22:28:37]

A revert of this CL (patchset #22 id:420001) has been created in
https://codereview.chromium.org/1282183002/ by kbr@chromium.org.

The reason for reverting is: I'm sorry, but this patch definitely causes
failures of one of the context_lost tests on Windows on multiple bots as
described in https://code.google.com/p/chromium/issues/detail?id=490295#c15 .
I'm sorry the CQ isn't catching these, and I'm not sure why it isn't -- but it's
important to fix these failures rather than leaving them on the tree.
.

[reveman, 2015-08-17 18:44:29]

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

[commit-bot: I haz the power, 2015-08-17 18:45:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/500001

[Ken Russell (switch to Gerrit), 2015-08-17 19:59:06]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2015-08-17 19:59:07]

https://codereview.chromium.org/1230203007/diff/500001/gpu/command_buffer/cli...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/1230203007/diff/500001/gpu/command_buffer/cli...
gpu/command_buffer/client/cmd_buffer_helper.cc:202:
error::IsError(command_buffer_->GetLastState().error));
If the error::IsError case is hit in this DCHECK, will the code below work
correctly? In particular, the call to WaitForGetOffsetInRange? Will callers
notice the failures?

[commit-bot: I haz the power, 2015-08-17 20:01:10]

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

[commit-bot: I haz the power, 2015-08-17 20:01:12]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[piman, 2015-08-17 22:29:55]

lgtm

https://codereview.chromium.org/1230203007/diff/500001/gpu/command_buffer/cli...
File gpu/command_buffer/client/cmd_buffer_helper.cc (right):

https://codereview.chromium.org/1230203007/diff/500001/gpu/command_buffer/cli...
gpu/command_buffer/client/cmd_buffer_helper.cc:202:
error::IsError(command_buffer_->GetLastState().error));
On 2015/08/17 19:59:07, Ken Russell wrote:
> If the error::IsError case is hit in this DCHECK, will the code below work
> correctly? In particular, the call to WaitForGetOffsetInRange? Will callers
> notice the failures?

WaitForGetOffsetInRange will return false immediately.

[reveman, 2015-08-18 08:11:07]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-18 08:11:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, vmpstr@chromium.org, vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps500001
(title: "rebase and include error check in DCHECK")

[commit-bot: I haz the power, 2015-08-18 08:11:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/500001

[commit-bot: I haz the power, 2015-08-18 10:05:09]

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

[commit-bot: I haz the power, 2015-08-18 10:05:12]

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

[reveman, 2015-08-18 10:32:08]

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

[reveman, 2015-08-18 10:32:08]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-18 10:32:10]

+jamesr for third_party/mojo/src/mojo/public/c/gles2/

[reveman, 2015-08-18 10:32:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, piman@chromium.org, vmpstr@chromium.org,
vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps520001
(title: "fix mojo")

[commit-bot: I haz the power, 2015-08-18 10:32:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/520001

[commit-bot: I haz the power, 2015-08-18 11:45:51]

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

[commit-bot: I haz the power, 2015-08-18 11:45:53]

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

[reveman, 2015-08-18 14:59:04]

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

[commit-bot: I haz the power, 2015-08-18 14:59:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/540001

[reveman, 2015-08-18 15:04:31]

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

[reveman, 2015-08-18 15:04:32]

+sky for components/html_viewer and mojo/cc

[commit-bot: I haz the power, 2015-08-18 16:01:57]

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

[commit-bot: I haz the power, 2015-08-18 16:01:59]

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

[reveman, 2015-08-18 16:19:05]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-18 16:19:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, vmpstr@chromium.org, piman@chromium.org,
vmiura@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps560001
(title: "actually use the worker context")

[commit-bot: I haz the power, 2015-08-18 16:19:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/560001

[sky, 2015-08-18 16:52:31]

LGTM

[commit-bot: I haz the power, 2015-08-18 17:19:00]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2015-08-18 17:19:51]

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

[sky, 2015-08-18 21:44:38]

A revert of this CL (patchset #29 id:560001) has been created in
https://codereview.chromium.org/1298143003/ by sky@chromium.org.

The reason for reverting is: Sorry Dave. I'm reverting as this broke mandoline.
To see the breakage build mandoline:all, and run with:
mandoline --oopif-always-create-new-frame-tree
http://phuang.github.io/iframe.html .

We do have mandoline tests that run, but they run with the code path 
gfx::GLSurfaceTestSupport::InitializeOneOff(), which is why they likely still
pass. I'll see about making the tests not run with that config..

[Fady Samuel, 2015-08-18 22:56:51]

On 2015/08/18 21:44:38, sky wrote:
> A revert of this CL (patchset #29 id:560001) has been created in
> https://codereview.chromium.org/1298143003/ by mailto:sky@chromium.org.
> 
> The reason for reverting is: Sorry Dave. I'm reverting as this broke
mandoline.
> To see the breakage build mandoline:all, and run with:
> mandoline --oopif-always-create-new-frame-tree
> http://phuang.github.io/iframe.html .
> 
> We do have mandoline tests that run, but they run with the code path 
> gfx::GLSurfaceTestSupport::InitializeOneOff(), which is why they likely still
> pass. I'll see about making the tests not run with that config..

I started to fix this crash with the patch applied but didn't get to far:
https://codereview.chromium.org/1288653005 FYI if you'd like to continue where I
left off.

[reveman, 2015-08-19 11:01:06]

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

[commit-bot: I haz the power, 2015-08-19 11:01:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/580001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/580001

[commit-bot: I haz the power, 2015-08-19 12:15:51]

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

[commit-bot: I haz the power, 2015-08-19 12:15:52]

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

[reveman, 2015-08-19 13:08:34]

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

[commit-bot: I haz the power, 2015-08-19 13:09:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/610001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/610001

[commit-bot: I haz the power, 2015-08-19 14:24:22]

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

[commit-bot: I haz the power, 2015-08-19 14:24:23]

Dry run: This issue passed the CQ dry run.

[reveman, 2015-08-19 15:50:14]

piman, can you take a look at let me know if you think this temporary workaround
for mojo is OK  or if you prefer if we did something else

https://codereview.chromium.org/1230203007/diff/610001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1230203007/diff/610001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2120: // crbug.com/522440
This temporary solution allows us to land this without having to implement
proper worker context support in mojo.

[piman, 2015-08-19 19:32:57]

lgtm

[reveman, 2015-08-19 19:51:13]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-19 19:51:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, sky@chromium.org, vmiura@chromium.org,
vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps610001
(title: "remove components/html_viewer/ change")

[commit-bot: I haz the power, 2015-08-19 19:51:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/610001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/610001

[commit-bot: I haz the power, 2015-08-19 20:01:02]

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

[commit-bot: I haz the power, 2015-08-19 20:01:04]

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

[reveman, 2015-08-19 20:08:06]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-08-19 20:08:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/610001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/610001

[commit-bot: I haz the power, 2015-08-19 20:16:15]

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

[commit-bot: I haz the power, 2015-08-19 20:16:17]

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

[reveman, 2015-08-20 09:24:05]

The CQ bit was checked by reveman@chromium.org

[reveman, 2015-08-20 09:24:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, sky@chromium.org, piman@chromium.org,
vmiura@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1230203007/#ps630001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-20 09:24:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/630001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/630001

[commit-bot: I haz the power, 2015-08-20 11:32:00]

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

[commit-bot: I haz the power, 2015-08-20 11:32:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[reveman, 2015-08-20 11:53:03]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2015-08-20 11:53:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1230203007/630001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1230203007/630001

[commit-bot: I haz the power, 2015-08-20 13:41:47]

Committed patchset #33 (id:630001)

[commit-bot: I haz the power, 2015-08-20 13:42:28]

Patchset 33 (id:??) landed as
https://crrev.com/91d23278a61f82e03bf4bac2a5c509579a57edd0
Cr-Commit-Position: refs/heads/master@{#344490}

[Eric Caruso, 2015-09-10 21:10:17]

A revert of this CL (patchset #33 id:630001) has been created in
https://codereview.chromium.org/1336703002/ by ejcaruso@chromium.org.

The reason for reverting is: This CL is responsible for issue 525811. Pixel 2015
gets strange artifacts across suspend/resume that only go away when you reboot..