Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368353003:
Add a script to run the PGO benchmarks. (Closed)
|
Created:
4 years, 2 months ago by Sébastien Marchand Modified:
4 years, 2 months ago Reviewers:
scottmg CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a script to run the PGO benchmarks.
This is mostly a copy of https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/pgo/resources/run_benchmark.py (which will disappear)
I think that it make more sense to keep this script in the Chrome repo, so people can easily do a local PGO build.
BUG=309849
Committed: https://crrev.com/f3595515a0d2f5345ce56ba276077ad8694c2521
Cr-Commit-Position: refs/heads/master@{#421276}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address Scott's comment. #
Total comments: 2
Patch Set 3 : . #Messages
Total messages: 14 (6 generated)
Description was changed from ========== Add a script to run the PGO benchmarks. BUG=309849 ========== to ========== Add a script to run the PGO benchmarks. This is mostly a copy of https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/pgo/resou... (which will disappear) I think that it make more sense to keep this script in the Chrome repo, so people can easily do a local PGO build. BUG=309849 ==========
sebmarchand@chromium.org changed reviewers: + scottmg@chromium.org
PTAL.
https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... File build/win/run_pgo_profiling_benchmarks.py (right): https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:4: """Utility script to run the benchmarks during the profiling step of a PGO nit; blank line after copyright before block comment. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:56: """Find the directory containing pgosweep.exe.""" nit; " target_bits should be 32 or 64." since I probably would have passed "x86" or "x64". :) https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:65: raise Exception('The toolchain JSON file is invalid.') nit; 'The toolchain JSON file's "path" entry (%s) does not refer to a path' or something. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:81: if not pgo_sweep_dir: I don't think this can return empty/None because the error conditions all raise, so you can just remove this. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:104: # TODO(sebmarchand): Make this run in parallel. [Seems like you wouldn't want to run benchmarks in parallel because it'd thrash things/change timings? Dunno.] https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:121: subprocess.call(benchmark_command, env=env) check_call maybe? https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:123: continue log something about exception maybe? https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:134: parser.add_option('--target-bits', help='The target\'s bitness.', type=int) Maybe "target_cpu" and make it a string that's "x86" or "x64" to make it match GN if you plan to do this.
Thanks! PTAnL. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... File build/win/run_pgo_profiling_benchmarks.py (right): https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:4: """Utility script to run the benchmarks during the profiling step of a PGO On 2016/09/27 17:39:30, scottmg wrote: > nit; blank line after copyright before block comment. Done. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:56: """Find the directory containing pgosweep.exe.""" On 2016/09/27 17:39:30, scottmg wrote: > nit; " target_bits should be 32 or 64." since I probably would have passed "x86" > or "x64". :) Done, added a check as well. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:65: raise Exception('The toolchain JSON file is invalid.') On 2016/09/27 17:39:30, scottmg wrote: > nit; 'The toolchain JSON file's "path" entry (%s) does not refer to a path' or > something. Done. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:81: if not pgo_sweep_dir: On 2016/09/27 17:39:30, scottmg wrote: > I don't think this can return empty/None because the error conditions all raise, > so you can just remove this. Ha, good point! https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:104: # TODO(sebmarchand): Make this run in parallel. On 2016/09/27 17:39:30, scottmg wrote: > [Seems like you wouldn't want to run benchmarks in parallel because it'd thrash > things/change timings? Dunno.] I don't think that it could cause any issue? When I use pgosweep I always specify the process PID, so it shouldn't be an issue. Making the benchmarks run in parallel will really help to shorten the profiling step. The 'benchmarking' part of the benchmarks doesn't matter, I could probably rename this to run_pgo_profiling_scenarios? https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:121: subprocess.call(benchmark_command, env=env) On 2016/09/27 17:39:30, scottmg wrote: > check_call maybe? Done. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:123: continue On 2016/09/27 17:39:30, scottmg wrote: > log something about exception maybe? Done. https://codereview.chromium.org/2368353003/diff/1/build/win/run_pgo_profiling... build/win/run_pgo_profiling_benchmarks.py:134: parser.add_option('--target-bits', help='The target\'s bitness.', type=int) On 2016/09/27 17:39:30, scottmg wrote: > Maybe "target_cpu" and make it a string that's "x86" or "x64" to make it match > GN if you plan to do this. Done.
lgtm (We don't need to --no-sandbox or similar so that the renderer can load their dll, right?) https://codereview.chromium.org/2368353003/diff/20001/build/win/run_pgo_profi... File build/win/run_pgo_profiling_benchmarks.py (right): https://codereview.chromium.org/2368353003/diff/20001/build/win/run_pgo_profi... build/win/run_pgo_profiling_benchmarks.py:66: raise Exception('The toolchain JSON file\'s "path" entry (%s) does not ' You changed the wrong exception here.
Nop, they don't need any particular flag, works with the sandbox! https://codereview.chromium.org/2368353003/diff/20001/build/win/run_pgo_profi... File build/win/run_pgo_profiling_benchmarks.py (right): https://codereview.chromium.org/2368353003/diff/20001/build/win/run_pgo_profi... build/win/run_pgo_profiling_benchmarks.py:66: raise Exception('The toolchain JSON file\'s "path" entry (%s) does not ' On 2016/09/27 18:04:08, scottmg wrote: > You changed the wrong exception here. /facepalm/
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2368353003/#ps40001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a script to run the PGO benchmarks. This is mostly a copy of https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/pgo/resou... (which will disappear) I think that it make more sense to keep this script in the Chrome repo, so people can easily do a local PGO build. BUG=309849 ========== to ========== Add a script to run the PGO benchmarks. This is mostly a copy of https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/pgo/resou... (which will disappear) I think that it make more sense to keep this script in the Chrome repo, so people can easily do a local PGO build. BUG=309849 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add a script to run the PGO benchmarks. This is mostly a copy of https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/pgo/resou... (which will disappear) I think that it make more sense to keep this script in the Chrome repo, so people can easily do a local PGO build. BUG=309849 ========== to ========== Add a script to run the PGO benchmarks. This is mostly a copy of https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/pgo/resou... (which will disappear) I think that it make more sense to keep this script in the Chrome repo, so people can easily do a local PGO build. BUG=309849 Committed: https://crrev.com/f3595515a0d2f5345ce56ba276077ad8694c2521 Cr-Commit-Position: refs/heads/master@{#421276} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f3595515a0d2f5345ce56ba276077ad8694c2521 Cr-Commit-Position: refs/heads/master@{#421276} |
