Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
302763003:
First version of the PGO recipe. (Closed)
|
Created:
6 years, 7 months ago by Sébastien Marchand Modified:
6 years, 5 months ago Reviewers:
iannucci CC:
chromium-reviews, kjellander-cc_chromium.org, cmp-cc_chromium.org, ilevy-cc_chromium.org, stip+watch_chromium.org Visibility:
Public. |
DescriptionFirst version of the PGO recipe.
BUG=309849
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=281316
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Address Robbie's comments. #
Total comments: 26
Patch Set 3 : Address 2nd round of comments. #Patch Set 4 : Address 2nd round of comments. #
Total comments: 4
Patch Set 5 : Fix the kwargs passed to set_config #
Messages
Total messages: 17 (0 generated)
Hey Robbie, can you please take a quick look at this recipe ? This is still a PoC (there's no real tests etc) but at least it works locally ! I wasn't sure if I should update the chromium main recipe or if I should create a new one, but I think that the cleaner way to do this is by adding this new recipe... This is a really particular build process (instrument/profile/optimize) and this will add a lot of complexity to the main recipe... But you might have another opinion on that :). If this approach lgty I'll add more tests and I'll finish the setup of the Chromium PGO builder.
approach looks good! Adding a new recipe is definitely the right thing to do. https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... File scripts/slave/recipes/chromium_pgo.py (right): https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:65: if api.platform.is_win: can this recipe even run on non-windows? PGO is pretty win-specific, right? https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:77: yield api.python.inline( I think theres api.path.rmcontents which does (basically) this. Maybe you could tweak it to take a glob? https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:97: yield api.chromium.compile(targets=['chrome']) should probably plan to upload the artifacts to google storage as well :) https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:101: return ''.join(c if c.isalnum() else '_' for c in text) You can nest this inside the GenTests function :) https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:108: test = ( nit: I would just yield this directly instead of having the temp var
Thanks, committing ! Hopefully this won't break anything :) https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... File scripts/slave/recipes/chromium_pgo.py (right): https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:65: if api.platform.is_win: On 2014/05/27 20:44:21, iannucci wrote: > can this recipe even run on non-windows? PGO is pretty win-specific, right? PGO isn't really Windows specific (it could potentially be used for ChromeOS) but this recipe isn't intended to be uses on another platform than Windows in its current state... I'll remove the check but is there a way to make sure that this recipe isn't used on another platform ? https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:97: yield api.chromium.compile(targets=['chrome']) On 2014/05/27 20:44:21, iannucci wrote: > should probably plan to upload the artifacts to google storage as well :) I'm not sure at this point... The purpose of this builder is to make sure that the PGO build doesn't break... This can happen if someone introduce a change that make the compiler/linker to derail, or if there's some flakiness in link.exe... (the PGO support is pretty new, so I won't be too surprised if it was the case !). If needed I'll upload the builds to google storage, but presently I don't think that it's super useful. I might also use this recipe to find the best set of benchmarks to run one day... https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:101: return ''.join(c if c.isalnum() else '_' for c in text) On 2014/05/27 20:44:21, iannucci wrote: > You can nest this inside the GenTests function :) Done. https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:108: test = ( On 2014/05/27 20:44:21, iannucci wrote: > nit: I would just yield this directly instead of having the temp var Done.
The CQ bit was checked by sebmarchand@chromium.org
The CQ bit was unchecked by sebmarchand@chromium.org
Oops, wrong CL !
Oops, wrong CL !
Sorry for the long delay but the PGO build was broken so I had to fix it before continuing on this... PTAnL. Any suggestion for the tests ? https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... File scripts/slave/recipes/chromium_pgo.py (right): https://codereview.chromium.org/302763003/diff/20001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:77: yield api.python.inline( On 2014/05/27 20:44:21, iannucci wrote: > I think theres api.path.rmcontents which does (basically) this. Maybe you could > tweak it to take a glob? Done, this is a little bit ugly because glob.glob(foo/*) doesn't match the subdirectories in foo so I had to add an optional filter argument to the function. (I can't pass the glob directly in the path).
friendly ping :)
Lookin' pretty good. Did you get a chance to run it with ./scripts/tools/run_recipe.py yet? https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... File scripts/slave/recipe_modules/chromium/config.py (right): https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipe_modules/chromium/config.py:418: @config_ctx() you should probably depend on a compiler (ninja), 'default_compiler', and either 'shared_library' or 'static_library' https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipe_modules/chromium/config.py:423: c.gyp_env.GYP_DEFINES['fastbuild'] = 0 I would set this by doing fastbuild(c, invert=True) This has some additional error-checking advantages (for example, if someone later tried to apply the fastbuild config, it would be an error, instead of just turning it on). I see some others have done this wrong already in this file :) https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... File scripts/slave/recipe_modules/path/api.py (right): https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipe_modules/path/api.py:236: that match it. specify that this should be a glob pattern appended to path https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... File scripts/slave/recipes/chromium_pgo.py (right): https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:57: args) personally, I would format this like return api.python( 'Telemetry benchmark: %s' % testname, api.path['checkout'].join('tools', 'perf', 'run_benchmark'), ['--profiler=win_pgo_profiler', '--use-live-sites', testname] ) ? https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:63: api.chromium.set_config('chromium', 'Release') flip the order of these two lines. Since 'chromium' depends on 'gclient', this second one will end up setting the config 'chromium' on the gclient module (no, it doesn't make a lot of sense, yes, it needs to be redesigned). Also, you need to use kwargs for additional variables in set_config, like BUILD_CONFIG='Release' https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:66: yield api.bot_update.ensure_checkout(), you'll want to remove these trailing commas https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:67: if not api.step_history.last_step().json.output['did_run']: we can just assert that this always uses bot_update. No reason to add a new recipe which doesn't. https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:71: api.chromium.apply_config('chrome_pgo_instrument') you should probably just set this on line 63 https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:73: yield api.chromium.compile(targets=['chrome']) targets won't be necessary since you set it in the config already. https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:77: api.chromium.c.build_dir, I'm wondering if it wouldn't be better to just have the path be a glob-ish, e.g. api.chromium.c.build_dir.join('*.pgc') Then rmcontents could just append a '*' to the end of argv[1] if argv[1] doesn't have any *'s in it, and always use glob.glob. One less code path. https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:86: api.chromium.apply_config('chrome_pgo_optimize') you'll want 'set_config' here, also will need to pass in BUILD_CONFIG='Release' https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:87: yield api.chromium.runhooks() do we really need to run hooks again? that's sad :( https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:88: yield api.chromium.compile(targets=['chrome']) same re: targets https://chromiumcodereview.appspot.com/302763003/diff/30001/scripts/slave/rec... scripts/slave/recipes/chromium_pgo.py:91: def GenTests(api): it would be good to train the tests and include the expectations in the CL. ./scripts/slave/unittests/recipe_simulation_test.py train chromium_pgo
Address Robbie's comments.
Thanks, PTAnL. (Sorry for the delay) https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipe_mod... File scripts/slave/recipe_modules/chromium/config.py (right): https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipe_mod... scripts/slave/recipe_modules/chromium/config.py:418: @config_ctx() On 2014/06/19 21:37:17, iannucci wrote: > you should probably depend on a compiler (ninja), 'default_compiler', and either > 'shared_library' or 'static_library' Done. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipe_mod... scripts/slave/recipe_modules/chromium/config.py:423: c.gyp_env.GYP_DEFINES['fastbuild'] = 0 On 2014/06/19 21:37:17, iannucci wrote: > I would set this by doing > > fastbuild(c, invert=True) > > This has some additional error-checking advantages (for example, if someone > later tried to apply the fastbuild config, it would be an error, instead of just > turning it on). > > I see some others have done this wrong already in this file :) Done. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipe_mod... File scripts/slave/recipe_modules/path/api.py (right): https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipe_mod... scripts/slave/recipe_modules/path/api.py:236: that match it. On 2014/06/19 21:37:17, iannucci wrote: > specify that this should be a glob pattern appended to path I've removed this. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... File scripts/slave/recipes/chromium_pgo.py (right): https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:57: args) On 2014/06/19 21:37:17, iannucci wrote: > personally, I would format this like > > return api.python( > 'Telemetry benchmark: %s' % testname, > api.path['checkout'].join('tools', 'perf', 'run_benchmark'), > ['--profiler=win_pgo_profiler', '--use-live-sites', testname] > ) > > ? Shorter and more readable, I like that. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:63: api.chromium.set_config('chromium', 'Release') On 2014/06/19 21:37:17, iannucci wrote: > flip the order of these two lines. Since 'chromium' depends on 'gclient', this > second one will end up setting the config 'chromium' on the gclient module (no, > it doesn't make a lot of sense, yes, it needs to be redesigned). > > Also, you need to use kwargs for additional variables in set_config, like > BUILD_CONFIG='Release' Done. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:66: yield api.bot_update.ensure_checkout(), On 2014/06/19 21:37:17, iannucci wrote: > you'll want to remove these trailing commas Oops, thanks. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:71: api.chromium.apply_config('chrome_pgo_instrument') On 2014/06/19 21:37:17, iannucci wrote: > you should probably just set this on line 63 Done. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:73: yield api.chromium.compile(targets=['chrome']) On 2014/06/19 21:37:17, iannucci wrote: > targets won't be necessary since you set it in the config already. Done. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:77: api.chromium.c.build_dir, On 2014/06/19 21:37:17, iannucci wrote: > I'm wondering if it wouldn't be better to just have the path be a glob-ish, e.g. > api.chromium.c.build_dir.join('*.pgc') > > Then rmcontents could just append a '*' to the end of argv[1] if argv[1] doesn't > have any *'s in it, and always use glob.glob. One less code path. Done.Looks like there's a rmwildcard function now ! https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:86: api.chromium.apply_config('chrome_pgo_optimize') On 2014/06/19 21:37:17, iannucci wrote: > you'll want 'set_config' here, also will need to pass in BUILD_CONFIG='Release' Done. https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:87: yield api.chromium.runhooks() On 2014/06/19 21:37:17, iannucci wrote: > do we really need to run hooks again? that's sad :( Yep, we have to regenerate the build (.ninja) files as the compiler options change... https://codereview.chromium.org/302763003/diff/30001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:88: yield api.chromium.compile(targets=['chrome']) On 2014/06/19 21:37:17, iannucci wrote: > same re: targets Done.
I think you happen to be lucky because BUILD_CONFIG defaults to 'Release' :) Other than that, lgtm! https://codereview.chromium.org/302763003/diff/90001/scripts/slave/recipes/ch... File scripts/slave/recipes/chromium_pgo.py (right): https://codereview.chromium.org/302763003/diff/90001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:56: { 'BUILD_CONFIG': 'Release' }) Just `BUILD_CONFIG='Release'` https://codereview.chromium.org/302763003/diff/90001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:76: api.chromium.set_config('chrome_pgo_optimize', { 'BUILD_CONFIG': 'Release' }) same deal w/ kwargs. Right now the second parameter is being interpreted as a boolean 'optional' value. Function def is here: https://code.google.com/p/chromium/codesearch#chromium/tools/build/scripts/sl... Man I wish python had static types :/
Thanks, committing. Now I've to figure out how to make sure that this recipe get used by the PGO bot... https://codereview.chromium.org/302763003/diff/90001/scripts/slave/recipes/ch... File scripts/slave/recipes/chromium_pgo.py (right): https://codereview.chromium.org/302763003/diff/90001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:56: { 'BUILD_CONFIG': 'Release' }) On 2014/07/03 19:31:41, iannucci wrote: > Just `BUILD_CONFIG='Release'` Done. https://codereview.chromium.org/302763003/diff/90001/scripts/slave/recipes/ch... scripts/slave/recipes/chromium_pgo.py:76: api.chromium.set_config('chrome_pgo_optimize', { 'BUILD_CONFIG': 'Release' }) On 2014/07/03 19:31:41, iannucci wrote: > same deal w/ kwargs. Right now the second parameter is being interpreted as a > boolean 'optional' value. Function def is here: > https://code.google.com/p/chromium/codesearch#chromium/tools/build/scripts/sl... > > Man I wish python had static types :/ Thanks for the explanation, fixed :)
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/302763003/110001
Message was sent while issue was closed.
Change committed as 281316 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
