Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
257723003:
First version of gpuveto testing script (Closed)
|
Created:
6 years, 7 months ago by robertphillips Modified:
6 years, 7 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionFirst version of gpuveto testing script
This CL just sketches out the structure of the gpuveto testing process. Two big areas for improvement are:
render the picture in tiles and label as unsuitable if any gpu tile is slower than raster
decide on whether tilegrid is used or not
As expected the current gpuveto heuristic isn't so good:
predicted suitable unsuitable
-------------------------------------------
actual suitable 10 17
actual unsuitable 15 27
Committed: http://code.google.com/p/skia/source/detail?r=14416
Patch Set 1 #
Total comments: 21
Patch Set 2 : Addressed code review issues #
Total comments: 12
Patch Set 3 : more code review changes #Patch Set 4 : added new line #Messages
Total messages: 13 (0 generated)
Comments mainly about python style. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py File tools/test_gpuveto.py (right): https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode8 tools/test_gpuveto.py:8: """ """Script to test out suitableForGpuRasterization (via gpuveto).""" https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode31 tools/test_gpuveto.py:31: Returns path to an existing program binary. Keep this line above, eg: """Returns path to an existing program binary. ... """ The first line is a one-line description of the function similarly everywhere below as well. From python's style guide: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode41 tools/test_gpuveto.py:41: Nit: Need 2 newlines between top level definitions (here and everywhere below). https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode46 tools/test_gpuveto.py:46: """ Change above to: """Returns list of all files from the provided argument. @param dir_or_file: .... @returns ... """ https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode66 tools/test_gpuveto.py:66: """ """Executes a process and waits for it to complete. @param args is passed into subprocess.Popen(). @returns a tuple of the process output (returncode, output) """ https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode67 tools/test_gpuveto.py:67: proc = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) Nit: max line length in python is 80. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:110: args = [self.bench_pictures, '-r', skp_file, '--repeat', '20', '--config', '8888'] Nit: max line length in python is 80. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:122: args2 = [self.bench_pictures, '-r', skp_file, '--repeat', '20', '--config', 'gpu'] Nit: max line length in python is 80. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:133: sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % (skp_file, Move all arguments to the next line: sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % ( skp_file, suitable, rasterTime, gpuTime)) https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:150: if not main_argv or main_argv[0] in ['-h', '-?', '-help', '--help']: Could you please use argparse instead for this? It will make it more extensible in the future. Eg: parser = argparse.ArgumentParser() parser.add_argument('--skp_path', help='Path to the SKP(s). Can either be a directory containing SKPs or a single SKP.', required=True) args = parser.parse_args() TestObj().process_skps(list_files(args.skp_path)) https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:156: exit(main(sys.argv[1])) Please use sys.exit(main(sys.argv[1])) instead.
https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py File tools/test_gpuveto.py (right): https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode8 tools/test_gpuveto.py:8: """ On 2014/04/28 16:58:44, rmistry wrote: > """Script to test out suitableForGpuRasterization (via gpuveto).""" Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode31 tools/test_gpuveto.py:31: Returns path to an existing program binary. On 2014/04/28 16:58:44, rmistry wrote: > Keep this line above, eg: > """Returns path to an existing program binary. > > ... > """ > > The first line is a one-line description of the function similarly everywhere > below as well. > From python's style guide: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode41 tools/test_gpuveto.py:41: On 2014/04/28 16:58:44, rmistry wrote: > Nit: Need 2 newlines between top level definitions (here and everywhere below). Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode46 tools/test_gpuveto.py:46: """ On 2014/04/28 16:58:44, rmistry wrote: > Change above to: > > """Returns list of all files from the provided argument. > > @param dir_or_file: .... > > @returns ... > """ Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode66 tools/test_gpuveto.py:66: """ On 2014/04/28 16:58:44, rmistry wrote: > """Executes a process and waits for it to complete. > > @param args is passed into subprocess.Popen(). > @returns a tuple of the process output (returncode, output) > """ Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcode67 tools/test_gpuveto.py:67: proc = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) On 2014/04/28 16:58:44, rmistry wrote: > Nit: max line length in python is 80. Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:110: args = [self.bench_pictures, '-r', skp_file, '--repeat', '20', '--config', '8888'] On 2014/04/28 16:58:44, rmistry wrote: > Nit: max line length in python is 80. Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:122: args2 = [self.bench_pictures, '-r', skp_file, '--repeat', '20', '--config', 'gpu'] On 2014/04/28 16:58:44, rmistry wrote: > Nit: max line length in python is 80. Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:133: sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % (skp_file, On 2014/04/28 16:58:44, rmistry wrote: > Move all arguments to the next line: > > sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % ( > skp_file, suitable, rasterTime, gpuTime)) Done. https://codereview.chromium.org/257723003/diff/1/tools/test_gpuveto.py#newcod... tools/test_gpuveto.py:156: exit(main(sys.argv[1])) On 2014/04/28 16:58:44, rmistry wrote: > Please use sys.exit(main(sys.argv[1])) instead. Done.
Last few comments. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py File tools/test_gpuveto.py (right): https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:22: USAGE = """ No longer needed. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:29: def find_path_to_program(program): Lets delete find_path_to_program and directly use run_binary.find_path_to_program(program) instead. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:70: output, ignore = proc.communicate() Use _ instead of ignore. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:75: class TestObj(object): Rename this to maybe GpuVeto ? https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:140: sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % ( Use single-quotes instead of double-quotes (to be consistent with rest of the file). https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:153: Add one more newline here.
https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py File tools/test_gpuveto.py (right): https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:22: USAGE = """ On 2014/04/28 18:12:04, rmistry wrote: > No longer needed. Done. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:29: def find_path_to_program(program): On 2014/04/28 18:12:04, rmistry wrote: > Lets delete find_path_to_program and directly use > run_binary.find_path_to_program(program) instead. Done. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:70: output, ignore = proc.communicate() On 2014/04/28 18:12:04, rmistry wrote: > Use _ instead of ignore. Done. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:75: class TestObj(object): On 2014/04/28 18:12:04, rmistry wrote: > Rename this to maybe GpuVeto ? Done. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:140: sys.stdout.write("%s: gpuveto: %d raster %.2f gpu: %.2f\n" % ( On 2014/04/28 18:12:04, rmistry wrote: > Use single-quotes instead of double-quotes (to be consistent with rest of the > file). Done. https://codereview.chromium.org/257723003/diff/20001/tools/test_gpuveto.py#ne... tools/test_gpuveto.py:153: On 2014/04/28 18:12:04, rmistry wrote: > Add one more newline here. Done.
LGTM
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/robertphillips@google.com/257723003/4...
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 257723003-40001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** These files should end in a newline character: tools/test_gpuveto.py Was the presubmit check useful? If not, run "git cl presubmit -v" to figure out which PRESUBMIT.py was run, then run git blame on the file to figure out who to ask for help.
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/robertphillips@google.com/257723003/6...
Message was sent while issue was closed.
Change committed as 14416 |
