cc: Allocate resources on worker context.

cc: Allocate resources on worker context.

ScopedWriteLockGL will not pre-allocate its resource if async worker
context (gpu scheduler) is enabled. Instead it will only create a GL id
and mailbox for the resource on the compositor context. Later on the
worker context, ScopedTextureProvider creates a GL id and allocates the
resource if necessary. This works with both GL and GMB resources.

This CL also includes minor cleanup in resource provider code.

R=piman
BUG=514819
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[sunnyps, 2017-05-15 19:19:59]

Description was changed from

==========
WIP Allocate resources on worker context

BUG=
==========

to

==========
WIP Allocate resources on worker context

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunnyps, 2017-05-16 00:37:11]

Description was changed from

==========
WIP Allocate resources on worker context

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Allocate resources on worker context.

ScopedWriteLockGL will not pre-allocate its resource if async worker
context (gpu scheduler) is enabled. Instead it will only create a GL id
and mailbox for the resource on the compositor context. Later on the
worker context, ScopedTextureProvider creates a GL id and allocates the
resource if necessary. This works with both GL and GMB resources.

This CL also includes minor cleanup in resource provider code.

R=piman
BUG=514819
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunnyps, 2017-05-16 00:37:17]

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

[commit-bot: I haz the power, 2017-05-16 00:38:37]

Dry run: CQ is trying da patch.

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

[sunnyps, 2017-05-16 00:52:29]

Description was changed from

==========
cc: Allocate resources on worker context.

ScopedWriteLockGL will not pre-allocate its resource if async worker
context (gpu scheduler) is enabled. Instead it will only create a GL id
and mailbox for the resource on the compositor context. Later on the
worker context, ScopedTextureProvider creates a GL id and allocates the
resource if necessary. This works with both GL and GMB resources.

This CL also includes minor cleanup in resource provider code.

R=piman
BUG=514819
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Allocate resources on worker context.

ScopedWriteLockGL will not pre-allocate its resource if async worker
context (gpu scheduler) is enabled. Instead it will only create a GL id
and mailbox for the resource on the compositor context. Later on the
worker context, ScopedTextureProvider creates a GL id and allocates the
resource if necessary. This works with both GL and GMB resources.

This CL also includes minor cleanup in resource provider code.

R=piman
BUG=514819
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sunnyps, 2017-05-16 01:30:35]

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

[commit-bot: I haz the power, 2017-05-16 01:31:35]

Dry run: CQ is trying da patch.

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

[sunnyps, 2017-05-16 01:34:50]

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

[sunnyps, 2017-05-16 01:34:50]

piman PTAL

vmiura, ericrk FYI PTAL if you like

[commit-bot: I haz the power, 2017-05-16 01:47:59]

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

[commit-bot: I haz the power, 2017-05-16 01:48:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-05-17 20:36:38]

Please hold off on l-g-t-m-ing this.

There's a bug with GMB resources with async worker context disabled.

[piman, 2017-05-17 20:58:58]

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:311: ResourceProvider::Resource* resource_;
I don't think it's safe to keep a pointer to the Resource, if we're going to
access it from another thread. It lives in an unordered_map, and operations on
that map (even if they touch other elements) could invalidate the iterators due
to rehashing.
That would be even more likely to occur if we changed the map to a flat_map
(which could make sense given the number of elements and typical operations on
them).

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:736: void
LazyAllocate(ResourceProvider::Resource* resource,
nit: no need for ResourceProvider::

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:740: void
LazyAllocateGpuMemoryBuffer(ResourceProvider::Resource* resource);
nit: ditto

[sunnyps, 2017-05-17 22:01:35]

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:311: ResourceProvider::Resource* resource_;
On 2017/05/17 20:58:58, piman wrote:
> I don't think it's safe to keep a pointer to the Resource, if we're going to
> access it from another thread. It lives in an unordered_map, and operations on
> that map (even if they touch other elements) could invalidate the iterators
due
> to rehashing.

unordered_map doesn't invalidate references except when removing that particular
item. It invalidates iterators on insert when growing the map but not
references. The behavior is similar to vector<forward_list<T>>.

In fact, we used pointers until a change I made last year:
https://codereview.chromium.org/1951193002/

> That would be even more likely to occur if we changed the map to a flat_map
> (which could make sense given the number of elements and typical operations on
> them).

I agree that using pointers limits the choice of containers we can use. Do you
think it's acceptable to copy the entire Resource struct into the lock? Copying
only parts of the struct seemed to complicate the code.

[piman, 2017-05-17 22:08:11]

On 2017/05/17 22:01:35, sunnyps wrote:
>
https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
> File cc/resources/resource_provider.h (right):
> 
>
https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
> cc/resources/resource_provider.h:311: ResourceProvider::Resource* resource_;
> On 2017/05/17 20:58:58, piman wrote:
> > I don't think it's safe to keep a pointer to the Resource, if we're going to
> > access it from another thread. It lives in an unordered_map, and operations
on
> > that map (even if they touch other elements) could invalidate the iterators
> due
> > to rehashing.
> 
> unordered_map doesn't invalidate references except when removing that
particular
> item. It invalidates iterators on insert when growing the map but not
> references. The behavior is similar to vector<forward_list<T>>.
> 
> In fact, we used pointers until a change I made last year:
> https://codereview.chromium.org/1951193002/
> 
> > That would be even more likely to occur if we changed the map to a flat_map
> > (which could make sense given the number of elements and typical operations
on
> > them).
> 
> I agree that using pointers limits the choice of containers we can use. Do you
> think it's acceptable to copy the entire Resource struct into the lock?
Copying
> only parts of the struct seemed to complicate the code.

Yeah, I was thinking that could be an option. SGTM.
If going that route though, and still using functions from ResourceProvider on
the raster threads, can you make sure to document which ones are meant to be
thread-safe - if possible by making them static when it makes sense.

[vmiura, 2017-05-17 22:13:09]

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

[vmiura, 2017-05-17 22:13:10]

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:311: ResourceProvider::Resource* resource_;
On 2017/05/17 22:01:35, sunnyps wrote:
> On 2017/05/17 20:58:58, piman wrote:
> > I don't think it's safe to keep a pointer to the Resource, if we're going to
> > access it from another thread. It lives in an unordered_map, and operations
on
> > that map (even if they touch other elements) could invalidate the iterators
> due
> > to rehashing.
> 
> unordered_map doesn't invalidate references except when removing that
particular
> item. It invalidates iterators on insert when growing the map but not
> references. The behavior is similar to vector<forward_list<T>>.
> 
> In fact, we used pointers until a change I made last year:
> https://codereview.chromium.org/1951193002/

I think pointers can be invalidated when the items are rehashed.  See Iterator
invalidation table.

http://en.cppreference.com/w/cpp/container/unordered_map

[sunnyps, 2017-05-17 22:14:49]

Iterators can be but not references or pointers. From that same page, below
the iterator invalidation table:

   - References and pointers to either key or data stored in the container
   are only invalidated by erasing that element, even when the corresponding
   iterator is invalidated.


On Wed, May 17, 2017 at 3:13 PM <vmiura@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
> File cc/resources/resource_provider.h (right):
>
>
>
https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
> cc/resources/resource_provider.h:311: ResourceProvider::Resource*
> resource_;
> On 2017/05/17 22:01:35, sunnyps wrote:
> > On 2017/05/17 20:58:58, piman wrote:
> > > I don't think it's safe to keep a pointer to the Resource, if we're
> going to
> > > access it from another thread. It lives in an unordered_map, and
> operations on
> > > that map (even if they touch other elements) could invalidate the
> iterators
> > due
> > > to rehashing.
> >
> > unordered_map doesn't invalidate references except when removing that
> particular
> > item. It invalidates iterators on insert when growing the map but not
> > references. The behavior is similar to vector<forward_list<T>>.
> >
> > In fact, we used pointers until a change I made last year:
> > https://codereview.chromium.org/1951193002/
>
> I think pointers can be invalidated when the items are rehashed. See
> Iterator invalidation table.
>
> http://en.cppreference.com/w/cpp/container/unordered_map
>
> https://codereview.chromium.org/2885533002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sunnyps, 2017-05-27 02:15:50]

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

[commit-bot: I haz the power, 2017-05-27 02:15:59]

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-05-27 02:26:39]

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

[commit-bot: I haz the power, 2017-05-27 02:26:40]

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

[sunnyps, 2017-05-27 02:38:31]

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

[commit-bot: I haz the power, 2017-05-27 02:38:43]

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-05-27 03:21:05]

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

[commit-bot: I haz the power, 2017-05-27 03:21:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sunnyps, 2017-05-31 01:22:23]

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

[commit-bot: I haz the power, 2017-05-31 01:23:11]

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-05-31 02:15:09]

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

[commit-bot: I haz the power, 2017-05-31 02:15:10]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  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-31 19:57:45]

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

[commit-bot: I haz the power, 2017-05-31 19:58:43]

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-05-31 21:07:27]

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

[commit-bot: I haz the power, 2017-05-31 21:07:28]

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

[sunnyps, 2017-06-01 00:28:25]

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

[commit-bot: I haz the power, 2017-06-01 00:29: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-06-01 02:40:47]

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

[commit-bot: I haz the power, 2017-06-01 02:40:48]

Dry run: This issue passed the CQ dry run.

[sunnyps, 2017-06-01 22:07:22]

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

[commit-bot: I haz the power, 2017-06-01 22:08:33]

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-06-02 01:24:20]

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

[commit-bot: I haz the power, 2017-06-02 01:24:21]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunnyps, 2017-06-08 00:28:53]

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

[commit-bot: I haz the power, 2017-06-08 00:29:26]

Dry run: CQ is trying da patch.

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

[sunnyps, 2017-06-08 00:30:07]

PTAL

Addressed all comments and added tests for ScopedWriteLockGL (both async and
default). This CL implements worker context allocation for both GL texture and
GMB resources.

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/2885533002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.h:311: ResourceProvider::Resource* resource_;
On 2017/05/17 22:13:10, vmiura wrote:
> On 2017/05/17 22:01:35, sunnyps wrote:
> > On 2017/05/17 20:58:58, piman wrote:
> > > I don't think it's safe to keep a pointer to the Resource, if we're going
to
> > > access it from another thread. It lives in an unordered_map, and
operations
> on
> > > that map (even if they touch other elements) could invalidate the
iterators
> > due
> > > to rehashing.
> > 
> > unordered_map doesn't invalidate references except when removing that
> particular
> > item. It invalidates iterators on insert when growing the map but not
> > references. The behavior is similar to vector<forward_list<T>>.
> > 
> > In fact, we used pointers until a change I made last year:
> > https://codereview.chromium.org/1951193002/
> 
> I think pointers can be invalidated when the items are rehashed.  See Iterator
> invalidation table.
> 
> http://en.cppreference.com/w/cpp/container/unordered_map

Changed this to not use Resource*. Although it's not a problem now because of
unordered_map semantics, requiring pointers to not be invalidated severely
restricts the choice of containers.

[commit-bot: I haz the power, 2017-06-08 00:38:18]

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

[commit-bot: I haz the power, 2017-06-08 00:38:19]

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

[piman, 2017-06-09 22:14:20]

https://codereview.chromium.org/2885533002/diff/260001/cc/resources/resource_...
File cc/resources/resource_provider.cc (left):

https://codereview.chromium.org/2885533002/diff/260001/cc/resources/resource_...
cc/resources/resource_provider.cc:1681: resource->SetSynchronized();
Why are we removing this one? Won't this cause extra WaitSyncTokens when we
reuse the resource?

https://codereview.chromium.org/2885533002/diff/260001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2885533002/diff/260001/cc/resources/resource_...
cc/resources/resource_provider.cc:1150: bool async_worker_context_enabled)
nit: I'm not sure the parameter name is just right, because we pass false in
ResourceProvider::CopyToResource, even though async workers might very well be
enabled.
Maybe something like "defer_resource_allocation" or something?

https://codereview.chromium.org/2885533002/diff/260001/cc/resources/resource_...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/2885533002/diff/260001/cc/resources/resource_...
cc/resources/resource_provider_unittest.cc:3899: false);
nit: you may want to add Mock::VerifyAndClearExpectations(context) here and
other places to make sure the expectations have happened by now (i.e. are not
deferred until a later point).