Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2261713003:
[Telemetry] Add trivial pages page set to BattOr benchmarks. (Closed)
|
Created:
4 years, 4 months ago by rnephew (Reviews Here) Modified:
4 years, 4 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Telemetry] Add trivial pages page set to BattOr benchmarks.
BUG=639308
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq
Committed: https://crrev.com/6175392bf638b00303602cb7a8895fc59e23fd83
Cr-Commit-Position: refs/heads/master@{#414009}
Patch Set 1 #
Total comments: 3
Patch Set 2 : only run on mac #Patch Set 3 : [Telemetry] Add trivial pages page set to BattOr benchmarks. #
Total comments: 8
Patch Set 4 : [Telemetry] Add trivial pages page set to BattOr benchmarks. #
Total comments: 2
Patch Set 5 : [Telemetry] Add trivial pages page set to BattOr benchmarks. #
Total comments: 6
Patch Set 6 : [Telemetry] Add trivial pages page set to BattOr benchmarks. #Patch Set 7 : [Telemetry] Add trivial pages page set to BattOr benchmarks. #Patch Set 8 : Rebase #Messages
Total messages: 37 (10 generated)
Description was changed from ========== [Telemetry] Add trivial pages page set to BattOr benchmarks. BUG=639308 ========== to ========== [Telemetry] Add trivial pages page set to BattOr benchmarks. BUG=639308 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ==========
rnephew@chromium.org changed reviewers: + aschulman@chromium.org, charliea@chromium.org, erikchen@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:121: class BattOrTrivialPages(_BattOrBenchmark): Should I limit this only to mac? Its running fine locally on android.
is it possible to reduce the run time for each page? iirc it's set to 30 sec in this page set and that would be a ton of trace datacand may lead to oom exceptions
On 2016/08/19 16:56:19, aschulman wrote: > is it possible to reduce the run time for each page? iirc it's set to 30 sec in > this page set and that would be a ton of trace datacand may lead to oom > exceptions sure - you can use whatever run time you want! The value comes from here: https://cs-staging.chromium.org/chromium/src/tools/perf/measurements/power.py... but you're not using that measurement, so it shouldn't matter, right?
https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:121: class BattOrTrivialPages(_BattOrBenchmark): On 2016/08/19 16:49:43, rnephew (Reviews Here) wrote: > Should I limit this only to mac? Its running fine locally on android. If you want to run the tests on Android, I recommend using a subset of the pages. Each page is repeated 3 times in the page set, with different mac compositing settings. You could rename mac_gpu_sites to trivial_pages.py, and then perform the appropriate refactor inside the class.
We aren't running that metric, so it isn't running the 30 seconds to get the idle power.
https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... tools/perf/benchmarks/battor.py:121: class BattOrTrivialPages(_BattOrBenchmark): On 2016/08/19 17:01:31, erikchen wrote: > On 2016/08/19 16:49:43, rnephew (Reviews Here) wrote: > > Should I limit this only to mac? Its running fine locally on android. > > If you want to run the tests on Android, I recommend using a subset of the > pages. Each page is repeated 3 times in the page set, with different mac > compositing settings. > > You could rename mac_gpu_sites to trivial_pages.py, and then perform the > appropriate refactor inside the class. Set to only run on mac.
On 2016/08/19 17:06:39, rnephew (Reviews Here) wrote: > https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... > File tools/perf/benchmarks/battor.py (right): > > https://codereview.chromium.org/2261713003/diff/1/tools/perf/benchmarks/batto... > tools/perf/benchmarks/battor.py:121: class BattOrTrivialPages(_BattOrBenchmark): > On 2016/08/19 17:01:31, erikchen wrote: > > On 2016/08/19 16:49:43, rnephew (Reviews Here) wrote: > > > Should I limit this only to mac? Its running fine locally on android. > > > > If you want to run the tests on Android, I recommend using a subset of the > > pages. Each page is repeated 3 times in the page set, with different mac > > compositing settings. > > > > You could rename mac_gpu_sites to trivial_pages.py, and then perform the > > appropriate refactor inside the class. > > Set to only run on mac. I can't tell how long the measurement is going to run for. Is this implicitly defined by imeline_based_measurement?
> I can't tell how long the measurement is going to run for. Is this implicitly > defined by imeline_based_measurement? I really feel like that wait in the power measurement is in a strange place. Most other tests I've seen has waits like that in RunPageInteractions. Currently it just runs for ~1 second. Thats probably just the time it takes for everything to be brought up and then brought down. I can make a subset of the pageset that as is runPageInteractions waits for a few seconds.
On 2016/08/19 17:33:07, rnephew (Reviews Here) wrote: > > I can't tell how long the measurement is going to run for. Is this implicitly > > defined by imeline_based_measurement? > > I really feel like that wait in the power measurement is in a strange place. > Most other tests I've seen has waits like that in RunPageInteractions. > > Currently it just runs for ~1 second. Thats probably just the time it takes for > everything to be brought up and then brought down. I can make a subset of the > pageset that as is runPageInteractions waits for a few seconds. Sure, feel free to do so.
On 2016/08/19 17:40:27, erikchen wrote: > On 2016/08/19 17:33:07, rnephew (Reviews Here) wrote: > > > I can't tell how long the measurement is going to run for. Is this > implicitly > > > defined by imeline_based_measurement? > > > > I really feel like that wait in the power measurement is in a strange place. > > Most other tests I've seen has waits like that in RunPageInteractions. > > > > Currently it just runs for ~1 second. Thats probably just the time it takes > for > > everything to be brought up and then brought down. I can make a subset of the > > pageset that as is runPageInteractions waits for a few seconds. > > Sure, feel free to do so. I think we should make sure the wait time between Eric's benchmark & battor based one should be the same, otherwise we are not comparing apple to apple.
> I think we should make sure the wait time between Eric's benchmark & battor > based one should be the same, otherwise we are not comparing apple to apple. 30 seconds is _a lot_ of BattOr data though.
> 30 seconds is _a lot_ of BattOr data though. To expand on this, we might start having OOM issues. That is why we lowered the length of other battor benchmarks.
On 2016/08/19 17:53:16, rnephew (Reviews Here) wrote: > > 30 seconds is _a lot_ of BattOr data though. > > To expand on this, we might start having OOM issues. That is why we lowered the > length of other battor benchmarks. +1 to what Ned said. If the main thing that we're trying to test is the stability of the BattOr numbers, I'd recommend disabling all tracing categories except for 'rail' maybe. This CL (https://codereview.chromium.org/2239053003/diff/160001/tools/perf/benchmarks/...) shows how to do that.
On 2016/08/19 18:56:01, charliea wrote: > On 2016/08/19 17:53:16, rnephew (Reviews Here) wrote: > > > 30 seconds is _a lot_ of BattOr data though. > > > > To expand on this, we might start having OOM issues. That is why we lowered > the > > length of other battor benchmarks. > > +1 to what Ned said. If the main thing that we're trying to test is the > stability of the BattOr numbers, I'd recommend disabling all tracing categories > except for 'rail' maybe. This CL > (https://codereview.chromium.org/2239053003/diff/160001/tools/perf/benchmarks/...) > shows how to do that. Just to clarify: I'd recommend only enabling 'rail' so that we can collect 30s worth of data without having OOM issues.
On 2016/08/19 18:56:38, charliea wrote: > On 2016/08/19 18:56:01, charliea wrote: > > On 2016/08/19 17:53:16, rnephew (Reviews Here) wrote: > > > > 30 seconds is _a lot_ of BattOr data though. > > > > > > To expand on this, we might start having OOM issues. That is why we lowered > > the > > > length of other battor benchmarks. > > > > +1 to what Ned said. If the main thing that we're trying to test is the > > stability of the BattOr numbers, I'd recommend disabling all tracing > categories > > except for 'rail' maybe. This CL > > > (https://codereview.chromium.org/2239053003/diff/160001/tools/perf/benchmarks/...) > > shows how to do that. > > Just to clarify: I'd recommend only enabling 'rail' so that we can collect 30s > worth of data without having OOM issues. Ok, added the ability to wait for an amount of time passed at time of adding page to the story set. It is currently set to 30 seconds. Also turned on only rails.
https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:34: def __init__(self, page_set, shared_page_state_class, url, name, wait=None): you shouldn't need a default argument here, right? https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:38: self._wait = wait Can you add a comment that _wait is intended to be a duration in seconds? That's not otherwise clear. https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:169: super(MacGpuTrivialPagesStorySetWithWait, self).__init__( This looks like a giant copy paste. Could you find a way to refactor this?
https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:34: def __init__(self, page_set, shared_page_state_class, url, name, wait=None): On 2016/08/19 19:20:05, erikchen wrote: > you shouldn't need a default argument here, right? Done. https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:38: self._wait = wait On 2016/08/19 19:20:05, erikchen wrote: > Can you add a comment that _wait is intended to be a duration in seconds? That's > not otherwise clear. Done. https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:169: super(MacGpuTrivialPagesStorySetWithWait, self).__init__( On 2016/08/19 19:20:05, erikchen wrote: > This looks like a giant copy paste. Could you find a way to refactor this? Done.
lgtm % nits https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:38: self._wait = wait On 2016/08/19 20:20:55, rnephew (Reviews Here) wrote: > On 2016/08/19 19:20:05, erikchen wrote: > > Can you add a comment that _wait is intended to be a duration in seconds? > That's > > not otherwise clear. > > Done. Did you forget to include the comment in your patch set? Do you think the comment would be more clear here or on MacGpuTrivialPagesStorySet.__init__ now that you've done the refactor? https://codereview.chromium.org/2261713003/diff/60001/tools/perf/page_sets/ma... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/60001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:128: def __init__(self, wait): Should this be a required argument? Do we need to modify benchmarks/power.py?
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/40001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:38: self._wait = wait On 2016/08/19 20:27:55, erikchen wrote: > On 2016/08/19 20:20:55, rnephew (Reviews Here) wrote: > > On 2016/08/19 19:20:05, erikchen wrote: > > > Can you add a comment that _wait is intended to be a duration in seconds? > > That's > > > not otherwise clear. > > > > Done. > > Did you forget to include the comment in your patch set? Do you think the > comment would be more clear here or on MacGpuTrivialPagesStorySet.__init__ now > that you've done the refactor? I wrote the comment, then did the refactoring and must have accidentally deleted it during that. I think it makes more sense at the init so putting it there. https://codereview.chromium.org/2261713003/diff/60001/tools/perf/page_sets/ma... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/60001/tools/perf/page_sets/ma... tools/perf/page_sets/mac_gpu_sites.py:128: def __init__(self, wait): On 2016/08/19 20:27:55, erikchen wrote: > Should this be a required argument? Do we need to modify benchmarks/power.py? Woops, should be an optional argument. Fixed.
lgtm https://codereview.chromium.org/2261713003/diff/120001/tools/perf/benchmarks/... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2261713003/diff/120001/tools/perf/benchmarks/... tools/perf/benchmarks/battor.py:135: return page_sets.MacGpuTrivialPagesStorySet(wait=30) Can you add some comment about we want to wait for 30s to make it comparable with the legacy power measurement? https://codereview.chromium.org/2261713003/diff/120001/tools/perf/page_sets/m... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/120001/tools/perf/page_sets/m... tools/perf/page_sets/mac_gpu_sites.py:41: if self._wait: nits: there is no need for the "if ..." if the wait value is 0 instead of None https://codereview.chromium.org/2261713003/diff/120001/tools/perf/page_sets/m... tools/perf/page_sets/mac_gpu_sites.py:128: def __init__(self, wait=None): nits: s/wait/wait_in_seconds Also make the default value to be 0 instead of None.
https://codereview.chromium.org/2261713003/diff/120001/tools/perf/benchmarks/... File tools/perf/benchmarks/battor.py (right): https://codereview.chromium.org/2261713003/diff/120001/tools/perf/benchmarks/... tools/perf/benchmarks/battor.py:135: return page_sets.MacGpuTrivialPagesStorySet(wait=30) On 2016/08/22 12:58:37, nednguyen wrote: > Can you add some comment about we want to wait for 30s to make it comparable > with the legacy power measurement? Done. https://codereview.chromium.org/2261713003/diff/120001/tools/perf/page_sets/m... File tools/perf/page_sets/mac_gpu_sites.py (right): https://codereview.chromium.org/2261713003/diff/120001/tools/perf/page_sets/m... tools/perf/page_sets/mac_gpu_sites.py:41: if self._wait: On 2016/08/22 12:58:37, nednguyen wrote: > nits: there is no need for the "if ..." if the wait value is 0 instead of None Done. https://codereview.chromium.org/2261713003/diff/120001/tools/perf/page_sets/m... tools/perf/page_sets/mac_gpu_sites.py:128: def __init__(self, wait=None): On 2016/08/22 12:58:37, nednguyen wrote: > nits: s/wait/wait_in_seconds > > Also make the default value to be 0 instead of None. Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2261713003/#ps140001 (title: "[Telemetry] Add trivial pages page set to BattOr benchmarks.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
On 2016/08/22 23:13:13, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_retina_perf_cq on master.tryserver.chromium.perf (JOB_TIMED_OUT, no build > URL) Only change was getting rid of duplicate code since the other CL landed.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/2261713003/#ps180001 (title: "Rebase")
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 #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Add trivial pages page set to BattOr benchmarks. BUG=639308 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq ========== to ========== [Telemetry] Add trivial pages page set to BattOr benchmarks. BUG=639308 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.perf:android_s5_perf_cq;master.tryserver.chromium.perf:linux_perf_cq;master.tryserver.chromium.perf:mac_retina_perf_cq;master.tryserver.chromium.perf:winx64_10_perf_cq Committed: https://crrev.com/6175392bf638b00303602cb7a8895fc59e23fd83 Cr-Commit-Position: refs/heads/master@{#414009} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6175392bf638b00303602cb7a8895fc59e23fd83 Cr-Commit-Position: refs/heads/master@{#414009} |
