Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2013223002:
Add WebView startup time benchmark (Closed)
|
Created:
4 years, 7 months ago by alexandermont Modified:
4 years, 6 months ago CC:
chromium-reviews, Primiano Tucci (use gerrit), telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd WebView startup time benchmark.
BUG=614374
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq
Committed: https://crrev.com/a9e1620934522f32e3a5f500551aadaaa3642a44
Cr-Commit-Position: refs/heads/master@{#399080}
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : #
Total comments: 8
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 4
Patch Set 12 : #
Total comments: 8
Patch Set 13 : #Messages
Total messages: 55 (20 generated)
Description was changed from ========== Add WebView startup time benchmark. ========== to ========== Add WebView startup time benchmark. CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
alexandermont@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com, zhenw@chromium.org
https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:41: @benchmark.Disabled('android') Huh? Is this a diffbase problem? Seems like we definitely don't want to alter this in the CL. https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:75: class WebviewStartupSystemHealthBenchmark(_SystemHealthBenchmark): nednguyen@chromium.org, do we want @benchmark.Disabled('all') on this for now? I'd assume that the most we want is @benchmark.Enabled('android') I'd definitely assume that we want some annotation on this, though, because otherwise it might try to run on non-Android devices, right? https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:76: """Chrome Webview Startup System Health Benchmark. nit: might want to making this "Chrome WebView Startup Time..." https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:77: """ nit: can we put the ending """ on the same line as the rest? https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:82: '-*,disabled-by-default-memory-infra')) Not sure exactly how this tracing category filter works, but I doubt that we want memory-infra enabled here? Also, any idea what -* is needed for? https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:83: options.SetTimelineBasedMetric('WebviewStartupMetric') note: based on suggestion in other CL, this should probably become 'webviewStartupMetric' https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:90: def Name(cls): not sure about the python style here, but same note as other CL: don't you sometimes name arguments like this 'unused' or something? I've also seen: del cls not sure when each is used https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:102: Also doubt that we want to alter this
https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:41: @benchmark.Disabled('android') On 2016/05/27 at 23:27:26, charliea wrote: > Huh? Is this a diffbase problem? Seems like we definitely don't want to alter this in the CL. I think this is correct. I didn't intend to change this. Now changed back. https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:76: """Chrome Webview Startup System Health Benchmark. On 2016/05/27 at 23:27:26, charliea wrote: > nit: might want to making this "Chrome WebView Startup Time..." Done https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:77: """ On 2016/05/27 at 23:27:26, charliea wrote: > nit: can we put the ending """ on the same line as the rest? Done https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:82: '-*,disabled-by-default-memory-infra')) On 2016/05/27 at 23:27:26, charliea wrote: > Not sure exactly how this tracing category filter works, but I doubt that we want memory-infra enabled here? > > Also, any idea what -* is needed for? Removed https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:83: options.SetTimelineBasedMetric('WebviewStartupMetric') On 2016/05/27 at 23:27:26, charliea wrote: > note: based on suggestion in other CL, this should probably become 'webviewStartupMetric' Done https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:90: def Name(cls): On 2016/05/27 at 23:27:26, charliea wrote: > not sure about the python style here, but same note as other CL: don't you sometimes name arguments like this 'unused' or something? > > I've also seen: > > del cls > > not sure when each is used I can't find anything specific about this in the Google Python style guide. But in general, my understanding is that since this is a @classmethod, the first argument is always "cls" (the class object), just like the first argument in ordinary methods is "self". https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:102: On 2016/05/27 at 23:27:26, charliea wrote: > Also doubt that we want to alter this Changed back
https://codereview.chromium.org/2013223002/diff/80001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/80001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:81: """Chrome Webview Startup Time system health Benchmark. """ nit: delete trailing space https://codereview.chromium.org/2013223002/diff/80001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:81: """Chrome Webview Startup Time system health Benchmark. """ nit: system health should be capitalized (like above) https://codereview.chromium.org/2013223002/diff/80001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:81: """Chrome Webview Startup Time system health Benchmark. """ nit: I think that we want to add more context to this pydoc. What is the "Chrome Webview Startup Time system health Benchmark"? What's the purpose of it? A sentence or two of context would be good. (the memory benchmark provides a link to the doc, but if we can do something inline, I think that's preferable)
Description was changed from ========== Add WebView startup time benchmark. CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add WebView startup time benchmark. CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
nednguyen@google.com changed reviewers: + perezju@chromium.org
https://codereview.chromium.org/2013223002/diff/80001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/80001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:81: """Chrome Webview Startup Time system health Benchmark. """ On 2016/06/06 at 19:52:53, charliea wrote: > nit: delete trailing space Done
https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/system_health.py:82: '-*,disabled-by-default-memory-infra')) On 2016/05/31 17:21:44, alexandermont wrote: > On 2016/05/27 at 23:27:26, charliea wrote: > > Not sure exactly how this tracing category filter works, but I doubt that we > want memory-infra enabled here? > > > > Also, any idea what -* is needed for? > > Removed The -* is used on memory infra benchmarks to disable (chrome) tracing on everything except memory infra. Also not sure if system_health is the best location for this benchmark. Maybe it should live on its own new file? +primiano any thoughts?
nit: CL subject shouldn't end in a period nit: Please add a line between the CL subject line and the extra trybots line in the CL description nit: Please be sure to add the BUG=614374 line above the extra trybot line so that this CL links back to your tracking bug https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:79: """Chrome Webview Startup Time System Health Benchmark. I don't think this first line is useful. I'd prefer just """A benchmark that measures how long it takes WebView to start up and load a blank page. """ https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:84: page_set = page_sets.BlankPageSet nit: can you add a blank line between this line and the next? https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:86: options = timeline_based_measurement.Options( nit: this can be deleted (it's the default) https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:90: options.config.enable_chrome_trace = False nit: this can be deleted (it's the default)
Description was changed from ========== Add WebView startup time benchmark. CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:79: """Chrome Webview Startup Time System Health Benchmark. On 2016/06/08 at 20:57:58, charliea wrote: > I don't think this first line is useful. I'd prefer just > > """A benchmark that measures how long it takes WebView to start up and load a blank page. > """ Done https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:84: page_set = page_sets.BlankPageSet On 2016/06/08 at 20:57:58, charliea wrote: > nit: can you add a blank line between this line and the next? Done https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:86: options = timeline_based_measurement.Options( On 2016/06/08 at 20:57:58, charliea wrote: > nit: this can be deleted (it's the default) Done https://codereview.chromium.org/2013223002/diff/110002/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:90: options.config.enable_chrome_trace = False On 2016/06/08 at 20:57:58, charliea wrote: > nit: this can be deleted (it's the default) Done
non-owner lgtm (probably want zhenw@ for the owner LGTM?)
I tested it again with the changes and found: 1. enable_chrome_trace is default True. so we do need the line that sets it to False if we don't want the Chrome trace. 2. If we do include the Chrome trace here, we get the error message shown in https://github.com/catapult-project/catapult/issues/2373, even if atrace is set to have supportsExplicitClockSync set to false.
I am not entirely sure about the context for this benchmark. Why should it be a system health benchmark? Should this benchmark be placed in startup.py? https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:92: @benchmark.Enabled('android') This enables this benchmark on all android bots. Do all android bots run webview benchmarks or just Nexus 5X? perezju, do you have any idea? Should we use @benchmark.Enabled('android-webview')? https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:95: and load a blank page. This should be a 1-line description. And then put detailed description in a separate paragraph. https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:103: options.config.enable_chrome_trace = False Do we want to get atrace and chrome trace at the same time eventually? If so, can you add a TODO here to indicate that?
On 2016/06/08 22:29:56, Zhen Wang wrote: > I am not entirely sure about the context for this benchmark. Why should it be a > system health benchmark? Should this benchmark be placed in startup.py? > > https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... > File tools/perf/benchmarks/system_health.py (right): > > https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... > tools/perf/benchmarks/system_health.py:92: @benchmark.Enabled('android') > This enables this benchmark on all android bots. Do all android bots run webview > benchmarks or just Nexus 5X? > > perezju, do you have any idea? Should we use > @benchmark.Enabled('android-webview')? > > https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... > tools/perf/benchmarks/system_health.py:95: and load a blank page. > This should be a 1-line description. And then put detailed description in a > separate paragraph. > > https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... > tools/perf/benchmarks/system_health.py:103: options.config.enable_chrome_trace = > False > Do we want to get atrace and chrome trace at the same time eventually? If so, > can you add a TODO here to indicate that? This is a SH benchmark according to the sh council.
https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:103: options.config.enable_chrome_trace = False On 2016/06/08 at 22:29:56, Zhen Wang wrote: > Do we want to get atrace and chrome trace at the same time eventually? If so, can you add a TODO here to indicate that? Not in this benchmark. This benchmark just needs to know about the webview startup time, which just requires atrace. If there were ever to be a metric that required both atrace and chrome trace data, that would most likely be measured by a different benchmark.
On 2016/06/08 22:54:03, alexandermont wrote: > https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... > File tools/perf/benchmarks/system_health.py (right): > > https://codereview.chromium.org/2013223002/diff/190001/tools/perf/benchmarks/... > tools/perf/benchmarks/system_health.py:103: options.config.enable_chrome_trace = > False > On 2016/06/08 at 22:29:56, Zhen Wang wrote: > > Do we want to get atrace and chrome trace at the same time eventually? If so, > can you add a TODO here to indicate that? > > Not in this benchmark. This benchmark just needs to know about the webview > startup time, which just requires atrace. If there were ever to be a metric that > required both atrace and chrome trace data, that would most likely be measured > by a different benchmark. Can you add the reason of not requiring chrome trace as a comment? Thanks!
https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:92: @benchmark.Enabled('android') I am almost sure you will need @benchmark.Enabled('android-webview'). I will stamp once perezju confirms.
perezju@chromium.org changed reviewers: + petrcermak@chromium.org
+petrcermak FYI https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:92: @benchmark.Enabled('android') On 2016/06/09 00:35:08, Zhen Wang wrote: > I am almost sure you will need @benchmark.Enabled('android-webview'). > > I will stamp once perezju confirms. Yes, this should be android-webview. https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:93: class WebviewStartupSystemHealthBenchmark(_SystemHealthBenchmark): I don't think this should subclass _SystemHealthBenchmark, can you subclass directly perf_benchmark.PerfBenchmark instead?
perezju@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:101: page_set = page_sets.BlankPageSet +torne, do we also want to run the benchmark on some other pages?
https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:101: page_set = page_sets.BlankPageSet On 2016/06/09 11:02:51, perezju wrote: > +torne, do we also want to run the benchmark on some other pages? I don't think it's particularly important right now. A blank page captures the main thing we care about.
https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:93: class WebviewStartupSystemHealthBenchmark(_SystemHealthBenchmark): This class is now wedged in the middle of memory system health bemchmarks: class _MemorySystemHealthBenchmark *** class WebviewStartupSystemHealthBenchmark *** class DesktopMemorySystemHealthBenchmark class MobileMemorySystemHealthBenchmark Could you please move it either before, or after the memory block? Thanks!
https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... File tools/perf/benchmarks/system_health.py (right): https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:92: @benchmark.Enabled('android') On 2016/06/09 at 10:51:06, perezju wrote: > On 2016/06/09 00:35:08, Zhen Wang wrote: > > I am almost sure you will need @benchmark.Enabled('android-webview'). > > > > I will stamp once perezju confirms. > > Yes, this should be android-webview. Done https://codereview.chromium.org/2013223002/diff/210001/tools/perf/benchmarks/... tools/perf/benchmarks/system_health.py:93: class WebviewStartupSystemHealthBenchmark(_SystemHealthBenchmark): On 2016/06/09 at 12:56:03, petrcermak wrote: > This class is now wedged in the middle of memory system health bemchmarks: > > class _MemorySystemHealthBenchmark > *** class WebviewStartupSystemHealthBenchmark *** > class DesktopMemorySystemHealthBenchmark > class MobileMemorySystemHealthBenchmark > > Could you please move it either before, or after the memory block? Thanks! Done
lgtm
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2013223002/#ps230001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013223002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: winx64_10_perf_cq on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013223002/230001
The CQ bit was unchecked by alexandermont@chromium.org
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013223002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: winx64_10_perf_cq on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013223002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: winx64_10_perf_cq on tryserver.chromium.perf (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.perf/builders/winx64_10_perf_c...)
Description was changed from ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013223002/230001
Message was sent while issue was closed.
Description was changed from ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq ========== to ========== Add WebView startup time benchmark. BUG=614374 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq Committed: https://crrev.com/a9e1620934522f32e3a5f500551aadaaa3642a44 Cr-Commit-Position: refs/heads/master@{#399080} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a9e1620934522f32e3a5f500551aadaaa3642a44 Cr-Commit-Position: refs/heads/master@{#399080} |
