Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10095016:
Add Dartium to browser_perf tests (Closed)
|
Created:
8 years, 8 months ago by vsm Modified:
8 years, 8 months ago Reviewers:
Emily Fortuna CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd Dartium to browser_perf tests
Committed: https://code.google.com/p/dart/source/detail?r=6696
Patch Set 1 #
Total comments: 5
Patch Set 2 : Check for valid combos #
Total comments: 6
Patch Set 3 : Address comments #Messages
Total messages: 7 (0 generated)
https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... tools/testing/perf_testing/run_perf_tests.py:410: self.test.test_runner.run_cmd(['echo', 'Revision: ' + revision], outfile) Do you want to print out a slightly different output here to distinguish between dartium revisions and our repo revisions? https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... tools/testing/perf_testing/run_perf_tests.py:434: geo_mean += math.log(self.test.values_dict[platform][variant][benchmark][ > 80 char Also, why add the try/pass? If you're getting an error, that's indicative that we're not processing the files correctly. I did notice -- in my "FileProcessor" objects, I deal with error handling -- if the trace is incomplete, but I have some data, I fill in the remaining ones with 0 (or whatever is appropriate). You may want to add similar failure resistance in the Dromaeo FileProcessors.
https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... tools/testing/perf_testing/run_perf_tests.py:434: geo_mean += math.log(self.test.values_dict[platform][variant][benchmark][ Ahh, I meant to clean this up. Because we're skipping the platform=dartium, variant=js combo, this fails. I can keep track of valid combo and check here. WDYT? On 2012/04/16 19:22:15, Emily Fortuna wrote: > > 80 char > Also, why add the try/pass? If you're getting an error, that's indicative that > we're not processing the files correctly. I did notice -- in my "FileProcessor" > objects, I deal with error handling -- if the trace is incomplete, but I have > some data, I fill in the remaining ones with 0 (or whatever is appropriate). You > may want to add similar failure resistance in the Dromaeo FileProcessors.
https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... tools/testing/perf_testing/run_perf_tests.py:434: geo_mean += math.log(self.test.values_dict[platform][variant][benchmark][ On 2012/04/16 20:41:08, vsm wrote: > Ahh, I meant to clean this up. Because we're skipping the platform=dartium, > variant=js combo, this fails. > > I can keep track of valid combo and check here. WDYT? > > On 2012/04/16 19:22:15, Emily Fortuna wrote: > > > 80 char > > Also, why add the try/pass? If you're getting an error, that's indicative that > > we're not processing the files correctly. I did notice -- in my > "FileProcessor" > > objects, I deal with error handling -- if the trace is incomplete, but I have > > some data, I fill in the remaining ones with 0 (or whatever is appropriate). > You > > may want to add similar failure resistance in the Dromaeo FileProcessors. > That can work.
https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10095016/diff/1/tools/testing/perf_tes... tools/testing/perf_testing/run_perf_tests.py:434: geo_mean += math.log(self.test.values_dict[platform][variant][benchmark][ Done. PTAL. On 2012/04/16 20:50:17, Emily Fortuna wrote: > On 2012/04/16 20:41:08, vsm wrote: > > Ahh, I meant to clean this up. Because we're skipping the platform=dartium, > > variant=js combo, this fails. > > > > I can keep track of valid combo and check here. WDYT? > > > > On 2012/04/16 19:22:15, Emily Fortuna wrote: > > > > 80 char > > > Also, why add the try/pass? If you're getting an error, that's indicative > that > > > we're not processing the files correctly. I did notice -- in my > > "FileProcessor" > > > objects, I deal with error handling -- if the trace is incomplete, but I > have > > > some data, I fill in the remaining ones with 0 (or whatever is appropriate). > > You > > > may want to add similar failure resistance in the Dromaeo FileProcessors. > > > > That can work.
lgtm, with comments, please. My apologies for sitting on this and not getting back to you quickly! https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:442: geo_mean += math.log(self.test.values_dict[platform][variant][benchmark][ 80 char. https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:592: clear_axis = False This line also needs to be indented two more spaces. https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:692: # TODO(vsm): It's inconvenient that run_cmd isn't in scope here. Perhaps in any "BrowserTest" just before it is actually run, the Tester performs any set up needed, such as downloading an updated version fo DRT.
https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:442: geo_mean += math.log(self.test.values_dict[platform][variant][benchmark][ On 2012/04/17 19:59:29, Emily Fortuna wrote: > 80 char. Done. https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:592: clear_axis = False Thanks for the catch! On 2012/04/17 19:59:29, Emily Fortuna wrote: > This line also needs to be indented two more spaces. https://chromiumcodereview.appspot.com/10095016/diff/5002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:692: # TODO(vsm): It's inconvenient that run_cmd isn't in scope here. This does mean checking multiple times on a single run and potentially using different versions for different Testers. On 2012/04/17 19:59:29, Emily Fortuna wrote: > Perhaps in any "BrowserTest" just before it is actually run, the Tester performs > any set up needed, such as downloading an updated version fo DRT. |
