Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10027026:
Major refactoring for run_perf_tests.py. (Closed)
|
Created:
8 years, 8 months ago by Emily Fortuna Modified:
8 years, 8 months ago Reviewers:
vsm CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionFix to generate correct graphs.
Patch Set 1 : #Patch Set 2 : #
Total comments: 21
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Messages
Total messages: 6 (0 generated)
Thanks for cleaning this up! I like where this is headed. First wave of comments. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:35: DART_INSTALL_LOCATION = abspath(os.path.join(dirname(abspath(__file__)), I'd hoist this up with TOOLS_PATH. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:69: output, not_used = p.communicate(std_in); Not sure how standard this is, but I like: output, _ = .... when I don't care about a value in the tuple. Up to you. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:71: print output Perhaps print only if self.verbose? https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:486: # we wish to graph. :-) https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:571: class BenchpressAndCoCommandLineTest(RuntimePerformanceTest): "BenchpressAndCo" is a mouthful (fingerpressfull?). Maybe "Core" or "Common" or something like that? https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:579: BenchpressAndCoCommandLineTest.name(), ['commandline'], I could be wrong, but I think you could write self.name(), etc., here. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1179: f.close() Extra newline for new class. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1201: DromaeoSizeTest.name(): DromaeoSizeTest, Can you get rid of the redundancy here? E.g., something like: available_suites = dict((suite.name(), suite) for suite in [ BenchpressAndCoCommandLineTest, CompileTimeAndSizeTest. ... ]) https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1204: def make_test(self, test_name, test_runner): Why is this an instance method but not the below? https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1215: runner.parse_args() I suggest changing this method to only return continuous. Every other value is only passed right back to runner and just clutters this method.
PTAL https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:35: DART_INSTALL_LOCATION = abspath(os.path.join(dirname(abspath(__file__)), On 2012/04/09 20:19:58, vsm wrote: > I'd hoist this up with TOOLS_PATH. Done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:69: output, not_used = p.communicate(std_in); On 2012/04/09 20:19:58, vsm wrote: > Not sure how standard this is, but I like: > output, _ = .... > when I don't care about a value in the tuple. Up to you. Done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:71: print output On 2012/04/09 20:19:58, vsm wrote: > Perhaps print only if self.verbose? This is how we get all of our output in our trace files, and error messages if something goes terribly wrong. I think we want to keep this around pretty much always. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:571: class BenchpressAndCoCommandLineTest(RuntimePerformanceTest): On 2012/04/09 20:19:58, vsm wrote: > "BenchpressAndCo" is a mouthful (fingerpressfull?). Maybe "Core" or "Common" or > something like that? Done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:579: BenchpressAndCoCommandLineTest.name(), ['commandline'], On 2012/04/09 20:19:58, vsm wrote: > I could be wrong, but I think you could write self.name(), etc., here. Done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1179: f.close() On 2012/04/09 20:19:58, vsm wrote: > Extra newline for new class. Done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1201: DromaeoSizeTest.name(): DromaeoSizeTest, On 2012/04/09 20:19:58, vsm wrote: > Can you get rid of the redundancy here? E.g., something like: > > available_suites = dict((suite.name(), suite) for suite in [ > BenchpressAndCoCommandLineTest, > CompileTimeAndSizeTest. > ... > ]) Ah, much nicer. done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1204: def make_test(self, test_name, test_runner): On 2012/04/09 20:19:58, vsm wrote: > Why is this an instance method but not the below? Done. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:1215: runner.parse_args() On 2012/04/09 20:19:58, vsm wrote: > I suggest changing this method to only return continuous. Every other value is > only passed right back to runner and just clutters this method. Done.
LGTM with another style comment. https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:71: print output Isn't trace file output going directly to the "out" file defined above? But, I think it's fine to keep this as is if you prefer. On 2012/04/09 21:15:03, Emily Fortuna wrote: > On 2012/04/09 20:19:58, vsm wrote: > > Perhaps print only if self.verbose? > > This is how we get all of our output in our trace files, and error messages if > something goes terribly wrong. I think we want to keep this around pretty much > always. https://chromiumcodereview.appspot.com/10027026/diff/1006/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10027026/diff/1006/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:585: self.name(), ['commandline'], You can replace all uses of CommonCommandLineTest. with self. below as well. I don't see a mandate either way in the Python style guide, so up to you. But, if you don't change the other cases, please switch this back to be consistent.
https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10027026/diff/8002/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:71: print output On 2012/04/09 21:50:18, vsm wrote: > Isn't trace file output going directly to the "out" file defined above? > > But, I think it's fine to keep this as is if you prefer. > > On 2012/04/09 21:15:03, Emily Fortuna wrote: > > On 2012/04/09 20:19:58, vsm wrote: > > > Perhaps print only if self.verbose? > > > > This is how we get all of our output in our trace files, and error messages if > > something goes terribly wrong. I think we want to keep this around pretty much > > always. > Yes, you're right. This is one of those cases where I like to actually see that the script is actually making progress instead of hanging. https://chromiumcodereview.appspot.com/10027026/diff/1006/tools/testing/perf_... File tools/testing/perf_testing/run_perf_tests.py (right): https://chromiumcodereview.appspot.com/10027026/diff/1006/tools/testing/perf_... tools/testing/perf_testing/run_perf_tests.py:585: self.name(), ['commandline'], On 2012/04/09 21:50:18, vsm wrote: > You can replace all uses of CommonCommandLineTest. with self. below as well. I > don't see a mandate either way in the Python style guide, so up to you. But, if > you don't change the other cases, please switch this back to be consistent. Done.
lgtm! |
