gpu: Add out-of-line copy ctors for complex classes.

gpu: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

BUG=436357
R=piman@chromium.org, dcheng@chromium.org, thakis@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/3b7b8b27fd11096c0eaff14c44656481b669be5e
Cr-Commit-Position: refs/heads/master@{#378590}

[vmpstr, 2016-02-24 00:34:37]

Description was changed from

==========
gpu: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

BUG=436357
R=piman@chromium.org, dcheng@chromium.org, thakis@chromium.org
==========

to

==========
gpu: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

BUG=436357
R=piman@chromium.org, dcheng@chromium.org, thakis@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[vmpstr, 2016-02-24 00:36:20]

Please take a look.

[piman, 2016-02-24 01:06:57]

Overall lgtm, but please watch perf bots - at least one of them (SyncToken)
could be in the critical path (a few tens per frame - one per resource - copied
probably at least a few times each) and I'm not sure of the impact if the copy
is not trivial any more.

https://codereview.chromium.org/1725113002/diff/1/gpu/command_buffer/common/s...
File gpu/command_buffer/common/sync_token.h (right):

https://codereview.chromium.org/1725113002/diff/1/gpu/command_buffer/common/s...
gpu/command_buffer/common/sync_token.h:34: SyncToken(const SyncToken& other);
Ouch, this one could be in the critical path - please watch perf bots...

[Nico, 2016-02-24 01:13:00]

lgtm2

[vmpstr, 2016-02-25 00:07:02]

On 2016/02/24 01:06:57, piman wrote:
> Overall lgtm, but please watch perf bots - at least one of them (SyncToken)
> could be in the critical path (a few tens per frame - one per resource -
copied
> probably at least a few times each) and I'm not sure of the impact if the copy
> is not trivial any more.
> 
>
https://codereview.chromium.org/1725113002/diff/1/gpu/command_buffer/common/s...
> File gpu/command_buffer/common/sync_token.h (right):
> 
>
https://codereview.chromium.org/1725113002/diff/1/gpu/command_buffer/common/s...
> gpu/command_buffer/common/sync_token.h:34: SyncToken(const SyncToken& other);
> Ouch, this one could be in the critical path - please watch perf bots...

Ok, I'll watch the perf... Do you know off the top of your head what a good
tests to watch is? Ie where would there be the most of these sync points?

[piman, 2016-02-25 06:37:51]

On Wed, Feb 24, 2016 at 4:07 PM, <vmpstr@chromium.org> wrote:

> On 2016/02/24 01:06:57, piman wrote:
> > Overall lgtm, but please watch perf bots - at least one of them
> (SyncToken)
> > could be in the critical path (a few tens per frame - one per resource -
> copied
> > probably at least a few times each) and I'm not sure of the impact if
> the copy
> > is not trivial any more.
> >
> >
>
>
https://codereview.chromium.org/1725113002/diff/1/gpu/command_buffer/common/s...
> > File gpu/command_buffer/common/sync_token.h (right):
> >
> >
>
>
https://codereview.chromium.org/1725113002/diff/1/gpu/command_buffer/common/s...
> > gpu/command_buffer/common/sync_token.h:34: SyncToken(const SyncToken&
> other);
> > Ouch, this one could be in the critical path - please watch perf bots...
>
> Ok, I'll watch the perf... Do you know off the top of your head what a good
> tests to watch is? Ie where would there be the most of these sync points?
>

Maybe thread_times on tough_compositor_cases? Anything with many layers
(many resources).
Also, maybe cc_perftests, I think some of them also have setups with many
layers.

>
> https://codereview.chromium.org/1725113002/
>

-- 
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.

[vmpstr, 2016-02-29 21:10:54]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-02-29 21:10:54]

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/1725113002/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-29 21:11:18]

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

[commit-bot: I haz the power, 2016-02-29 22:24:43]

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

[commit-bot: I haz the power, 2016-02-29 22:24:44]

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

[vmpstr, 2016-02-29 23:27:53]

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

[commit-bot: I haz the power, 2016-02-29 23:28:30]

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

[commit-bot: I haz the power, 2016-03-01 00:12:09]

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

[commit-bot: I haz the power, 2016-03-01 00:12:10]

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

[vmpstr, 2016-03-01 19:42:10]

On 2016/03/01 00:12:10, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Do you peeps have any idea about the bot failures, by any chance? I'm trying to
reproduce this locally but I can't seem to do it. It's a linker error, which
makes me believe that some component doesn't depend properly on the gpu part
that has sync_tokens. However, it also seems that the other SyncToken
constructors are fine...

I'm at a loss.

[vmpstr, 2016-03-01 19:49:30]

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

[commit-bot: I haz the power, 2016-03-01 19:50:32]

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

[commit-bot: I haz the power, 2016-03-01 20:27:23]

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

[commit-bot: I haz the power, 2016-03-01 20:27:24]

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

[vmpstr, 2016-03-01 21:23:23]

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

[commit-bot: I haz the power, 2016-03-01 21:24:05]

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

[commit-bot: I haz the power, 2016-03-01 22:47:36]

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

[commit-bot: I haz the power, 2016-03-01 22:47:37]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2016-03-01 22:47:47]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-03-01 22:47:47]

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/1725113002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-03-01 22:48:44]

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

[commit-bot: I haz the power, 2016-03-01 23:00:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 23:02:32]

Description was changed from

==========
gpu: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

BUG=436357
R=piman@chromium.org, dcheng@chromium.org, thakis@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
gpu: Add out-of-line copy ctors for complex classes.

This patch adds out of line copy constructors for classes that our
clang-plugin considers heavy. This is an effort to enable copy
constructor checks by default.

BUG=436357
R=piman@chromium.org, dcheng@chromium.org, thakis@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

Committed: https://crrev.com/3b7b8b27fd11096c0eaff14c44656481b669be5e
Cr-Commit-Position: refs/heads/master@{#378590}
==========

[commit-bot: I haz the power, 2016-03-01 23:02:33]

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