Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1435243002:
Re-implements start_with_url.* benchmarks as start_with_url2.* using TBM (Closed)
|
Created:
5 years, 1 month ago by gabadie_google Modified:
5 years, 1 month ago Reviewers:
Zhen Wang, erikchen, nednguyen, nednguyen(REVIEW IN OTHER ACC), eakuefner, pasko, gabadie CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@b00 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-implements start_with_url.* benchmarks as start_with_url2.* using TBM
start_with_url.* performance benchmark are currently using histograms.
This patch set re-implements start_with_url.* as start_with_url2.* using
TBM for comparisons of the metric results before letting
start_with_url2.* overwrite start_with_url.*. However the
foreground_tab_request_start metric requires 552472 to be fixed.
BUG=539287
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
Committed: https://crrev.com/7033604f1c3822f4a73190cf9fef458c7cd66b9b
Cr-Commit-Position: refs/heads/master@{#361007}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixes review concerns #
Total comments: 3
Patch Set 3 : Removes outdated comment #
Total comments: 10
Patch Set 4 : Fixes eakuefner's concerns #
Total comments: 25
Patch Set 5 : Adds startup metric's unit tests #
Total comments: 8
Patch Set 6 : After offline discussion with nednguyen, replacing _AddEventsResults with a mock model to directly … #
Total comments: 28
Patch Set 7 : Nits fixes #Patch Set 8 : Rebases to have linux_android_rel_ng working #
Total comments: 23
Patch Set 9 : Rebases to have linux_android_rel_ng working, the patch set #8's dependency still had an issue #Patch Set 10 : Fixes pasko's nits #
Depends on Patchset: Messages
Total messages: 59 (18 generated)
Description was changed from ========== Re-implements start_with_url.* benchmarks as start_with_url2.* using TBM start_with_url.* performance benchmark are currently using histograms. This patch set re-implements start_with_url.* as start_with_url2.* using TBM for comparisons of the metric results before letting start_with_url2.* overwrite start_with_url.*. However the foreground_tab_request_start metric requires 552472 to be fixed. BUG=539287 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ========== to ========== Re-implements start_with_url.* benchmarks as start_with_url2.* using TBM start_with_url.* performance benchmark are currently using histograms. This patch set re-implements start_with_url.* as start_with_url2.* using TBM for comparisons of the metric results before letting start_with_url2.* overwrite start_with_url.*. However the foreground_tab_request_start metric requires 552472 to be fixed. BUG=539287 CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect ==========
gabadie@chromium.org changed reviewers: + zhenw@chromium.org
On 2015/11/12 09:15:35, gabadie wrote: > mailto:gabadie@chromium.org changed reviewers: > + mailto:zhenw@chromium.org PTAL
https://codereview.chromium.org/1435243002/diff/1/tools/perf/benchmarks/start... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/1/tools/perf/benchmarks/start... tools/perf/benchmarks/start_with_url2.py:31: trace_memory = tracing_category_filter.TracingCategoryFilter( How did you enable memory-infra? Where is blink.console? And the name trace_memory is not precise. You also have startup and blink.user_timing. Maybe just use cateogry_filter as the var name. https://codereview.chromium.org/1435243002/diff/1/tools/perf/page_sets/startu... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/1/tools/perf/page_sets/startu... tools/perf/page_sets/startup_pages.py:63: BrowserStartupSharedState) nit: put this to the line above. https://codereview.chromium.org/1435243002/diff/1/tools/perf/page_sets/startu... tools/perf/page_sets/startup_pages.py:69: pass Do you need action_runner.Wait(10), like in StartedPage? (also for RunPageInteractions similarly.)
gabadie@chromium.org changed reviewers: + gabadie@chromium.org
New patch set ready. PTAL :) https://codereview.chromium.org/1435243002/diff/1/tools/perf/benchmarks/start... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/1/tools/perf/benchmarks/start... tools/perf/benchmarks/start_with_url2.py:31: trace_memory = tracing_category_filter.TracingCategoryFilter( On 2015/11/13 00:21:27, Zhen Wang wrote: > How did you enable memory-infra? Where is blink.console? > > And the name trace_memory is not precise. You also have startup and > blink.user_timing. Maybe just use cateogry_filter as the var name. Oups indeed. Bad naming. It has absolutly nothing to do with memory. My bad. Having said that, I don't need blink.console because I am not using interaction records. https://codereview.chromium.org/1435243002/diff/1/tools/perf/page_sets/startu... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/1/tools/perf/page_sets/startu... tools/perf/page_sets/startup_pages.py:63: BrowserStartupSharedState) On 2015/11/13 00:21:28, Zhen Wang wrote: > nit: put this to the line above. Done. https://codereview.chromium.org/1435243002/diff/1/tools/perf/page_sets/startu... tools/perf/page_sets/startup_pages.py:69: pass On 2015/11/13 00:21:28, Zhen Wang wrote: > Do you need action_runner.Wait(10), like in StartedPage? (also for > RunPageInteractions similarly.) Not sure. It works without and it still needs a quiet fair amount of digging if we can get rid of it. I'll just put it back and open an bug for it.
zhenw@chromium.org changed reviewers: + eakuefner@chromium.org
lgtm with nits. +eakuefner for metrics part. https://codereview.chromium.org/1435243002/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:30: # the timeline markers used for mapping threads to tabs. nit: remove the comment if not for memory-infra and no need for blink.console.
eakuefner@chromium.org changed reviewers: + nednguyen@google.com
+nednguyen for Telemetry OWNERs In general, please do not leave commented code in your CL. Instead, put TODOs where necessary, and put the code you intend to land later in another CL. https://codereview.chromium.org/1435243002/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 since this is a new file. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:26: keychain_metric.KeychainMetric.CustomizeBrowserOptions(options) You probably don't need this since you're not collecting keychain metrics in this benchmark. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:65: # TODO(gabadie): Uncomment this method and pageset_repeat++. This a current Please don't leave commented code in your CL like this. You can upload another CL, or put snippets in a bug, etc. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... tools/perf/page_sets/startup_pages.py:69: # It is still not clear why we would need to wait 10s here. It has been Put this text in the bug and replace this comment with something like # TODO(gabadie): Get rid of this (crbug.com/555504) Then, when you get around to figuring out whether it's a problem, you can either remove the comment, or remove both the comment and the code :) I'm definitely interested in seeing this happen soon, by the way, since saving 10 seconds per page per bot per run will add up in a big way. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... tools/perf/page_sets/startup_pages.py:80: class StartupPagesPageSetTBM(story.StorySet): Seems like you need to record this page set using record_wpr, and upload the recording to cloud storage (JSON and sha1 should be part of this CL): https://www.chromium.org/developers/telemetry/record_a_page_set https://codereview.chromium.org/1435243002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:11: # Map of all startup metric's values's to the trace events. I think this is fine without the comment.
https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... tools/perf/page_sets/startup_pages.py:80: class StartupPagesPageSetTBM(story.StorySet): On 2015/11/17 at 16:51:52, eakuefner wrote: > Seems like you need to record this page set using record_wpr, and upload the recording to cloud storage (JSON and sha1 should be part of this CL): https://www.chromium.org/developers/telemetry/record_a_page_set Whoops, since you're pointing to an archive_data_file that already exists for the pages, you don't need to rerecord.
https://codereview.chromium.org/1435243002/diff/20001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/20001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/11/17 16:51:52, eakuefner wrote: > nit: 2015 since this is a new file. Done. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:26: keychain_metric.KeychainMetric.CustomizeBrowserOptions(options) On 2015/11/17 16:51:52, eakuefner wrote: > You probably don't need this since you're not collecting keychain metrics in > this benchmark. Done. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_url2.py:65: # TODO(gabadie): Uncomment this method and pageset_repeat++. This a current On 2015/11/17 16:51:52, eakuefner wrote: > Please don't leave commented code in your CL like this. You can upload another > CL, or put snippets in a bug, etc. Done. Created crbug.com/557242. https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/perf/page_sets/st... tools/perf/page_sets/startup_pages.py:69: # It is still not clear why we would need to wait 10s here. It has been On 2015/11/17 16:51:52, eakuefner wrote: > Put this text in the bug and replace this comment with something like > > # TODO(gabadie): Get rid of this (crbug.com/555504) > > Then, when you get around to figuring out whether it's a problem, you can either > remove the comment, or remove both the comment and the code :) > > I'm definitely interested in seeing this happen soon, by the way, since saving > 10 seconds per page per bot per run will add up in a big way. Done. https://codereview.chromium.org/1435243002/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:11: # Map of all startup metric's values's to the trace events. On 2015/11/17 16:51:52, eakuefner wrote: > I think this is fine without the comment. Done.
nednguyen@google.com changed reviewers: + erikchen@chromium.org
CC: erikchen who has a lot of expertise about browser startup. Gabadie: can you make a synthetic regression on the browser startup and see if your startup metric can catch it?
On 2015/11/17 18:24:47, nednguyen wrote: > CC: erikchen who has a lot of expertise about browser startup. > > Gabadie: can you make a synthetic regression on the browser startup and see if > your startup metric can catch it? Already did actually and of course worked as expected. I would like add that we didn't want to replace startup_with_url right away to actually to land this one first and then compare the results and make sure they are coherent before replacing startup_with_url with these new TBM benchmarks. Waiting for review and approval for metrics/startup.py.
On 2015/11/18 10:12:46, gabadie wrote: > On 2015/11/17 18:24:47, nednguyen wrote: > > CC: erikchen who has a lot of expertise about browser startup. > > > > Gabadie: can you make a synthetic regression on the browser startup and see if > > your startup metric can catch it? > > Already did actually and of course worked as expected. I would like add that we > didn't want to replace startup_with_url right away to actually to land this one > first and then compare the results and make sure they are coherent before > replacing startup_with_url with these new TBM benchmarks. This is a sane plan. Can you update the CL description with the link the a results of sample run that shows regression? > > Waiting for review and approval for metrics/startup.py. metrics/startup.py looks good to me. Can you add test coverage?
pasko@chromium.org changed reviewers: + pasko@chromium.org
fly-by comment .. On 2015/11/18 14:16:55, nednguyen wrote: > On 2015/11/18 10:12:46, gabadie wrote: > > On 2015/11/17 18:24:47, nednguyen wrote: > > > CC: erikchen who has a lot of expertise about browser startup. > > > > > > Gabadie: can you make a synthetic regression on the browser startup and see > if > > > your startup metric can catch it? > > > > Already did actually and of course worked as expected. I would like add that > we > > didn't want to replace startup_with_url right away to actually to land this > one > > first and then compare the results and make sure they are coherent before > > replacing startup_with_url with these new TBM benchmarks. > > This is a sane plan. Can you update the CL description with the link the a > results of sample run that shows regression? It is not clear what is requested here, since the testing was done locally, did you mean the patch and the json with results to be published in pastebin or something? Do you want to make sure the data is consistent? For consistency checks it seems enough to make sure that both tests produce near identical measurement results.
https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:8: _PROCESS_CREATION = 'Startup.BrowserProcessCreation' please avoid introducing unused constants, it is better to add them when they are actually needed. Reading the code is simpler if there is less code to read, and there are more readers than writers, so that's a win. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:14: #OR ("begin/end event's name"), The "begin/end event's name" is somewhat ambiguous. I would say something like this prior to the _METRICS definition, not inside it: # A dictionary that maps metric names to a value, which can be either of the two: # 1. Event name if the event itself contains reported duration # 2. A tuple of two event names if the value to report is the time difference between starting these events https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:34: #TODO(gabadie): once 552472 fixed, enable this code please reference bugs as crbug.com/XXXXXX (to be consistent with the rest of the code) nit: one space before TODO nit: all comments should be valid sentences, so needs a period at the end https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:66: # Maps all the events we are interested in. Please avoid 'we' in comments. Also, the comments describing blocks of the implementation should preferably be phrased in imperative mood. Here you could say: Produce a map of events to track. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:77: # Generates the metric values according to the tracked events s/Generates/Generate/ s/events/events./ https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. I would shorten this as: # The single event contains the duration to report. And the one below: # The duration is defined as the difference between two event starts. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:93: nit: this empty line makes it 0.01% harder for me to read the condition, as well as the previous empty line https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:97: assert duration != None, 'something is wrong in _METRICS' this assert message is not helpful, better not have it at all if it is not possible to explain what happened in a short form
On 2015/11/18 14:45:40, pasko wrote: > fly-by comment .. > > On 2015/11/18 14:16:55, nednguyen wrote: > > On 2015/11/18 10:12:46, gabadie wrote: > > > On 2015/11/17 18:24:47, nednguyen wrote: > > > > CC: erikchen who has a lot of expertise about browser startup. > > > > > > > > Gabadie: can you make a synthetic regression on the browser startup and > see > > if > > > > your startup metric can catch it? > > > > > > Already did actually and of course worked as expected. I would like add that > > we > > > didn't want to replace startup_with_url right away to actually to land this > > one > > > first and then compare the results and make sure they are coherent before > > > replacing startup_with_url with these new TBM benchmarks. > > > > This is a sane plan. Can you update the CL description with the link the a > > results of sample run that shows regression? > > It is not clear what is requested here, since the testing was done locally, did > you mean the patch and the json with results to be published in pastebin or > something? Do you want to make sure the data is consistent? > Yes > For consistency checks it seems enough to make sure that both tests produce near > identical measurement results. Ah, if we already have existing similar metric, it is fine.
nednguyen@chromium.org changed reviewers: + nednguyen@chromium.org
https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. On 2015/11/18 15:30:58, pasko wrote: > I would shorten this as: > # The single event contains the duration to report. > > And the one below: > # The duration is defined as the difference between two event starts. Also I forgot to mention that we usually use thread time (cpu time) instead of wall time to denoise the metric when os deschedules browser's threads. Though for this case, I am not sure if using thread time applies.
https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. On 2015/11/18 15:55:14, nednguyen(REVIEW IN OTHER ACC) wrote: > On 2015/11/18 15:30:58, pasko wrote: > > I would shorten this as: > > # The single event contains the duration to report. > > > > And the one below: > > # The duration is defined as the difference between two event starts. > > Also I forgot to mention that we usually use thread time (cpu time) instead of > wall time to denoise the metric when os deschedules browser's threads. Though > for this case, I am not sure if using thread time applies. Clock time is still the primary way to measure how Chrome performs on startup. Looking only at CPU time would not include time we waste blocked on disk, which we are heavily optimizing for on startup. Also, this is a very end-to-end metric that includes activity from many threads and processes, looking at one thread would remove a lot of important delays from the picture.
https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. On 2015/11/18 16:14:56, pasko wrote: > On 2015/11/18 15:55:14, nednguyen(REVIEW IN OTHER ACC) wrote: > > On 2015/11/18 15:30:58, pasko wrote: > > > I would shorten this as: > > > # The single event contains the duration to report. > > > > > > And the one below: > > > # The duration is defined as the difference between two event starts. > > > > Also I forgot to mention that we usually use thread time (cpu time) instead of > > wall time to denoise the metric when os deschedules browser's threads. Though > > for this case, I am not sure if using thread time applies. > > Clock time is still the primary way to measure how Chrome performs on startup. > > Looking only at CPU time would not include time we waste blocked on disk, which > we are heavily optimizing for on startup. Also, this is a very end-to-end metric > that includes activity from many threads and processes, looking at one thread > would remove a lot of important delays from the picture. Ok. Let keep this for now since we also need to caliberate these metrica against legacy ones. As we go, we can try to denoise by creating other detailed metrics using other trace events.
https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. On 2015/11/18 16:22:33, nednguyen(REVIEW IN OTHER ACC) wrote: > On 2015/11/18 16:14:56, pasko wrote: > > On 2015/11/18 15:55:14, nednguyen(REVIEW IN OTHER ACC) wrote: > > > On 2015/11/18 15:30:58, pasko wrote: > > > > I would shorten this as: > > > > # The single event contains the duration to report. > > > > > > > > And the one below: > > > > # The duration is defined as the difference between two event starts. > > > > > > Also I forgot to mention that we usually use thread time (cpu time) instead > of > > > wall time to denoise the metric when os deschedules browser's threads. > Though > > > for this case, I am not sure if using thread time applies. > > > > Clock time is still the primary way to measure how Chrome performs on startup. > > > > Looking only at CPU time would not include time we waste blocked on disk, > which > > we are heavily optimizing for on startup. Also, this is a very end-to-end > metric > > that includes activity from many threads and processes, looking at one thread > > would remove a lot of important delays from the picture. > > Ok. Let keep this for now since we also need to caliberate these metrica against > legacy ones. As we go, we can try to denoise by creating other detailed metrics > using other trace events. Agreed. Detailed metrics help understand end-to-end metrics.
Unit test added, PTAL. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:8: _PROCESS_CREATION = 'Startup.BrowserProcessCreation' On 2015/11/18 15:30:59, pasko wrote: > please avoid introducing unused constants, it is better to add them when they > are actually needed. Reading the code is simpler if there is less code to read, > and there are more readers than writers, so that's a win. I thought it would be nice to have it ready here because we could easily add more metric afterwards (like between _PROCESS_CREATION and _MAIN_ENTRY_POINT for instance). https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:14: #OR ("begin/end event's name"), On 2015/11/18 15:30:58, pasko wrote: > The "begin/end event's name" is somewhat ambiguous. > > I would say something like this prior to the _METRICS definition, not inside it: > # A dictionary that maps metric names to a value, which can be either of the > two: > # 1. Event name if the event itself contains reported duration > # 2. A tuple of two event names if the value to report is the time difference > between starting these events Done. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:34: #TODO(gabadie): once 552472 fixed, enable this code On 2015/11/18 15:30:58, pasko wrote: > please reference bugs as crbug.com/XXXXXX (to be consistent with the rest of the > code) > > nit: one space before TODO > nit: all comments should be valid sentences, so needs a period at the end Done. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:66: # Maps all the events we are interested in. On 2015/11/18 15:30:59, pasko wrote: > Please avoid 'we' in comments. Also, the comments describing blocks of the > implementation should preferably be phrased in imperative mood. > > Here you could say: > Produce a map of events to track. Done. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:77: # Generates the metric values according to the tracked events On 2015/11/18 15:30:58, pasko wrote: > s/Generates/Generate/ > s/events/events./ Done. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. On 2015/11/18 15:30:58, pasko wrote: > I would shorten this as: > # The single event contains the duration to report. > > And the one below: > # The duration is defined as the difference between two event starts. Done. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:85: # trace event, therefor we directly fetch the duration of this event. On 2015/11/18 16:22:33, nednguyen(REVIEW IN OTHER ACC) wrote: > On 2015/11/18 16:14:56, pasko wrote: > > On 2015/11/18 15:55:14, nednguyen(REVIEW IN OTHER ACC) wrote: > > > On 2015/11/18 15:30:58, pasko wrote: > > > > I would shorten this as: > > > > # The single event contains the duration to report. > > > > > > > > And the one below: > > > > # The duration is defined as the difference between two event starts. > > > > > > Also I forgot to mention that we usually use thread time (cpu time) instead > of > > > wall time to denoise the metric when os deschedules browser's threads. > Though > > > for this case, I am not sure if using thread time applies. > > > > Clock time is still the primary way to measure how Chrome performs on startup. > > > > Looking only at CPU time would not include time we waste blocked on disk, > which > > we are heavily optimizing for on startup. Also, this is a very end-to-end > metric > > that includes activity from many threads and processes, looking at one thread > > would remove a lot of important delays from the picture. > > Ok. Let keep this for now since we also need to caliberate these metrica against > legacy ones. As we go, we can try to denoise by creating other detailed metrics > using other trace events. Done. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:93: On 2015/11/18 15:30:58, pasko wrote: > nit: this empty line makes it 0.01% harder for me to read the condition, as well > as the previous empty line Removing this line and all the similar one makes it harder for me to read, I prefer when my code is ventilated. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:97: assert duration != None, 'something is wrong in _METRICS' On 2015/11/18 15:30:58, pasko wrote: > this assert message is not helpful, better not have it at all if it is not > possible to explain what happened in a short form Done.
https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:8: _PROCESS_CREATION = 'Startup.BrowserProcessCreation' On 2015/11/18 16:53:34, gabadie wrote: > On 2015/11/18 15:30:59, pasko wrote: > > please avoid introducing unused constants, it is better to add them when they > > are actually needed. Reading the code is simpler if there is less code to > read, > > and there are more readers than writers, so that's a win. > > I thought it would be nice to have it ready here because we could easily add > more metric afterwards (like between _PROCESS_CREATION and _MAIN_ENTRY_POINT for > instance). Not having this line does not prevent us from easily adding more metrics afterwards. Dead code is a tax for readers. If there is not immediate plan to use something you added, do not add. https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:34: #TODO(gabadie): once 552472 fixed, enable this code On 2015/11/18 16:53:34, gabadie wrote: > On 2015/11/18 15:30:58, pasko wrote: > > please reference bugs as crbug.com/XXXXXX (to be consistent with the rest of > the > > code) > > > > nit: one space before TODO > > nit: all comments should be valid sentences, so needs a period at the end > > Done. I agree with the opinion already expressed in this review that we should avoid commented out code. So the TODO should say something like: TODO(gabadie): Enable foreground_tab_request_start once crbug.com/552472 is fixed. And remove the vommented-out code. It's easy to add it back. Reasoning is similar as previously: 1. tax for readers 2. the code in comments will likely rot, since nobody checks that it works with refactorings 3. we are strict in our attempts to keep everything consistent, sometimes insanely strict, the only way to deal with it is to get used to it https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:93: On 2015/11/18 16:53:34, gabadie wrote: > On 2015/11/18 15:30:58, pasko wrote: > > nit: this empty line makes it 0.01% harder for me to read the condition, as > well > > as the previous empty line > > Removing this line and all the similar one makes it harder for me to read, I > prefer when my code is ventilated. OK with me, but please note that it's not your code, it's _our_ code written by you. Big difference. https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:69: def events_iterator(): Pythonists would rather call it a generator, so I would suggest to name it as: GenerateEvents disclaimer: I am not a pythonist https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:105: assert duration != None, 'something is wrong in _METRICS[{}]'.format( The assert is trivial enough to not need extra output. Also it is not necessary because results.AddValue asserts on isinstance(value, something). Please remove the assert. https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:18: start, duration=None): why multiple lines? https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:60: # Test case to get the duration of the first occurance of a begin/end event. s/occurance/occurrence/ A 'begin/end event' looks confusing. This doc describes them as 'Duration Events': https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... I like this name more.
Changed the unittest to test with the API call AddWholeTraceResults. PTAL :-) https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:69: def events_iterator(): On 2015/11/18 17:52:41, pasko wrote: > Pythonists would rather call it a generator, so I would suggest to name it as: > GenerateEvents > > disclaimer: I am not a pythonist Done. https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:105: assert duration != None, 'something is wrong in _METRICS[{}]'.format( On 2015/11/18 17:52:41, pasko wrote: > The assert is trivial enough to not need extra output. Also it is not necessary > because results.AddValue asserts on isinstance(value, something). > > Please remove the assert. Done. https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:18: start, duration=None): On 2015/11/18 17:52:41, pasko wrote: > why multiple lines? Done. https://codereview.chromium.org/1435243002/diff/80001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:60: # Test case to get the duration of the first occurance of a begin/end event. On 2015/11/18 17:52:41, pasko wrote: > s/occurance/occurrence/ > > A 'begin/end event' looks confusing. > > This doc describes them as 'Duration Events': > https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... > > I like this name more. Done.
lgtm with some comments. Please also wait for Ethan & Pasko's stamps https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:23: def RunAggregator(self): nits: s/RunAggregator/ComputeStartupMetrics https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:78: DURATION) Can you also add test coverage for open_tabs_time, first_non_empty_paint_time,..?
lgtm with nits, please wait for pasko. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:5: import page_sets this and the next line should be switched; these are to be alphabetized by module name. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:6: from core import perf_benchmark newline between perf import section and third-party import section. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:13: class _StartWithUrlTBM(perf_benchmark.PerfBenchmark): nit: newline after this line. the rule is, blank line should come after docstring, but in this case you have no docstring. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:40: """Measure time to start Chrome cold with startup URLs""" nit: newline after docstring https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:56: """Measure time to start Chrome warm with startup URLs""" nit: newline after docstring. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:11: """ nit: looks like this can fit on the previous line https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:20: nit: two lines between top-level definitions https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:78: """ Pages for testing starting Chrome with a URL. nit: no space between """ and Pages. Newline after first line of docstring. https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup.py:36: # 'foreground_tab_request_start': Please remove this dead code and rephrase the todo. https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:27: def GenerateEvents(event_predicate): Sorry, but I disagree with pasko here. Please change this back: it will be consistent with IterAllEvents and with the general idea that is pervasive in tracing, of iterating through collections of events.
https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:27: def GenerateEvents(event_predicate): On 2015/11/18 at 18:39:53, eakuefner wrote: > Sorry, but I disagree with pasko here. Please change this back: it will be consistent with IterAllEvents and with the general idea that is pervasive in tracing, of iterating through collections of events. Through I do agree that it should be IterateEvents rather than iterate_events: we name methods with camel case and properties with lowercase and underscores.
generally looks good, a few more small nits https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:13: class _StartWithUrlTBM(perf_benchmark.PerfBenchmark): On 2015/11/18 18:39:52, eakuefner wrote: > nit: newline after this line. the rule is, blank line should come after > docstring, but in this case you have no docstring. it would be nice to have a docstring https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:40: """Measure time to start Chrome cold with startup URLs""" nit: s/Measure/Measures/ (it's a class) https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:27: def GenerateEvents(event_predicate): On 2015/11/18 18:39:53, eakuefner wrote: > Sorry, but I disagree with pasko here. Please change this back: it will be > consistent with IterAllEvents and with the general idea that is pervasive in > tracing, of iterating through collections of events. up to you, of course
Thank you guys! :-) https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:5: import page_sets On 2015/11/18 18:39:53, eakuefner wrote: > this and the next line should be switched; these are to be alphabetized by > module name. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:6: from core import perf_benchmark On 2015/11/18 18:39:52, eakuefner wrote: > newline between perf import section and third-party import section. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:13: class _StartWithUrlTBM(perf_benchmark.PerfBenchmark): On 2015/11/18 18:39:52, eakuefner wrote: > nit: newline after this line. the rule is, blank line should come after > docstring, but in this case you have no docstring. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:40: """Measure time to start Chrome cold with startup URLs""" On 2015/11/18 18:39:52, eakuefner wrote: > nit: newline after docstring Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:56: """Measure time to start Chrome warm with startup URLs""" On 2015/11/18 18:39:52, eakuefner wrote: > nit: newline after docstring. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:11: """ On 2015/11/18 18:39:53, eakuefner wrote: > nit: looks like this can fit on the previous line Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:20: On 2015/11/18 18:39:53, eakuefner wrote: > nit: two lines between top-level definitions Done. https://codereview.chromium.org/1435243002/diff/100001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:78: """ Pages for testing starting Chrome with a URL. On 2015/11/18 18:39:53, eakuefner wrote: > nit: no space between """ and Pages. Newline after first line of docstring. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup.py:36: # 'foreground_tab_request_start': On 2015/11/18 18:39:53, eakuefner wrote: > Please remove this dead code and rephrase the todo. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:23: def RunAggregator(self): On 2015/11/18 18:23:13, nednguyen wrote: > nits: s/RunAggregator/ComputeStartupMetrics Done. https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:27: def GenerateEvents(event_predicate): On 2015/11/18 18:41:11, eakuefner wrote: > On 2015/11/18 at 18:39:53, eakuefner wrote: > > Sorry, but I disagree with pasko here. Please change this back: it will be > consistent with IterAllEvents and with the general idea that is pervasive in > tracing, of iterating through collections of events. > > Through I do agree that it should be IterateEvents rather than iterate_events: > we name methods with camel case and properties with lowercase and underscores. Done. https://codereview.chromium.org/1435243002/diff/100001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:78: DURATION) On 2015/11/18 18:23:13, nednguyen wrote: > Can you also add test coverage for open_tabs_time, > first_non_empty_paint_time,..? Done.
almost there https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/startup.py:8: _PROCESS_CREATION = 'Startup.BrowserProcessCreation' On 2015/11/18 17:52:41, pasko wrote: > On 2015/11/18 16:53:34, gabadie wrote: > > On 2015/11/18 15:30:59, pasko wrote: > > > please avoid introducing unused constants, it is better to add them when > they > > > are actually needed. Reading the code is simpler if there is less code to > > read, > > > and there are more readers than writers, so that's a win. > > > > I thought it would be nice to have it ready here because we could easily add > > more metric afterwards (like between _PROCESS_CREATION and _MAIN_ENTRY_POINT > for > > instance). > > Not having this line does not prevent us from easily adding more metrics > afterwards. Dead code is a tax for readers. If there is not immediate plan to > use something you added, do not add. did you know that .. you are supposed to respond to all comments? If the issue does not require changes any more, just hit ack https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:15: """Measures time to start Chrome with startup URLs""" nit: period, same below https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:21: # TODO(gabadie): We don't want to replace start_with_url.* yet. please avoid 'We'. You could say: TODO(gabadie): change to start_with_url.* after confirming that both benchmarks produce the same results. https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:62: options = {'pageset_repeat': 10} Why 10 for warm and 5 for cold? Is this based on some rule related to how long the tests run. Apologies if it was discussed already, I did not pay much attention. https://codereview.chromium.org/1435243002/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:66: # Do not call super.RunNavigateSteps() to not reload the page that has s/to not reload/to avoid reloading/ https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup.py:39: # List of all event names to track. This is a set, not a list. I suppose you did not use 'list' as a verb here? Please remove the comment, the variable name explains it well. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup.py:51: track Chrome's performance.""" this second sentence is obvious, please remove https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:22: # pylint: disable=W0201 please educate me why this is needed, maybe add a comment https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:26: # Create a mock model usable by StartupTimelineMetric.AddWholeTraceResults() nit: period https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:27: def iterate_events(event_predicate): we name functions/methods/generators in CamelCase. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:42: # Code coverage for untracked events, to make sure there is nothing wrong we always try make sure that nothing wrong is going on. Please remove this second part of the sentence. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:76: # Generate duplicated events to make sure we consider only the first one. please avoid 'we' ... to make sure only the first event is considered
https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:62: options = {'pageset_repeat': 10} On 2015/11/19 13:08:17, pasko wrote: > Why 10 for warm and 5 for cold? Is this based on some rule related to how long > the tests run. Apologies if it was discussed already, I did not pay much > attention. For now, let's make these number the same as legacy benchmarks for the ease of calibration.
landing. https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:15: """Measures time to start Chrome with startup URLs""" On 2015/11/19 13:08:17, pasko wrote: > nit: period, same below Done. https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:21: # TODO(gabadie): We don't want to replace start_with_url.* yet. On 2015/11/19 13:08:17, pasko wrote: > please avoid 'We'. You could say: > > TODO(gabadie): change to start_with_url.* after confirming that both benchmarks > produce the same results. Done. https://codereview.chromium.org/1435243002/diff/140001/tools/perf/page_sets/s... File tools/perf/page_sets/startup_pages.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/perf/page_sets/s... tools/perf/page_sets/startup_pages.py:66: # Do not call super.RunNavigateSteps() to not reload the page that has On 2015/11/19 13:08:17, pasko wrote: > s/to not reload/to avoid reloading/ Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup.py:39: # List of all event names to track. On 2015/11/19 13:08:17, pasko wrote: > This is a set, not a list. I suppose you did not use 'list' as a verb here? > > Please remove the comment, the variable name explains it well. Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup.py:51: track Chrome's performance.""" On 2015/11/19 13:08:17, pasko wrote: > this second sentence is obvious, please remove Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... File tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:22: # pylint: disable=W0201 On 2015/11/19 13:08:17, pasko wrote: > please educate me why this is needed, maybe add a comment http://pylint-messages.wikidot.com/messages:w0201 Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:26: # Create a mock model usable by StartupTimelineMetric.AddWholeTraceResults() On 2015/11/19 13:08:17, pasko wrote: > nit: period Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:27: def iterate_events(event_predicate): On 2015/11/19 13:08:17, pasko wrote: > we name functions/methods/generators in CamelCase. My bad. Miss understood what eakuefner said. Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:42: # Code coverage for untracked events, to make sure there is nothing wrong On 2015/11/19 13:08:17, pasko wrote: > we always try make sure that nothing wrong is going on. Please remove this > second part of the sentence. Done. https://codereview.chromium.org/1435243002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/web_perf/metrics/startup_unittest.py:76: # Generate duplicated events to make sure we consider only the first one. On 2015/11/19 13:08:17, pasko wrote: > please avoid 'we' > > ... to make sure only the first event is considered Done.
lgtm, great stuff! thank you! https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... File tools/perf/benchmarks/start_with_url2.py (right): https://codereview.chromium.org/1435243002/diff/140001/tools/perf/benchmarks/... tools/perf/benchmarks/start_with_url2.py:62: options = {'pageset_repeat': 10} On 2015/11/19 15:56:46, nednguyen wrote: > On 2015/11/19 13:08:17, pasko wrote: > > Why 10 for warm and 5 for cold? Is this based on some rule related to how long > > the tests run. Apologies if it was discussed already, I did not pay much > > attention. > > For now, let's make these number the same as legacy benchmarks for the ease of > calibration. Acknowledged.
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, nednguyen@google.com, eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/1435243002/#ps180001 (title: "Fixes pasko's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1435243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435243002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_perf_bisect on tryserver.chromium.perf (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/...)
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/1435243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435243002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_perf_bisect on tryserver.chromium.perf (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/...)
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/1435243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435243002/180001
Please press the commit box one more time after http://crbug.com/559381 is fixed; win_perf_bisect has been having some issues that should hopefully be resolved after the tryserver is restarted.
The CQ bit was unchecked by eakuefner@chromium.org
The CQ bit was checked by eakuefner@chromium.org
tryserver restarted; box reticked.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1435243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435243002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
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/1435243002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1435243002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7033604f1c3822f4a73190cf9fef458c7cd66b9b Cr-Commit-Position: refs/heads/master@{#361007} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
