Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347163003:
Pass through in dual_browser_story for disabled benchmarks. (Closed)
DescriptionPass through in dual_browser_story for disabled benchmarks.
BUG=chromium:647714
Committed: https://crrev.com/5a722ee58ed81164ebb33a4d66c6fe19b01c051f
Cr-Commit-Position: refs/heads/master@{#422171}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Responding to review comments #
Total comments: 2
Patch Set 3 : Fixing logging statement #Messages
Total messages: 23 (4 generated)
eyaich@chromium.org changed reviewers: + nednguyen@google.com, perezju@chromium.org
https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:120: logging.warning('Valid chartjson ouput will not be generated.') This will effectively cause the benchmark to run even if it's disabled, no? Causing it to fail on devices where it cannot run. Is there an alternative to make this produce a valid disabled chartjson?
https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:120: logging.warning('Valid chartjson ouput will not be generated.') On 2016/09/29 13:01:45, perezju wrote: > This will effectively cause the benchmark to run even if it's disabled, no? > Causing it to fail on devices where it cannot run. > > Is there an alternative to make this produce a valid disabled chartjson? Well it won't fail, it will still technically succeed it just wont upload results and the json will return as results['enabled'] = False. As I mentioned in the team meeting yesterday, this is the only difference people should see with this new logic and we may want to do something like print DISABLED in the step or something since it will appear green even though it didn't actually run. I don't see an alternative to this as we made a conscious decision not to dynamically generate the set of perf benchmarks to run but instead run them all on each bot and just ignore the disabled ones.
https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:120: logging.warning('Valid chartjson ouput will not be generated.') On 2016/09/29 13:05:30, eyaich1 wrote: > On 2016/09/29 13:01:45, perezju wrote: > > This will effectively cause the benchmark to run even if it's disabled, no? > > Causing it to fail on devices where it cannot run. > > > > Is there an alternative to make this produce a valid disabled chartjson? > > Well it won't fail, it will still technically succeed it just wont upload > results and the json will return as results['enabled'] = False. > > As I mentioned in the team meeting yesterday, this is the only difference people > should see with this new logic and we may want to do something like print > DISABLED in the step or something since it will appear green even though it > didn't actually run. > > I don't see an alternative to this as we made a conscious decision not to > dynamically generate the set of perf benchmarks to run but instead run them all > on each bot and just ignore the disabled ones. Hmhh, why not define ShouldDisable(..) for the benchmarks that are using this story?
On 2016/09/29 13:08:40, nednguyen wrote: > https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... > File tools/perf/page_sets/dual_browser_story.py (right): > > https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... > tools/perf/page_sets/dual_browser_story.py:120: logging.warning('Valid chartjson > ouput will not be generated.') > On 2016/09/29 13:05:30, eyaich1 wrote: > > On 2016/09/29 13:01:45, perezju wrote: > > > This will effectively cause the benchmark to run even if it's disabled, no? > > > Causing it to fail on devices where it cannot run. > > > > > > Is there an alternative to make this produce a valid disabled chartjson? > > > > Well it won't fail, it will still technically succeed it just wont upload > > results and the json will return as results['enabled'] = False. > > > > As I mentioned in the team meeting yesterday, this is the only difference > people > > should see with this new logic and we may want to do something like print > > DISABLED in the step or something since it will appear green even though it > > didn't actually run. > > > > I don't see an alternative to this as we made a conscious decision not to > > dynamically generate the set of perf benchmarks to run but instead run them > all > > on each bot and just ignore the disabled ones. > > > Hmhh, why not define ShouldDisable(..) for the benchmarks that are using this > story? Well, this is actually broken. To clarify, the *intended* behavior is that: 1. Benchmarks using this shared state may be disabled either using a decorator, or by overriding ShouldDisable. 2. If the benchmark is disabled, the test should not run, but it should output a valid chartjson with "disabled: True" in it; i.e. what Emily is trying to accomplish here. Now, _CheckTestEnabled in this class is supposed to be the implementation needed to make (1) work, but it doesn't. In particular, the memory.dual_browser_test benchmark is currently disabled for all with a decorator [1], however $ tools/perf/run_benchmark memory.dual_browser_test --browser android-chrome causes the benchmark to start running. But even if (1) would work correctly, I think this CL is not enough to achieve (2), it just lets the benchmark continue running. [1]: https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks...
Looks like the benchmark disabling mechanism is broken everywhere, also on the regular shared page state, e.g. $ tools/perf/run_benchmark smoothness.tough_pinch_zoom_cases --browser android-chrome causes the benchmark to run, even if it is disabled for all in https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks...
On 2016/09/29 13:37:35, perezju wrote: > On 2016/09/29 13:08:40, nednguyen wrote: > > > https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... > > File tools/perf/page_sets/dual_browser_story.py (right): > > > > > https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... > > tools/perf/page_sets/dual_browser_story.py:120: logging.warning('Valid > chartjson > > ouput will not be generated.') > > On 2016/09/29 13:05:30, eyaich1 wrote: > > > On 2016/09/29 13:01:45, perezju wrote: > > > > This will effectively cause the benchmark to run even if it's disabled, > no? > > > > Causing it to fail on devices where it cannot run. > > > > > > > > Is there an alternative to make this produce a valid disabled chartjson? > > > > > > Well it won't fail, it will still technically succeed it just wont upload > > > results and the json will return as results['enabled'] = False. > > > > > > As I mentioned in the team meeting yesterday, this is the only difference > > people > > > should see with this new logic and we may want to do something like print > > > DISABLED in the step or something since it will appear green even though it > > > didn't actually run. > > > > > > I don't see an alternative to this as we made a conscious decision not to > > > dynamically generate the set of perf benchmarks to run but instead run them > > all > > > on each bot and just ignore the disabled ones. > > > > > > Hmhh, why not define ShouldDisable(..) for the benchmarks that are using this > > story? > > Well, this is actually broken. > > To clarify, the *intended* behavior is that: > > 1. Benchmarks using this shared state may be disabled either using a decorator, > or by overriding ShouldDisable. > > 2. If the benchmark is disabled, the test should not run, but it should output > a valid chartjson with "disabled: True" in it; i.e. what Emily is trying to > accomplish here. > > Now, _CheckTestEnabled in this class is supposed to be the implementation needed > to make (1) work, but it doesn't. In particular, the memory.dual_browser_test > benchmark is currently disabled for all with a decorator [1], however > > $ tools/perf/run_benchmark memory.dual_browser_test --browser android-chrome > > causes the benchmark to start running. > > But even if (1) would work correctly, I think this CL is not enough to > achieve (2), it just lets the benchmark continue running. > > [1]: > https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks... Oh. Is that because we plan to run this benchmark on internal waterfall and not the perf one? If so, since the config of internal waterfall is manually created, couldn't we just enable this on bots that can run it & not worry about early failure handling at page_set level?
On 2016/09/29 13:44:24, perezju wrote: > Looks like the benchmark disabling mechanism is broken everywhere, also on the > regular shared page state, e.g. > > $ tools/perf/run_benchmark smoothness.tough_pinch_zoom_cases --browser > android-chrome > > causes the benchmark to run, even if it is disabled for all in > https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks... Ooops, this seems very very bad.
On 2016/09/29 13:47:10, nednguyen wrote: > On 2016/09/29 13:44:24, perezju wrote: > > Looks like the benchmark disabling mechanism is broken everywhere, also on the > > regular shared page state, e.g. > > > > $ tools/perf/run_benchmark smoothness.tough_pinch_zoom_cases --browser > > android-chrome > > > > causes the benchmark to run, even if it is disabled for all in > > > https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks... > > Ooops, this seems very very bad. Yes this is a bug once my change goes in especially since I am no longer filtering out based on the decorator and the story_runner only looks at the ShouldDisable method. I will create a separate CL with this fix and this won't be checked in until that is complete.
On 2016/09/29 14:07:24, eyaich1 wrote: > On 2016/09/29 13:47:10, nednguyen wrote: > > On 2016/09/29 13:44:24, perezju wrote: > > > Looks like the benchmark disabling mechanism is broken everywhere, also on > the > > > regular shared page state, e.g. > > > > > > $ tools/perf/run_benchmark smoothness.tough_pinch_zoom_cases --browser > > > android-chrome > > > > > > causes the benchmark to run, even if it is disabled for all in > > > > > > https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks... > > > > Ooops, this seems very very bad. > > Yes this is a bug once my change goes in especially since I am no longer > filtering out based on the decorator and the story_runner only looks at the > ShouldDisable method. I will create a separate CL with this fix and this won't > be checked in until that is complete. Thanks! That sounds great.
On 2016/09/29 14:32:12, perezju wrote: > On 2016/09/29 14:07:24, eyaich1 wrote: > > On 2016/09/29 13:47:10, nednguyen wrote: > > > On 2016/09/29 13:44:24, perezju wrote: > > > > Looks like the benchmark disabling mechanism is broken everywhere, also on > > the > > > > regular shared page state, e.g. > > > > > > > > $ tools/perf/run_benchmark smoothness.tough_pinch_zoom_cases --browser > > > > android-chrome > > > > > > > > causes the benchmark to run, even if it is disabled for all in > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/tools/perf/benchmarks... > > > > > > Ooops, this seems very very bad. > > > > Yes this is a bug once my change goes in especially since I am no longer > > filtering out based on the decorator and the story_runner only looks at the > > ShouldDisable method. I will create a separate CL with this fix and this > won't > > be checked in until that is complete. > > Thanks! That sounds great. The fix went in for the correctly checking for a disabled benchmark. Can you PTAL at this one to move the swarming along? Thanks!
https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:82: self._CheckTestEnabled(test, possible_browser) I think now you can remove this check together with the method definition below; no need for it anymore. https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:92: sys.exit(0) Could you make this one die hard instead by raising an exception? Now it should be an error to attempt to force-run this benchmark on a device without the WebView shell installed.
https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:82: self._CheckTestEnabled(test, possible_browser) On 2016/09/30 13:13:29, perezju wrote: > I think now you can remove this check together with the method definition below; > no need for it anymore. Why did we ever need it? Would the execution ever get here on a disabled benchmark given that it would go through the check in benchmark_runner and/or story_runner? Was this just a safety check? I agree it is not needed though, removed. https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:92: sys.exit(0) On 2016/09/30 13:13:29, perezju wrote: > Could you make this one die hard instead by raising an exception? Now it should > be an error to attempt to force-run this benchmark on a device without the > WebView shell installed. Done.
lgtm w/nit, thanks! https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/1/tools/perf/page_sets/dual_b... tools/perf/page_sets/dual_browser_story.py:82: self._CheckTestEnabled(test, possible_browser) On 2016/09/30 13:22:35, eyaich1 wrote: > On 2016/09/30 13:13:29, perezju wrote: > > I think now you can remove this check together with the method definition > below; > > no need for it anymore. > > Why did we ever need it? Would the execution ever get here on a disabled > benchmark given that it would go through the check in benchmark_runner and/or > story_runner? Was this just a safety check? > > I agree it is not needed though, removed. I think I added it here _because_ of the other bug you fixed; probably because I noticed that the benchmark was trying to run even when it was disabled. https://codereview.chromium.org/2347163003/diff/20001/tools/perf/page_sets/du... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/20001/tools/perf/page_sets/du... tools/perf/page_sets/dual_browser_story.py:85: logging.warning('Skipping %s (%s) because %s browser is not available', nit: Skipping -> Can't run
Thanks! https://codereview.chromium.org/2347163003/diff/20001/tools/perf/page_sets/du... File tools/perf/page_sets/dual_browser_story.py (right): https://codereview.chromium.org/2347163003/diff/20001/tools/perf/page_sets/du... tools/perf/page_sets/dual_browser_story.py:85: logging.warning('Skipping %s (%s) because %s browser is not available', On 2016/09/30 13:28:01, perezju wrote: > nit: Skipping -> Can't run Done.
On 2016/09/30 13:32:22, eyaich1 wrote: > Thanks! > > https://codereview.chromium.org/2347163003/diff/20001/tools/perf/page_sets/du... > File tools/perf/page_sets/dual_browser_story.py (right): > > https://codereview.chromium.org/2347163003/diff/20001/tools/perf/page_sets/du... > tools/perf/page_sets/dual_browser_story.py:85: logging.warning('Skipping %s (%s) > because %s browser is not available', > On 2016/09/30 13:28:01, perezju wrote: > > nit: Skipping -> Can't run > > Done. lgtm
The CQ bit was checked by eyaich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org Link to the patchset: https://codereview.chromium.org/2347163003/#ps40001 (title: "Fixing logging statement")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Pass through in dual_browser_story for disabled benchmarks. BUG=chromium:647714 ========== to ========== Pass through in dual_browser_story for disabled benchmarks. BUG=chromium:647714 Committed: https://crrev.com/5a722ee58ed81164ebb33a4d66c6fe19b01c051f Cr-Commit-Position: refs/heads/master@{#422171} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5a722ee58ed81164ebb33a4d66c6fe19b01c051f Cr-Commit-Position: refs/heads/master@{#422171} |
