Reland of Separate out resource mailbox creation and fix synchronization issue.

Reland of Separate out resource mailbox creation and fix synchronization issue. (patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
>
> Reason for revert:
> Causing renderer synchronization issues
>
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
>
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
>
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

R=piman@chromium.org
BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/38e7e941648b23d1eb34602d63022884c3552e2d
Cr-Commit-Position: refs/heads/master@{#374280}

[David Yen, 2016-02-08 18:28:31]

Created Reland of Separate out resource mailbox creation and fix synchronization
issue.

[David Yen, 2016-02-08 18:29:54]

Description was changed from

==========
Reland of Separate out resource mailbox creation and fix synchronization issue.
(patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization
issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
> 
> Reason for revert:
> Causing renderer synchronization issues
> 
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
> 
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
> 
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

TBR=reveman@chromium.org,piman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=584381, 584780
==========

to

==========
Reland of Separate out resource mailbox creation and fix synchronization issue.
(patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization
issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
> 
> Reason for revert:
> Causing renderer synchronization issues
> 
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
> 
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
> 
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

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

[David Yen, 2016-02-08 18:30:01]

dyen@chromium.org changed reviewers:
- reveman@chromium.org

[piman, 2016-02-08 18:48:27]

On 2016/02/08 18:28:31, David Yen wrote:
> Created Reland of Separate out resource mailbox creation and fix
synchronization
> issue.

What's the difference between the 2 CLs?

[David Yen, 2016-02-08 18:50:00]

On 2016/02/08 18:48:27, piman wrote:
> On 2016/02/08 18:28:31, David Yen wrote:
> > Created Reland of Separate out resource mailbox creation and fix
> synchronization
> > issue.
> 
> What's the difference between the 2 CLs?

Still figuring this out, will ping when ready. I didn't realize the revert
button automatically sent out an email sorry about that.

[David Yen, 2016-02-08 19:07:56]

Description was changed from

==========
Reland of Separate out resource mailbox creation and fix synchronization issue.
(patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization
issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
> 
> Reason for revert:
> Causing renderer synchronization issues
> 
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
> 
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
> 
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

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

to

==========
Reland of Separate out resource mailbox creation and fix synchronization issue.
(patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization
issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
> 
> Reason for revert:
> Causing renderer synchronization issues
> 
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
> 
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
> 
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

R=piman@chromium.org
BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[David Yen, 2016-02-08 19:18:36]

On 2016/02/08 18:50:00, David Yen wrote:
> On 2016/02/08 18:48:27, piman wrote:
> > On 2016/02/08 18:28:31, David Yen wrote:
> > > Created Reland of Separate out resource mailbox creation and fix
> > synchronization
> > > issue.
> > 
> > What's the difference between the 2 CLs?
> 
> Still figuring this out, will ping when ready. I didn't realize the revert
> button automatically sent out an email sorry about that.

Ok patch #3 is ready to review but this is a bit of a disaster. I should have
created a separate "rebase" patch so you can diff just the differences. Let me
try to have a "undo/rebase" patch in #4 and reupload the changes as #5 so you
can diff the changes.

[David Yen, 2016-02-08 19:24:17]

Ok this looks much better, just look at the diff between #4 and #5.

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

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1206: // TODO(dyen): This is necessary because
we do not guarantee all resource
This surprised me a bit but resources can appear in the resource list twice? In
particular I saw this in the delegated renderer. Is that a bug? I can
investigate if so, otherwise this is how it needs to be done to support
duplicate resources (that or filtering it with a set as a pre-process).

[vmpstr, 2016-02-08 20:02:55]

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

[vmpstr, 2016-02-08 20:02:56]

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

https://codereview.chromium.org/1674313002/diff/160001/cc/resources/resource_...
cc/resources/resource_provider.cc:1156:
std::vector<ResourceIdArray::const_iterator> need_synchronization_indexes;
Was something invalidating these iterators or is the change here just a cleanup?

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

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1157: for (const ResourceId it : resources) {
nit: can you rename "it" to "id" or something like that here and whenever the
type is ResourceId in this function.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1189: for (Resource* resource_iter :
need_synchronization_resources) {
I'd prefer if we reserved "iter" and "it" for iterators... I think this can just
be "resource"

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1202:
++resources_.find(it)->second.exported_count;
nit: usually, we'd do something like

DCHECK(resources_.find(it) != resources_.end());
++resources_[it].exported_count;

I find it easier to read. However, that code was there before, so it's up to
you.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1214:
resource->mailbox.set_sync_token(gpu::SyncToken());
So is this just temporary here? It seems that it's resetting whatever sync token
we set on line 1190 isn't it? (Sorry if it's a naive question, I'm just trying
to understand).

[piman, 2016-02-08 21:09:24]

LGTM modulo nits

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

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1194: for (const ResourceId it : resources) {
nit: it->id here too.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1206: // TODO(dyen): This is necessary because
we do not guarantee all resource
On 2016/02/08 19:24:17, David Yen wrote:
> This surprised me a bit but resources can appear in the resource list twice?
In
> particular I saw this in the delegated renderer. Is that a bug? I can
> investigate if so, otherwise this is how it needs to be done to support
> duplicate resources (that or filtering it with a set as a pre-process).

Nothing really says they shouldn't, and in particular I think UI resources can
be used in multiple layers.

[David Yen, 2016-02-08 21:49:24]

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

https://codereview.chromium.org/1674313002/diff/160001/cc/resources/resource_...
cc/resources/resource_provider.cc:1156:
std::vector<ResourceIdArray::const_iterator> need_synchronization_indexes;
On 2016/02/08 20:02:55, vmpstr wrote:
> Was something invalidating these iterators or is the change here just a
cleanup?

Just a cleanup

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

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1157: for (const ResourceId it : resources) {
On 2016/02/08 20:02:56, vmpstr wrote:
> nit: can you rename "it" to "id" or something like that here and whenever the
> type is ResourceId in this function. 

Done.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1189: for (Resource* resource_iter :
need_synchronization_resources) {
On 2016/02/08 20:02:55, vmpstr wrote:
> I'd prefer if we reserved "iter" and "it" for iterators... I think this can
just
> be "resource"

Done.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1194: for (const ResourceId it : resources) {
On 2016/02/08 21:09:24, piman wrote:
> nit: it->id here too.

Done.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1202:
++resources_.find(it)->second.exported_count;
On 2016/02/08 20:02:55, vmpstr wrote:
> nit: usually, we'd do something like
> 
> DCHECK(resources_.find(it) != resources_.end());
> ++resources_[it].exported_count;
> 
> I find it easier to read. However, that code was there before, so it's up to
> you.

I just tried this but the Resource struct doesn't have the necessary constructor
to do bracket operator it looks like. I'll just keep it as is instead of adding
one for this purpose.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1206: // TODO(dyen): This is necessary because
we do not guarantee all resource
On 2016/02/08 21:09:24, piman wrote:
> On 2016/02/08 19:24:17, David Yen wrote:
> > This surprised me a bit but resources can appear in the resource list twice?
> In
> > particular I saw this in the delegated renderer. Is that a bug? I can
> > investigate if so, otherwise this is how it needs to be done to support
> > duplicate resources (that or filtering it with a set as a pre-process).
> 
> Nothing really says they shouldn't, and in particular I think UI resources can
> be used in multiple layers.

Acknowledged.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1214:
resource->mailbox.set_sync_token(gpu::SyncToken());
On 2016/02/08 20:02:56, vmpstr wrote:
> So is this just temporary here? It seems that it's resetting whatever sync
token
> we set on line 1190 isn't it? (Sorry if it's a naive question, I'm just trying
> to understand).

This is the other side of line 1163 where we add missing sync tokens for
resources which do not have any sync tokens. Once the resources are transferred
it no longer needs to be on the source resource. However if the resource is
reused we need to be sure no stale sync tokens are there so we know to fill in
any missing ones.

[vmpstr, 2016-02-08 23:36:52]

lgtm

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

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1202:
++resources_.find(it)->second.exported_count;
On 2016/02/08 21:49:24, David Yen wrote:
> On 2016/02/08 20:02:55, vmpstr wrote:
> > nit: usually, we'd do something like
> > 
> > DCHECK(resources_.find(it) != resources_.end());
> > ++resources_[it].exported_count;
> > 
> > I find it easier to read. However, that code was there before, so it's up to
> > you.
> 
> I just tried this but the Resource struct doesn't have the necessary
constructor
> to do bracket operator it looks like. I'll just keep it as is instead of
adding
> one for this purpose.

Ah makes sense. Thanks.

https://codereview.chromium.org/1674313002/diff/180001/cc/resources/resource_...
cc/resources/resource_provider.cc:1214:
resource->mailbox.set_sync_token(gpu::SyncToken());
On 2016/02/08 21:49:24, David Yen wrote:
> On 2016/02/08 20:02:56, vmpstr wrote:
> > So is this just temporary here? It seems that it's resetting whatever sync
> token
> > we set on line 1190 isn't it? (Sorry if it's a naive question, I'm just
trying
> > to understand).
> 
> This is the other side of line 1163 where we add missing sync tokens for
> resources which do not have any sync tokens. Once the resources are
transferred
> it no longer needs to be on the source resource. However if the resource is
> reused we need to be sure no stale sync tokens are there so we know to fill in
> any missing ones.

Gotcha.

[David Yen, 2016-02-09 00:04:21]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2016-02-09 00:04:21]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1674313002/#ps200001
(title: "Applied fixes")

[commit-bot: I haz the power, 2016-02-09 00:09:15]

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

[commit-bot: I haz the power, 2016-02-09 02:47:00]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-02-09 02:48:13]

Description was changed from

==========
Reland of Separate out resource mailbox creation and fix synchronization issue.
(patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization
issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
> 
> Reason for revert:
> Causing renderer synchronization issues
> 
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
> 
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
> 
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

R=piman@chromium.org
BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Reland of Separate out resource mailbox creation and fix synchronization issue.
(patchset #1 id:1 of https://codereview.chromium.org/1678613002/ )

Reason for revert:
Trying to reland patch with fixes.

Original issue's description:
> Revert of Separate out resource mailbox creation and fix synchronization
issue. (patchset #3 id:40001 of https://codereview.chromium.org/1666203003/ )
>
> Reason for revert:
> Causing renderer synchronization issues
>
> Original issue's description:
> > Separate out resource mailbox creation and fix synchronization issue.
> >
> > The resource transfer function has been split up so it does the lazy
> > resource mailbox creation in a separate pass. This allows us to setup
> > the sync token directly on the resource itself before the transfer.
> >
> > The texture binding code is also done on the first pass, this fixes
> > a subtle synchronization issue where previously the bound textures
> > could potentially not have a sync token attached to it if no mailboxes
> > were lazily created.
> >
> > BUG=584381
> > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> >
> > Committed: https://crrev.com/d5650cab68401bcd2bb89e1a98c65378d493f785
> > Cr-Commit-Position: refs/heads/master@{#373847}
>
> TBR=reveman@chromium.org,piman@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=584381, 584780
>
> Committed: https://crrev.com/a14747b0409da6ca3700c03a0a4e5cb28de9f3b0
> Cr-Commit-Position: refs/heads/master@{#373974}

R=piman@chromium.org
BUG=584381
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/38e7e941648b23d1eb34602d63022884c3552e2d
Cr-Commit-Position: refs/heads/master@{#374280}
==========

[commit-bot: I haz the power, 2016-02-09 02:48:14]

Patchset 6 (id:??) landed as
https://crrev.com/38e7e941648b23d1eb34602d63022884c3552e2d
Cr-Commit-Position: refs/heads/master@{#374280}