Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7982007:
add "did it run on GPU" check to frame_rate_tests.cc using AutomationProxy tracing feature (Closed)
|
Created:
9 years, 3 months ago by jbates Modified:
9 years, 1 month ago CC:
chromium-reviews, scheib, joth, Vangelis Kokkevis, jrt Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionadd "did it run on GPU" check to frame_rate_tests.cc using AutomationProxy tracing feature
BUG=95714
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109368
Patch Set 1 #
Total comments: 2
Patch Set 2 : ref build compatibility #Patch Set 3 : new query API #Patch Set 4 : added templated TraceEvent::GetArgAsIntType #
Total comments: 13
Patch Set 5 : . #Patch Set 6 : split out trace_analyzer changes #Patch Set 7 : merged #
Total comments: 4
Patch Set 8 : cleanup as discussed #Patch Set 9 : cleanup as discussed #
Total comments: 4
Patch Set 10 : HasFlag cleanup #Patch Set 11 : more cleanup #
Messages
Total messages: 17 (0 generated)
Split 2/3 from http://codereview.chromium.org/7866026/
Is the plan that test_ items are the only items that show up in tests? Or...? We should add a paragraph or two to the headers to explain the "preferred way to instrument for test."
http://codereview.chromium.org/7982007/diff/1/gpu/command_buffer/service/gles... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): http://codereview.chromium.org/7982007/diff/1/gpu/command_buffer/service/gles... gpu/command_buffer/service/gles2_cmd_decoder.cc:6572: gfx::GetGLImplementation() == gfx::kGLImplementationEGLGLES2); Is the idea here to distinguish between Mesa and "GPU"? Maybe is_mesa? or something more concrete? When we get swiftshader, what does this evolve to?
The "test_" category prefix is meant to be used by tests and ignored by the about:tracing UI. I'm not sure this should be a requirement, but it will help reduce clutter in about:tracing. If an event is useful to both tests and the about:tracing UI, it may be good for performance to not have two versions of it, but this is something that will become more apparent as more tests are written. http://codereview.chromium.org/7982007/diff/1/gpu/command_buffer/service/gles... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): http://codereview.chromium.org/7982007/diff/1/gpu/command_buffer/service/gles... gpu/command_buffer/service/gles2_cmd_decoder.cc:6572: gfx::GetGLImplementation() == gfx::kGLImplementationEGLGLES2); On 2011/10/11 20:37:56, nduca wrote: > Is the idea here to distinguish between Mesa and "GPU"? Maybe is_mesa? or > something more concrete? When we get swiftshader, what does this evolve to? How about I add a ToString for GLImplementation and just put that in the trace?
Nat: PTAL
We need to either roll the builds, or factor this so that there's less of this "if refbuild" pollution. Any ideas how to make this less teh-suck? http://codereview.chromium.org/7982007/diff/11002/base/test/trace_event_analy... File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7982007/diff/11002/base/test/trace_event_analy... base/test/trace_event_analyzer.h:135: // to get it and cast it to your type. Copy the way base-value works? GetArgAsBool/etc. The templating approach can fall over too easily. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:102: using namespace trace_analyzer; why using here only? That feels anomalous. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:121: if (!IsReferenceBuild()) { Why not just update the ref builds now? http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:166: // Check trace for GPU accleration. I can haz new function? http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:187: gpu_suffix = "_swfb"; Will we still have the _comp suffix? WE have an explosion of suffixes here and I think that means we're doing something wrong. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:191: trace_name += GetSuffixForTestFlags(GetParam()) + gpu_suffix; Merge this with the GetSuffixForTest bit? E.g. GetSuffixForTest(GetParams(), did_use_gpu)
I split out the API additions to trace_event_analyzer: http://codereview.chromium.org/8438058/ http://codereview.chromium.org/7982007/diff/11002/base/test/trace_event_analy... File base/test/trace_event_analyzer.h (right): http://codereview.chromium.org/7982007/diff/11002/base/test/trace_event_analy... base/test/trace_event_analyzer.h:135: // to get it and cast it to your type. On 2011/11/01 19:17:49, nduca wrote: > Copy the way base-value works? GetArgAsBool/etc. The templating approach can > fall over too easily. Done. Actually since queries already verify the existence of arguments, I added getters that assume the check for existence has already been made. This makes the test code a little cleaner. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:102: using namespace trace_analyzer; On 2011/11/01 19:17:49, nduca wrote: > why using here only? That feels anomalous. This is actually done in lots of other chromium code (helps to reduce namespace clutter where symbols aren't used). I don't mind either way though. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:121: if (!IsReferenceBuild()) { On 2011/11/01 19:17:49, nduca wrote: > Why not just update the ref builds now? Either this needs to be split up into the test changes and the trace event additions or we commit this with these checks, update ref build, then remove checks. Either way, it consists of three steps. In general, all tests will have this tedious 3-step problem: 1. add trace events. 2. update ref build. 3. add/update test. That, or they need to be designed to handle the case of missing events until the ref build is updated. I'm not sure what the best solution is, it seems icky with the reference build involved no matter what. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:166: // Check trace for GPU accleration. On 2011/11/01 19:17:49, nduca wrote: > I can haz new function? Done. http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:187: gpu_suffix = "_swfb"; On 2011/11/01 19:17:49, nduca wrote: > Will we still have the _comp suffix? WE have an explosion of suffixes here and I > think that means we're doing something wrong. I got rid of swfb, because we can infer that from a missing _gpu. I think we still need _comp as long as there is a chance of mesa or other software GL API (in which case we would see _comp as opposed to _comp_gpu). http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:191: trace_name += GetSuffixForTestFlags(GetParam()) + gpu_suffix; On 2011/11/01 19:17:49, nduca wrote: > Merge this with the GetSuffixForTest bit? E.g. GetSuffixForTest(GetParams(), > did_use_gpu) Done.
PTAL http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/11002/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:121: if (!IsReferenceBuild()) { On 2011/11/03 00:51:07, jbates wrote: > On 2011/11/01 19:17:49, nduca wrote: > > Why not just update the ref builds now? > > Either this needs to be split up into the test changes and the trace event > additions or we commit this with these checks, update ref build, then remove > checks. Either way, it consists of three steps. > > In general, all tests will have this tedious 3-step problem: > 1. add trace events. > 2. update ref build. > 3. add/update test. > > That, or they need to be designed to handle the case of missing events until the > ref build is updated. > > I'm not sure what the best solution is, it seems icky with the reference build > involved no matter what. Actually, thinking this over more... we probably won't need to many more trace events for performance tests, since we can test a lot of perf characteristics with only a few basic rendering and input trace events. I think we should get this in with ref build checks, update the bots, and then do a followup patch to remove the checks. The reason I think that's the best solution is: It's cleaner to write and commit the test + the new trace events in one CL, even if it requires some code that is conditional on IsReferenceBuild, because the test can't be verified without the trace events present. If they go in in stages, it's more likely that the trace events are discovered to be incorrect for some platform after commit or during the try-bot run. I also think it's a more efficient development flow to decouple the reference build update from writing new tests.
LGTM, and thanks for sharing all your thinking on the nits I had before. I agree with your reasoning. http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (left): http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:40: suffix += "_nogpu"; does the _nogpu get affected? Can we reconcile the stuff @junov is trying to achieve with this? Maybe ping him and if you two get to closure I'm happy.
http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:217: trace_name += GetSuffixForTestFlags(GetParam(), DidRunOnGpu(json_events)); Currently, the set of tests that run on the gpu are the tests that are run as part of the gpu_frame_rate test (the gpu waterfall counterpart of the frame_rate test) and do not have the kDisableGpu flag set. I think that rather than adding redundant info to the name, we could instead have a check. something like: bool ranOnGpu = DidRunOnGpu(json_events)); if (CommandLine::ForCurrentProcess()->HasSwitch("enable-gpu") && !(GetParam() & kDisableGpu)) { ASSERT_TRUE(ranOnGpu); } else { ASSERT_FALSE(ranOnGpu); } This way we would get a failure if a test that is expected to run on the GPU ends up not running on the GPU, and vice versa. I think it will be very useful to validate that tests do not run on the GPU when they are not supposed to, like on the perf waterfall. There is a risk that code churn in the command line switches could cause the tests to be executed using mesa, which is not the purpose of the test.
PTAL http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (left): http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:40: suffix += "_nogpu"; On 2011/11/08 00:27:42, nduca wrote: > does the _nogpu get affected? Can we reconcile the stuff @junov is trying to > achieve with this? Maybe ping him and if you two get to closure I'm happy. Done. http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/23001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:217: trace_name += GetSuffixForTestFlags(GetParam(), DidRunOnGpu(json_events)); On 2011/11/08 14:58:43, junov wrote: > Currently, the set of tests that run on the gpu are the tests that are run as > part of the gpu_frame_rate test (the gpu waterfall counterpart of the frame_rate > test) and do not have the kDisableGpu flag set. I think that rather than adding > redundant info to the name, we could instead have a check. something like: > > bool ranOnGpu = DidRunOnGpu(json_events)); > if (CommandLine::ForCurrentProcess()->HasSwitch("enable-gpu") && !(GetParam() & > kDisableGpu)) { > ASSERT_TRUE(ranOnGpu); > } else { > ASSERT_FALSE(ranOnGpu); > } > > This way we would get a failure if a test that is expected to run on the GPU > ends up not running on the GPU, and vice versa. I think it will be very useful > to validate that tests do not run on the GPU when they are not supposed to, like > on the perf waterfall. There is a risk that code churn in the command line > switches could cause the tests to be executed using mesa, which is not the > purpose of the test. Done as discussed.
LGTM, with suggested correction http://codereview.chromium.org/7982007/diff/31001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/31001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:61: bool HasFlag(FrameRateTestFlags flag) const { return (GetParam() & flag); } This test will not work correctly with multi-bit flags like kForceGpuComposited. It should be: return flag == (GetParam() & flag); http://codereview.chromium.org/7982007/diff/31001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:138: ASSERT_TRUE(HasFlag(kUseGpu) || !HasFlag(kForceGpuComposited)); With this assertion in place I think it is not necessary to implement kForceGpuComposited as a multi-bit flag. If you want to go back to a single bit, then you can ignore my previous comment about HasFlag.
Just did both, to be pedantic :) http://codereview.chromium.org/7982007/diff/31001/chrome/test/perf/frame_rate... File chrome/test/perf/frame_rate/frame_rate_tests.cc (right): http://codereview.chromium.org/7982007/diff/31001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:61: bool HasFlag(FrameRateTestFlags flag) const { return (GetParam() & flag); } On 2011/11/09 15:51:23, junov wrote: > This test will not work correctly with multi-bit flags like kForceGpuComposited. > It should be: > return flag == (GetParam() & flag); Done. http://codereview.chromium.org/7982007/diff/31001/chrome/test/perf/frame_rate... chrome/test/perf/frame_rate/frame_rate_tests.cc:138: ASSERT_TRUE(HasFlag(kUseGpu) || !HasFlag(kForceGpuComposited)); On 2011/11/09 15:51:23, junov wrote: > With this assertion in place I think it is not necessary to implement > kForceGpuComposited as a multi-bit flag. If you want to go back to a single bit, > then you can ignore my previous comment about HasFlag. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbates@chromium.org/7982007/36001
Try job failure for 7982007-36001 (retry) (retry) (retry) on win_rel for step "chrome_frame_net_tests". It's a second try, previously, step "chrome_frame_net_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbates@chromium.org/7982007/36001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is win_rel, revision is 109355, job name was 7982007-36001 (retry) (retry) (retry) (retry). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
