Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770113003:
gpu: Fix AsyncSwapBuffer tracing. (Closed)
|
Created:
3 years, 9 months ago by Daniele Castagna Modified:
3 years, 9 months ago CC:
chromium-reviews, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptiongpu: Fix AsyncSwapBuffer tracing.
AsyncSwapBuffer tracing assumed that at most one swap-buffer was in flight.
This is not true, since on CrOS we triple buffer and we can have up two
async swap-buffers pending.
The result of this was a confusing trace where one AsyncSwapBuffer trace
would start with swap-buffer timestamp of frame N and end at FinishSwapBuffer
timestamp of frame N - 1.
This CL fixes this issue using an id for every AsyncSwapBuffer trace.
In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised
at the same time in the tracing timeline.
BUG=705196
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2770113003
Cr-Commit-Position: refs/heads/master@{#459901}
Committed: https://chromium.googlesource.com/chromium/src/+/0bebf0ea569d282c3db14f067b5e8fdebf023c34
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address reveman's comments. #Patch Set 3 : signed/unsigned comparison. #
Total comments: 2
Patch Set 4 : Remove queue. #
Total comments: 2
Patch Set 5 : Reveman's nit. #Messages
Total messages: 43 (28 generated)
Description was changed from ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swapbuffer was in flight. This is not true, since on CrOS we triple buffer and we can have up to async swap buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every pending swap buffer tracing. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= ========== to ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swapbuffer was in flight. This is not true, since on CrOS we triple buffer and we can have up to async swap buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every pending swap buffer tracing. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
Description was changed from ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swapbuffer was in flight. This is not true, since on CrOS we triple buffer and we can have up to async swap buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every pending swap buffer tracing. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swapbuffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every pending swap buffer tracing. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swapbuffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every pending swap buffer tracing. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swap-buffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap-buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap-buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every AsyncSwapBuffer trace. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:929: void RegisterTraceAndFinishSwapBuffers(gfx::SwapResult result); how about FinishAsyncSwapBuffers? https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:2375: uint32_t async_swap_id_ = 0; nit: next_async_swap_id_ = 1 https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11890: CHECK_LT(pending_swaps_.size(), 2); nit: s/CHECK_LT/DCHECK_LT/ https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11891: ++async_swap_id_; nit: uint32_t async_swap_id = next_async_swap_id_++; https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:15514: CHECK_LT(pending_swaps_.size(), 2); nit: s/CHECK_LT/DCHECK_LT/ https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:15515: ++async_swap_id_; nit: uint32_t async_swap_id = next_async_swap_id_++; https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:15534: uint32_t oldest_swap_id = pending_swaps_.front(); nit: s/oldest_swap_id/async_swap_id/
https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:929: void RegisterTraceAndFinishSwapBuffers(gfx::SwapResult result); On 2017/03/24 at 06:35:09, reveman wrote: > how about FinishAsyncSwapBuffers? Done. https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:2375: uint32_t async_swap_id_ = 0; On 2017/03/24 at 06:35:09, reveman wrote: > nit: next_async_swap_id_ = 1 Not sure why you prefer to start from 1, but done. https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11890: CHECK_LT(pending_swaps_.size(), 2); On 2017/03/24 at 06:35:09, reveman wrote: > nit: s/CHECK_LT/DCHECK_LT/ Done. https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11891: ++async_swap_id_; On 2017/03/24 at 06:35:09, reveman wrote: > nit: uint32_t async_swap_id = next_async_swap_id_++; Done. https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:15514: CHECK_LT(pending_swaps_.size(), 2); On 2017/03/24 at 06:35:09, reveman wrote: > nit: s/CHECK_LT/DCHECK_LT/ Done. https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:15515: ++async_swap_id_; On 2017/03/24 at 06:35:09, reveman wrote: > nit: uint32_t async_swap_id = next_async_swap_id_++; Done. https://codereview.chromium.org/2770113003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:15534: uint32_t oldest_swap_id = pending_swaps_.front(); On 2017/03/24 at 06:35:10, reveman wrote: > nit: s/oldest_swap_id/async_swap_id/ Done.
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
dcastagna@chromium.org changed reviewers: + piman@chromium.org
+piman for ownership.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2770113003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:2376: std::queue<uint32_t> pending_swaps_; Because the swap ids are pushed (and popped) in order, maybe all you need is to keep track of the number of in-flight swaps, so that you don't need memory allocations / overhead of the queue. The argument is that you have an invariant which is that if pending_swaps_ is not empty, pending_swaps_.front() == next_async_swap_id_ - pending_swaps_.size(), so you don't actually need the contents of the queue (i.e. can track the size with a simple uint32_t).
https://codereview.chromium.org/2770113003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:2376: std::queue<uint32_t> pending_swaps_; Because the swap ids are pushed (and popped) in order, maybe all you need is to keep track of the number of in-flight swaps, so that you don't need memory allocations / overhead of the queue. The argument is that you have an invariant which is that if pending_swaps_ is not empty, pending_swaps_.front() == next_async_swap_id_ - pending_swaps_.size(), so you don't actually need the contents of the queue (i.e. can track the size with a simple uint32_t).
Description was changed from ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swap-buffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap-buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap-buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every AsyncSwapBuffer trace. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swap-buffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap-buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap-buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every AsyncSwapBuffer trace. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG=705196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2770113003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:2376: std::queue<uint32_t> pending_swaps_; On 2017/03/24 at 18:55:02, piman wrote: > Because the swap ids are pushed (and popped) in order, maybe all you need is to keep track of the number of in-flight swaps, so that you don't need memory allocations / overhead of the queue. > The argument is that you have an invariant which is that if pending_swaps_ is not empty, pending_swaps_.front() == next_async_swap_id_ - pending_swaps_.size(), so you don't actually need the contents of the queue (i.e. can track the size with a simple uint32_t). Yes, that works. It was the first iteration. We then thought it'd be clearer with a queue since it added really little overhead and it made it a little bit easier to debug what was going on in terms of swap ids. Changed it back to just two ints.
still lgtm https://codereview.chromium.org/2770113003/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:15532: DCHECK(pending_swaps_); nit: DCHECK_NE(pending_swaps_, 0u)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
https://codereview.chromium.org/2770113003/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2770113003/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:15532: DCHECK(pending_swaps_); On 2017/03/27 at 14:21:22, reveman wrote: > nit: DCHECK_NE(pending_swaps_, 0u) Changed to DCHECK_NE(0u, pending_swaps_).
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2770113003/#ps80001 (title: "Reveman's nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by dcastagna@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490652103828040,
"parent_rev": "6812c7313dd67fcfad6f95d4b5a91cf7dcfda06a", "commit_rev":
"0bebf0ea569d282c3db14f067b5e8fdebf023c34"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swap-buffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap-buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap-buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every AsyncSwapBuffer trace. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG=705196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== gpu: Fix AsyncSwapBuffer tracing. AsyncSwapBuffer tracing assumed that at most one swap-buffer was in flight. This is not true, since on CrOS we triple buffer and we can have up two async swap-buffers pending. The result of this was a confusing trace where one AsyncSwapBuffer trace would start with swap-buffer timestamp of frame N and end at FinishSwapBuffer timestamp of frame N - 1. This CL fixes this issue using an id for every AsyncSwapBuffer trace. In this way multiple (at most 2 hopefully) AsyncSwapBuffers can be visualised at the same time in the tracing timeline. BUG=705196 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2770113003 Cr-Commit-Position: refs/heads/master@{#459901} Committed: https://chromium.googlesource.com/chromium/src/+/0bebf0ea569d282c3db14f067b5e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0bebf0ea569d282c3db14f067b5e... |
