Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1687353002:
Force time elapsed queries on certain drivers. (Closed)
DescriptionForce time elapsed queries on certain drivers.
Certain drivers, particularly Mac drivers and ANGLE when running on the
D3D11 backend do not support timestamp queries despite implementing an
extension that has them in the specification (ARB_timer_query and
EXT_disjoint_timer_query). To indicate this, the driver returns 0 when
queried for the size of the timestamps they return using glGetQueryiv.
Additional logic was added to the GPUTimingImpl class to check for that
when using those extensions and to fall back to the time elapsed query
implementation of timestamps if the driver does not have native support.
BUG=587173
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/71c79ec0e0742a0b4478f33ed9ef9cb50cc4df55
Cr-Commit-Position: refs/heads/master@{#376049}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix nits #
Total comments: 1
Patch Set 3 : Small refactoring and check for time elapsed support #Patch Set 4 : Move to lazy implementation and fix tests #Patch Set 5 : Add bug to commit message #
Total comments: 2
Patch Set 6 : Refactor tests #
Total comments: 2
Patch Set 7 : nits #
Total comments: 3
Patch Set 8 : Initial patch #Patch Set 9 : Initial patch #Messages
Total messages: 46 (24 generated)
The CQ bit was checked by ewell@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687353002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687353002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG= ========== to ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG= ==========
ewell@google.com changed reviewers: + dyen@chromium.org, kbr@chromium.org
The CQ bit was checked by geofflang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687353002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687353002/1
https://codereview.chromium.org/1687353002/diff/1/ui/gl/gpu_timing.cc File ui/gl/gpu_timing.cc (right): https://codereview.chromium.org/1687353002/diff/1/ui/gl/gpu_timing.cc#newcode335 ui/gl/gpu_timing.cc:335: // Certain drivers such as Mac drivers and ANGLE's D3D11 backend do not nit: assert that type is GPUTiming::kTimerTypeDisjoint or GPUTiming::kTimerTypeARB. https://codereview.chromium.org/1687353002/diff/1/ui/gl/gpu_timing.cc#newcode340 ui/gl/gpu_timing.cc:340: if (timestamp_bits == 0) { nit: return timestamp_bits == 0;
Do we have to query the counter bits for GL_TIME_ELAPSED as well? I suppose if that is 0 we should just disable timer queries completely and set the timer type to be invalid. https://codereview.chromium.org/1687353002/diff/20001/ui/gl/gpu_timing.cc File ui/gl/gpu_timing.cc (right): https://codereview.chromium.org/1687353002/diff/20001/ui/gl/gpu_timing.cc#new... ui/gl/gpu_timing.cc:340: GLint timestamp_bits; Could we just make these 3 lines a static function outside of the class? Then just call that function in after lines 316 and 318?
Description was changed from ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG= ========== to ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by geofflang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687353002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG=587173 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/1687353002/diff/80001/ui/gl/gpu_timing_fake.cc File ui/gl/gpu_timing_fake.cc (right): https://codereview.chromium.org/1687353002/diff/80001/ui/gl/gpu_timing_fake.c... ui/gl/gpu_timing_fake.cc:83: bool query_timestamp_counter_bits) { Whether or not timestamp counter bits is actually queries is leaking too much implementation detail, we would basically have to update that boolean as we update our implementation. Since we don't really particularly care about it just do an EXPECT_CALL at the top of the function just like the "GenQueries" at the top and make it always set it to 64 (IE. Copy and paste the code above and remove ".Times(Exactly(1))"). This makes it so you don't have to change anything else either. If we end up writing a test that does test we do the right thing when the query counter bits is 0, we can just write a test that mocks that function within the test itself.
The CQ bit was checked by ewell@google.com to run a CQ dry run
https://codereview.chromium.org/1687353002/diff/80001/ui/gl/gpu_timing_fake.cc File ui/gl/gpu_timing_fake.cc (right): https://codereview.chromium.org/1687353002/diff/80001/ui/gl/gpu_timing_fake.c... ui/gl/gpu_timing_fake.cc:83: bool query_timestamp_counter_bits) { On 2016/02/17 19:04:55, David Yen wrote: > Whether or not timestamp counter bits is actually queries is leaking too much > implementation detail, we would basically have to update that boolean as we > update our implementation. Since we don't really particularly care about it just > do an EXPECT_CALL at the top of the function just like the "GenQueries" at the > top and make it always set it to 64 (IE. Copy and paste the code above and > remove ".Times(Exactly(1))"). This makes it so you don't have to change anything > else either. > > If we end up writing a test that does test we do the right thing when the query > counter bits is 0, we can just write a test that mocks that function within the > test itself. Done.
The CQ bit was unchecked by ewell@google.com
The CQ bit was checked by ewell@google.com
The CQ bit was unchecked by ewell@google.com
The CQ bit was checked by ewell@google.com
The CQ bit was unchecked by ewell@google.com
The CQ bit was checked by ewell@google.com to run a CQ dry run
On 2016/02/17 21:06:30, Ian Ewell wrote: > https://codereview.chromium.org/1687353002/diff/80001/ui/gl/gpu_timing_fake.cc > File ui/gl/gpu_timing_fake.cc (right): > > https://codereview.chromium.org/1687353002/diff/80001/ui/gl/gpu_timing_fake.c... > ui/gl/gpu_timing_fake.cc:83: bool query_timestamp_counter_bits) { > On 2016/02/17 19:04:55, David Yen wrote: > > Whether or not timestamp counter bits is actually queries is leaking too much > > implementation detail, we would basically have to update that boolean as we > > update our implementation. Since we don't really particularly care about it > just > > do an EXPECT_CALL at the top of the function just like the "GenQueries" at the > > top and make it always set it to 64 (IE. Copy and paste the code above and > > remove ".Times(Exactly(1))"). This makes it so you don't have to change > anything > > else either. > > > > If we end up writing a test that does test we do the right thing when the > query > > counter bits is 0, we can just write a test that mocks that function within > the > > test itself. > > Done.
The CQ bit was unchecked by ewell@google.com
The CQ bit was checked by ewell@google.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687353002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687353002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
LGTM with nits. https://codereview.chromium.org/1687353002/diff/100001/ui/gl/gpu_timing_fake.cc File ui/gl/gpu_timing_fake.cc (left): https://codereview.chromium.org/1687353002/diff/100001/ui/gl/gpu_timing_fake.... ui/gl/gpu_timing_fake.cc:84: // Time Stamp based queries. Was this accidentally deleted? https://codereview.chromium.org/1687353002/diff/100001/ui/gl/gpu_timing_fake.cc File ui/gl/gpu_timing_fake.cc (right): https://codereview.chromium.org/1687353002/diff/100001/ui/gl/gpu_timing_fake.... ui/gl/gpu_timing_fake.cc:129: .Times(Exactly(1)) nit: Also remove this ".Times(Exactly(1))"
dyen@chromium.org changed reviewers: + sievers@chromium.org
+sievers@ for command_buffer review
I had to ask David about how this fallback path worked (estimating GPU timestamps with CPU timestamps) but understanding it a bit better now, lgtm.
https://codereview.chromium.org/1687353002/diff/120001/ui/gl/gpu_timing.cc File ui/gl/gpu_timing.cc (right): https://codereview.chromium.org/1687353002/diff/120001/ui/gl/gpu_timing.cc#ne... ui/gl/gpu_timing.cc:408: timestamp_bit_count_gl_ = QueryTimestampBits(); Do you have to do that every time though? Isn't GL_QUERY_COUNTER_BITS for GL_TIMESTAMP more of a static property?
https://codereview.chromium.org/1687353002/diff/120001/ui/gl/gpu_timing.cc File ui/gl/gpu_timing.cc (right): https://codereview.chromium.org/1687353002/diff/120001/ui/gl/gpu_timing.cc#ne... ui/gl/gpu_timing.cc:408: timestamp_bit_count_gl_ = QueryTimestampBits(); On 2016/02/17 23:08:45, sievers wrote: > Do you have to do that every time though? Isn't GL_QUERY_COUNTER_BITS for > GL_TIMESTAMP more of a static property? It only lazily initializes it once. It's done lazily so contexts which do not query timestamps do not have to query the counter.
lgtm
https://codereview.chromium.org/1687353002/diff/120001/ui/gl/gpu_timing.cc File ui/gl/gpu_timing.cc (right): https://codereview.chromium.org/1687353002/diff/120001/ui/gl/gpu_timing.cc#ne... ui/gl/gpu_timing.cc:408: timestamp_bit_count_gl_ = QueryTimestampBits(); On 2016/02/17 23:08:45, sievers wrote: > Do you have to do that every time though? Isn't GL_QUERY_COUNTER_BITS for > GL_TIMESTAMP more of a static property? It is only called once and then timestamp_bit_count_gl is updated so that it is never called again. It is done here as it affects the fewest amount of tests (putting it in the constructor broke twice as many tests before the fixes to the tests.
The CQ bit was checked by ewell@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dyen@chromium.org Link to the patchset: https://codereview.chromium.org/1687353002/#ps120001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1687353002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1687353002/120001
Message was sent while issue was closed.
Description was changed from ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG=587173 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG=587173 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG=587173 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Force time elapsed queries on certain drivers. Certain drivers, particularly Mac drivers and ANGLE when running on the D3D11 backend do not support timestamp queries despite implementing an extension that has them in the specification (ARB_timer_query and EXT_disjoint_timer_query). To indicate this, the driver returns 0 when queried for the size of the timestamps they return using glGetQueryiv. Additional logic was added to the GPUTimingImpl class to check for that when using those extensions and to fall back to the time elapsed query implementation of timestamps if the driver does not have native support. BUG=587173 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/71c79ec0e0742a0b4478f33ed9ef9cb50cc4df55 Cr-Commit-Position: refs/heads/master@{#376049} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/71c79ec0e0742a0b4478f33ed9ef9cb50cc4df55 Cr-Commit-Position: refs/heads/master@{#376049} |
