cc: Move GpuMemoryBuffer allocation to worker threads.

cc: Move GpuMemoryBuffer allocation to worker threads.

Allocation of GpuMemoryBuffers requires a round-trip to the browser
process and by moving this to worker threads it is no longer on
the critical path on the compositor.

The LockForWrite mechanism in the ResourceProvider has been
adjusted to properly support worker thread allocation of
resources.

BUG=418553
Committed: https://crrev.com/2b6d4cba32db0500609b85ed8f9a3dc44fd8555f
Cr-Commit-Position: refs/heads/master@{#301433}

[reveman, 2014-10-10 16:12:52]

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

[reveman, 2014-10-10 16:12:55]

This is a follow up to the CHROMIUM_image refactor that enables worker thread
mapping of GpuMemoryBuffers. Please take a look.

Btw, I think we should to the same for shared bitmaps. If not for performance,
for consistency. That's for another patch though.

[piman, 2014-10-10 21:52:09]

couple of nits/suggestions.

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:942: ResourceProvider::Resource*
ResourceProvider::UnlockForWrite(ResourceId id) {
nit: since you keep the resource_id in the locks, maybe you could just instead
keep the Resource* and have this be a void
ResourceProvider::UnlockForWrite(Resource* resource), and save a map lookup?

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1065: DCHECK(resource->image_id);
nit: remove DCHECK as with previous patch?

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1082:
resource_provider_->gpu_memory_buffer_manager_->AllocateGpuMemoryBuffer(
IIUC, this is called from the raster threads. ResourceProvider isn't really
thread safe (but GpuMemoryBufferManager is meant to be), so the
resource_provider_->gpu_memory_buffer_manager_ part of this feels iffy.
Should we save the GpuMemoryBufferManager pointer in the constructor?
Or can we make it an enforced rule that
resource_provider_->gpu_memory_buffer_manager_ can't change (e.g. making it a
const field - pointer to a non-const GpuMemoryBufferManager of course).

[reveman, 2014-10-26 22:30:48]

Sorry for the delay. I decided to hold back on this in case changes to 1-copy
rasterizer would require something different. Looks like this is exactly what we
want now that those changes are implemented. PTAL.

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:942: ResourceProvider::Resource*
ResourceProvider::UnlockForWrite(ResourceId id) {
On 2014/10/10 21:52:08, piman (Very slow to review) wrote:
> nit: since you keep the resource_id in the locks, maybe you could just instead
> keep the Resource* and have this be a void
> ResourceProvider::UnlockForWrite(Resource* resource), and save a map lookup?

Done.

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1065: DCHECK(resource->image_id);
On 2014/10/10 21:52:08, piman (Very slow to review) wrote:
> nit: remove DCHECK as with previous patch?

Done.

https://codereview.chromium.org/645943002/diff/1/cc/resources/resource_provid...
cc/resources/resource_provider.cc:1082:
resource_provider_->gpu_memory_buffer_manager_->AllocateGpuMemoryBuffer(
On 2014/10/10 21:52:08, piman (Very slow to review) wrote:
> IIUC, this is called from the raster threads. ResourceProvider isn't really
> thread safe (but GpuMemoryBufferManager is meant to be), so the
> resource_provider_->gpu_memory_buffer_manager_ part of this feels iffy.
> Should we save the GpuMemoryBufferManager pointer in the constructor?
> Or can we make it an enforced rule that
> resource_provider_->gpu_memory_buffer_manager_ can't change (e.g. making it a
> const field - pointer to a non-const GpuMemoryBufferManager of course).

Done. Saving a GpuMemoryBufferManager pointer in the ctor.

[piman, 2014-10-27 19:34:32]

lgtm

[reveman, 2014-10-27 19:42:46]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-10-27 19:44:31]

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

[commit-bot: I haz the power, 2014-10-27 20:33:43]

Committed patchset #2 (id:110001)

[commit-bot: I haz the power, 2014-10-27 20:34:17]

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