Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
962863002:
Add gpu_perftests to perfbots. (Closed)
|
Created:
5 years, 9 months ago by Daniele Castagna Modified:
5 years, 9 months ago Reviewers:
Ken Russell (switch to Gerrit), reveman, shatch, Nico, sullivan, jochen (gone - plz use gerrit) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd gpu_perftests to perfbots.
BUG=423481
Committed: https://crrev.com/4e5999debe76d4551875e71f3ab260d6035beea2
Cr-Commit-Position: refs/heads/master@{#319122}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add Win Nvidia/Intel and Mac OS 10.8/10.9. Remove --single-process-tests flag for Android. #
Total comments: 2
Patch Set 3 : Move gpu_perftests script under "Android Nexus5 Perf" bot defined above. #
Total comments: 3
Patch Set 4 : Make gpu_perftests run sequentially. s/--single-process-tests/--test-launcher-print-test-stdio=alwa… #
Total comments: 4
Patch Set 5 : Add a comment on the tests run serially. Rebase on master. #
Messages
Total messages: 33 (7 generated)
dcastagna@chromium.org changed reviewers: + simonhatch@chromium.org, sullivan@chromium.org
Hi Annie! We're trying to add gpu_perftests to the perf bots now that simonhatch@ landed crrev.com/890653002 I'm not sure if I need to add an isolate test for that binary and if the bots we added are headless or not (gpu_perftests is also measuring gpu times). Could you please advise if this cl is enough or if we need to do anything else? Thank you!
I have a buildbot setup locally, I can patch this in and test :) https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:65: "args": ["gpu_perftests", "--single-process-tests"] Do you need the --single-process-tests for android? https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:69: "Mac Builder": { I don't think you want to run this on a builder.
https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:65: "args": ["gpu_perftests", "--single-process-tests"] On 2015/02/26 at 21:49:56, shatch wrote: > Do you need the --single-process-tests for android? Probably not. I'm removing it. https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:69: "Mac Builder": { On 2015/02/26 at 21:49:56, shatch wrote: > I don't think you want to run this on a builder. I'm going to change it to "Mac 10.8 Perf (1)". I thought any bot with ChromiumPerf as master would be a perf bot but given your comment that's probabily not the case.
+kbr On 2015/02/26 21:47:07, Daniele Castagna wrote: > Hi Annie! > We're trying to add gpu_perftests to the perf bots now that simonhatch@ landed > crrev.com/890653002 > > I'm not sure if I need to add an isolate test for that binary and if the bots we > added are headless or not (gpu_perftests is also measuring gpu times). The perfbots have a KVM connected via VGA port for monitor. Ken, do you know if that has any affect on GPU perf tests? > Could you please advise if this cl is enough or if we need to do anything else? > > > Thank you!
On 2015/02/26 22:05:20, sullivan wrote: > +kbr > > On 2015/02/26 21:47:07, Daniele Castagna wrote: > > Hi Annie! > > We're trying to add gpu_perftests to the perf bots now that simonhatch@ landed > > crrev.com/890653002 > > > > I'm not sure if I need to add an isolate test for that binary and if the bots > we > > added are headless or not (gpu_perftests is also measuring gpu times). > > The perfbots have a KVM connected via VGA port for monitor. Ken, do you know if > that has any affect on GPU perf tests? The Raritan IP KVM is important for the GPU perf bots, in order to make them think a monitor is plugged in and to cause the GPU to be properly recognized and activated. We found this very important for the GPU "correctness" bots -- chromium.gpu, chromium.gpu.fyi, etc.
kbr@chromium.org changed reviewers: + kbr@chromium.org
https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:51: "Win 7 ATI GPU Perf": { Why not run these on the NVIDIA and Intel GPU perf bots too?
https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:69: "Mac Builder": { On 2015/02/26 22:03:28, Daniele Castagna wrote: > On 2015/02/26 at 21:49:56, shatch wrote: > > I don't think you want to run this on a builder. > > I'm going to change it to "Mac 10.8 Perf (1)". I thought any bot with > ChromiumPerf as master would be a perf bot but given your comment that's > probabily not the case. Nah it builds and packages it up for any mac perf bots. See http://build.chromium.org/p/chromium.perf/builders/Mac%20Builder/builds/73223 You probably want both the Mac 10.8 and 10.9 bots, as well as the various windows GPU bots.
https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:51: "Win 7 ATI GPU Perf": { On 2015/02/26 at 22:08:24, Ken Russell wrote: > Why not run these on the NVIDIA and Intel GPU perf bots too? I wanted to make sure it works on one bot before trying on all of them, but since both you and Simon seem confident enough I'm adding them. https://codereview.chromium.org/962863002/diff/1/testing/buildbot/chromium.pe... testing/buildbot/chromium.perf.json:69: "Mac Builder": { On 2015/02/26 at 22:09:23, shatch wrote: > On 2015/02/26 22:03:28, Daniele Castagna wrote: > > On 2015/02/26 at 21:49:56, shatch wrote: > > > I don't think you want to run this on a builder. > > > > I'm going to change it to "Mac 10.8 Perf (1)". I thought any bot with > > ChromiumPerf as master would be a perf bot but given your comment that's > > probabily not the case. > > Nah it builds and packages it up for any mac perf bots. See http://build.chromium.org/p/chromium.perf/builders/Mac%20Builder/builds/73223 > > You probably want both the Mac 10.8 and 10.9 bots, as well as the various windows GPU bots. Done.
So gave this a shot on my local android perf bot, works fine. One thing was that I seemed to need to build the gpu_perftests_apk target explicitly so please make sure that's being built and packaged up with all the perf tests. https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:78: "Android Nexus5 Perf": { We already define the android bots above, just add your scripts there. Also, do you just want to run this on the nexus 5 or all android bots? There's also the nexus 4, 7v2, 10, and moto e
On 2015/02/27 at 18:40:55, simonhatch wrote: > So gave this a shot on my local android perf bot, works fine. One thing was that I seemed to need to build the gpu_perftests_apk target explicitly so please make sure that's being built and packaged up with all the perf tests. > I just tried to remove out/Release and built with ninja -C out/Release. The apk gets built. > https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... > File testing/buildbot/chromium.perf.json (right): > > https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... > testing/buildbot/chromium.perf.json:78: "Android Nexus5 Perf": { > We already define the android bots above, just add your scripts there. > > Also, do you just want to run this on the nexus 5 or all android bots? There's also the nexus 4, 7v2, 10, and moto e
https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:78: "Android Nexus5 Perf": { On 2015/02/27 at 18:40:54, shatch wrote: > We already define the android bots above, just add your scripts there. > Done. > Also, do you just want to run this on the nexus 5 or all android bots? There's also the nexus 4, 7v2, 10, and moto e I think it currently fails on the nexus 7 while it works on the 5. I'd prefer to add the other devices later once I know the test works.
On 2015/02/27 20:09:39, Daniele Castagna wrote: > https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... > File testing/buildbot/chromium.perf.json (right): > > https://codereview.chromium.org/962863002/diff/20001/testing/buildbot/chromiu... > testing/buildbot/chromium.perf.json:78: "Android Nexus5 Perf": { > On 2015/02/27 at 18:40:54, shatch wrote: > > We already define the android bots above, just add your scripts there. > > > > Done. > > > Also, do you just want to run this on the nexus 5 or all android bots? There's > also the nexus 4, 7v2, 10, and moto e > > I think it currently fails on the nexus 7 while it works on the 5. I'd prefer to > add the other devices later once I know the test works. Ahh I see, lgtm
dcastagna@chromium.org changed reviewers: + jochen@chromium.org
+jochen for owners approval.
dcastagna@chromium.org changed reviewers: + thakis@chromium.org
+thakis for owners approval.
https://codereview.chromium.org/962863002/diff/40001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/40001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:52: "args": ["gpu_perftests", "--single-process-tests"] why is the --single-process flag needed?
https://codereview.chromium.org/962863002/diff/40001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/40001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:52: "args": ["gpu_perftests", "--single-process-tests"] On 2015/03/02 at 21:53:35, Nico wrote: > why is the --single-process flag needed? It's to avoid having one test slowing down/interfering other ones. simonhatch@ just informed me that the flag is not needed anymore and the test itself should specify that it should be run sequentially: https://codereview.chromium.org/963203002/ I'm going to change the test and I'll update you as soon as I have a cl for that.
PTAL. https://codereview.chromium.org/962863002/diff/40001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/40001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:52: "args": ["gpu_perftests", "--single-process-tests"] On 2015/03/02 at 22:03:42, Daniele Castagna wrote: > On 2015/03/02 at 21:53:35, Nico wrote: > > why is the --single-process flag needed? > > It's to avoid having one test slowing down/interfering other ones. simonhatch@ just informed me that the flag is not needed anymore and the test itself should specify that it should be run sequentially: https://codereview.chromium.org/963203002/ > > I'm going to change the test and I'll update you as soon as I have a cl for that. I included the change in this CL since it's only one line.
https://codereview.chromium.org/962863002/diff/60001/gpu/perftests/run_all_te... File gpu/perftests/run_all_tests.cc (right): https://codereview.chromium.org/962863002/diff/60001/gpu/perftests/run_all_te... gpu/perftests/run_all_tests.cc:14: return base::LaunchUnitTestsSerially(argc, argv, run_test_suite); Can you add a comment that explains why "Serially" is important? https://codereview.chromium.org/962863002/diff/60001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/60001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:52: "args": ["gpu_perftests", "--test-launcher-print-test-stdio=always"] Same question here: Why is this flag needed? Is this something devs would want to set when running the test too? If so, should ti just be on by default?
https://codereview.chromium.org/962863002/diff/60001/gpu/perftests/run_all_te... File gpu/perftests/run_all_tests.cc (right): https://codereview.chromium.org/962863002/diff/60001/gpu/perftests/run_all_te... gpu/perftests/run_all_tests.cc:14: return base::LaunchUnitTestsSerially(argc, argv, run_test_suite); On 2015/03/02 at 22:27:47, Nico wrote: > Can you add a comment that explains why "Serially" is important? Sure, done. https://codereview.chromium.org/962863002/diff/60001/testing/buildbot/chromiu... File testing/buildbot/chromium.perf.json (right): https://codereview.chromium.org/962863002/diff/60001/testing/buildbot/chromiu... testing/buildbot/chromium.perf.json:52: "args": ["gpu_perftests", "--test-launcher-print-test-stdio=always"] On 2015/03/02 at 22:27:47, Nico wrote: > Same question here: Why is this flag needed? Is this something devs would want to set when running the test too? If so, should ti just be on by default? My hypothesis is that with this flag we make sure perf_test::PrintResult writes directly to stdout. The infrastructure we have for perf tests reads directly from the output of the tests. This is what I'm guessing given the last comment of crrev.com/963203002. If someone from the Chrome Speed Team wants to clarify, I'd be happy to hear that. I tried to run the test locally and I see the perf results on stdout without that flag.
ok, asked there On Mon, Mar 2, 2015 at 2:45 PM, <dcastagna@chromium.org> wrote: > > https://codereview.chromium.org/962863002/diff/60001/gpu/ > perftests/run_all_tests.cc > File gpu/perftests/run_all_tests.cc (right): > > https://codereview.chromium.org/962863002/diff/60001/gpu/ > perftests/run_all_tests.cc#newcode14 > gpu/perftests/run_all_tests.cc:14: return > base::LaunchUnitTestsSerially(argc, argv, run_test_suite); > On 2015/03/02 at 22:27:47, Nico wrote: > >> Can you add a comment that explains why "Serially" is important? >> > > Sure, done. > > https://codereview.chromium.org/962863002/diff/60001/ > testing/buildbot/chromium.perf.json > File testing/buildbot/chromium.perf.json (right): > > https://codereview.chromium.org/962863002/diff/60001/ > testing/buildbot/chromium.perf.json#newcode52 > testing/buildbot/chromium.perf.json:52: "args": ["gpu_perftests", > "--test-launcher-print-test-stdio=always"] > On 2015/03/02 at 22:27:47, Nico wrote: > >> Same question here: Why is this flag needed? Is this something devs >> > would want to set when running the test too? If so, should ti just be on > by default? > > My hypothesis is that with this flag we make sure perf_test::PrintResult > writes directly to stdout. The infrastructure we have for perf tests > reads directly from the output of the tests. This is what I'm guessing > given the last comment of crrev.com/963203002. If someone from the > Chrome Speed Team wants to clarify, I'd be happy to hear that. > > I tried to run the test locally and I see the perf results on stdout > without that flag. > > https://codereview.chromium.org/962863002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm based on the discussion on that other cl
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from simonhatch@chromium.org Link to the patchset: https://codereview.chromium.org/962863002/#ps80001 (title: "Add a comment on the tests run serially. Rebase on master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962863002/80001
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
Adding reveman@ for gpu/perftests owner's approval.
lgtm
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4e5999debe76d4551875e71f3ab260d6035beea2 Cr-Commit-Position: refs/heads/master@{#319122} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
