Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410943005:
Implements AddStartupEventsForTelemetry() in startup_metric_utils.cc for in-coming startup telemetry (Closed)
|
Created:
5 years, 1 month ago by gabadie_google Modified:
5 years, 1 month ago 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. |
DescriptionImplements AddStartupEventsForTelemetry() in startup_metric_utils.cc for in-coming startup telemetry benchmarks using TBM.
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/4cec3b7462517d2b8911c131f33d141dd9bd25e0
Cr-Commit-Position: refs/heads/master@{#360592}
Patch Set 1 #Patch Set 2 : Uplift for review #Patch Set 3 : Moves mark trace events ton undeprecated startup_metric_utils.cc's functions #Patch Set 4 : Fixes compilation failure in startup_metric_utils.cc #Patch Set 5 : Replace added mark event by instant time event, and ignore the first cold results in warm benchmarks #
Total comments: 12
Patch Set 6 : Patchs split up (only chromium side changes) + gab's review fix #
Total comments: 4
Patch Set 7 : Gab's tips' fixes #
Total comments: 4
Patch Set 8 : Fixes TRACE_EVENT_INSTANT_WITH_TIMESTAMP to use TRACE_EVENT_PHASE_INSTANT #
Total comments: 4
Patch Set 9 : Renames TRACE_EVENT_INSTANT_WITH_TIMESTAMP to TRACE_EVENT_INSTANT_WITH_TIMESTAMP0 #Patch Set 10 : Rebases on top of 443bd11bc8106bea98a89819b7bb1b35d6e5ddf8 #Patch Set 11 : s/g_browser_process_creation_ticks/g_process_creation_ticks #Patch Set 12 : Fixes linux_android_rel_ng failures #
Total comments: 2
Patch Set 13 : Brings back AddStartupEventsForTelemetry #Patch Set 14 : Fixes telemetry_perf_unittests on linux_android_rel_ng #Patch Set 15 : Fixes miss leading instant events caused by patch set #14 #
Dependent Patchsets: Messages
Total messages: 41 (14 generated)
gabadie@chromium.org changed reviewers: + ben@chromium.org, gabadie@chromium.org, nednguyen@google.com
PTAL, thanks. =)
On 2015/11/09 14:24:35, gabadie wrote: > PTAL, thanks. =) It has been ported to my attention that there is a slight issue, trace mark events are reserved for the JS api mark & measure. I'll make the modifications.
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: + gab@chromium.org
Hi there, Just uploaded patch set #5 ready to review. It fixes all the concern I was getting offline. I'm now inviting anyone to review their respective part. And please excuse me for the spamming because of the previous patch sets. Thank you. =)
On 2015/11/10 16:07:29, gabadie wrote: > Hi there, > > Just uploaded patch set #5 ready to review. It fixes all the concern I was > getting offline. I'm now inviting anyone to review their respective part. And > please excuse me for the spamming because of the previous patch sets. > > Thank you. =) Can you move the chrome change to a separate CL? dsinclair@, simonhatch@ & zhenw@ can review those changes
startup_metrics_util lg, a few comments below https://codereview.chromium.org/1410943005/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1410943005/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1708: TRACE_EVENT_END0("startup", "Startup.BrowserOpenTabs"); I like these, except I'm not convinced they belong in this specific change? Perhaps split it into multiple dedicated CLs? https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:312: void TraceMissingStartupInstantEvents() { Would prefer an even more specific name, e.g. AddStartupEventsForTelemetry(). With a description like: // Records "instant" trace events for global timings observed by telemetry. (if we ever need something more generic, we should add it to the macros in this file, but for now a dedicated helper sgtm) https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:316: is_first_call = false; This method only be called once hence you shouldn't need to do this check. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:321: TRACE_EVENT_INSTANT_WITH_TIMESTAMP("startup", Suggest running "git cl format", the argument alignment as-is is incorrect. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:323: StartupTimeToTimeTicks(g_process_creation_time.Get()).ToInternalValue()); FYI: Very soon this should use g_process_creation_ticks instead (i.e. whenever https://codereview.chromium.org/1425263003/ completes its journey through CQ). https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:327: StartupTimeToTimeTicks(g_main_entry_point_time.Get()).ToInternalValue()); g_main_entry_point_ticks for this too
gabadie@chromium.org changed reviewers: - ben@chromium.org, gab@chromium.org, gabadie@chromium.org, nednguyen@google.com
gabadie@chromium.org changed reviewers: + gab@chromium.org, gabadie@chromium.org, simonhatch@chromium.org
simonhatch@chromium.org: Please review changes in base/trace_event/common/trace_event_common.h gab@chromium.org: Please review changes in components/startup_metric_utils/browser/startup_metric_utils.cc Thanks you. :) https://codereview.chromium.org/1410943005/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1410943005/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1708: TRACE_EVENT_END0("startup", "Startup.BrowserOpenTabs"); On 2015/11/10 19:30:53, gab wrote: > I like these, except I'm not convinced they belong in this specific change? > Perhaps split it into multiple dedicated CLs? Done. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:312: void TraceMissingStartupInstantEvents() { On 2015/11/10 19:30:53, gab wrote: > Would prefer an even more specific name, e.g. AddStartupEventsForTelemetry(). > > With a description like: > > // Records "instant" trace events for global timings observed by telemetry. > > > (if we ever need something more generic, we should add it to the macros in this > file, but for now a dedicated helper sgtm) Done. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:316: is_first_call = false; On 2015/11/10 19:30:53, gab wrote: > This method only be called once hence you shouldn't need to do this check. Good catch! Done. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:321: TRACE_EVENT_INSTANT_WITH_TIMESTAMP("startup", On 2015/11/10 19:30:53, gab wrote: > Suggest running "git cl format", the argument alignment as-is is incorrect. My bad. Done. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:323: StartupTimeToTimeTicks(g_process_creation_time.Get()).ToInternalValue()); On 2015/11/10 19:30:53, gab wrote: > FYI: Very soon this should use g_process_creation_ticks instead (i.e. whenever > https://codereview.chromium.org/1425263003/ completes its journey through CQ). Done. https://codereview.chromium.org/1410943005/diff/80001/components/startup_metr... components/startup_metric_utils/browser/startup_metric_utils.cc:327: StartupTimeToTimeTicks(g_main_entry_point_time.Get()).ToInternalValue()); On 2015/11/10 19:30:53, gab wrote: > g_main_entry_point_ticks for this too Done.
Thanks gab for your feedback!
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 ========== Implements AddStartupEventsForTelemetry() in startup_metric_utils.cc for in-coming startup telemetry benchmarks using TBM. 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 ==========
lgtm w/ comments, thanks! https://codereview.chromium.org/1410943005/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1410943005/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:317: // TODO(gabadie): Once startup_with_url.* benchmarks are replaced by Please add bug# https://codereview.chromium.org/1410943005/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:354: } Also, FYI going forward, I think telemetry should care about more of the timings in this file (which are mostly all more relevant than "main entry point" -- e.g. first paint, etc.).
Gab's comment fixed. PTAL :) https://codereview.chromium.org/1410943005/diff/100001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1410943005/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:317: // TODO(gabadie): Once startup_with_url.* benchmarks are replaced by On 2015/11/12 14:41:38, gab wrote: > Please add bug# Done. https://codereview.chromium.org/1410943005/diff/100001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:354: } On 2015/11/12 14:41:38, gab wrote: > Also, FYI going forward, I think telemetry should care about more of the timings > in this file (which are mostly all more relevant than "main entry point" -- e.g. > first paint, etc.). Telemetry w'll use thoses values to diff to other instant events on its own side for convenience, the reason why it is done this way.
https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ Should this be TRACE_EVENT_MARK_WITH_TIMESTAMP0 and grouped with the _TIMESTAMP1 below?
https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ On 2015/11/13 17:32:04, shatch wrote: > Should this be TRACE_EVENT_MARK_WITH_TIMESTAMP0 and grouped with the _TIMESTAMP1 > below? Actually not really. I asked charliea@chromium.org (who is in charge of implementing the mark & measure standardized JS API) about this, and there is a difference between an INSTANT event and a MARK event. A mark EVENT is pretty much an INSTANT event. However the MARK event get also referenced on C++ side so the JS API mark & measure can access it. If I was using MARK events, then these startup specific events would be accessible on the w3c standardized API (http://www.w3.org/TR/user-timing/), and we don't what that.
https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ On 2015/11/16 12:09:03, gabadie wrote: > On 2015/11/13 17:32:04, shatch wrote: > > Should this be TRACE_EVENT_MARK_WITH_TIMESTAMP0 and grouped with the > _TIMESTAMP1 > > below? > > Actually not really. I asked mailto:charliea@chromium.org (who is in charge of > implementing the mark & measure standardized JS API) about this, and there is a > difference between an INSTANT event and a MARK event. A mark EVENT is pretty > much an INSTANT event. However the MARK event get also referenced on C++ side so > the JS API mark & measure can access it. If I was using MARK events, then these > startup specific events would be accessible on the w3c standardized API > (http://www.w3.org/TR/user-timing/), and we don't what that. So should you be using TRACE_EVENT_PHASE_INSTANT instead, since you don't want to use a MARK event but your macro specifies TRACE_EVENT_PHASE_MARK?
Good catch Simon! I have made the fix. =) https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1410943005/diff/120001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ Oups... Turned out one commit was missing in my branch. Good catch! My bad. Fixed. =)
simonhatch@google.com changed reviewers: + simonhatch@google.com
lgtm w/ comments https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ nit: TRACE_EVENT_INSTANT_WITH_TIMESTAMP0 https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ TRACE_EVENT_FLAG_NONE | scope?
Thanks guys! :-) https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ On 2015/11/16 15:46:49, sh wrote: > TRACE_EVENT_FLAG_NONE | scope? Done. https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... base/trace_event/common/trace_event_common.h:312: #define TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ On 2015/11/16 15:46:49, sh wrote: > nit: TRACE_EVENT_INSTANT_WITH_TIMESTAMP0 Done.
On 2015/11/17 10:53:48, gabadie wrote: > Thanks guys! :-) > > https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... > File base/trace_event/common/trace_event_common.h (right): > > https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... > base/trace_event/common/trace_event_common.h:312: #define > TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ > On 2015/11/16 15:46:49, sh wrote: > > TRACE_EVENT_FLAG_NONE | scope? > > Done. > > https://codereview.chromium.org/1410943005/diff/140001/base/trace_event/commo... > base/trace_event/common/trace_event_common.h:312: #define > TRACE_EVENT_INSTANT_WITH_TIMESTAMP(category_group, name, scope, \ > On 2015/11/16 15:46:49, sh wrote: > > nit: TRACE_EVENT_INSTANT_WITH_TIMESTAMP0 > > Done. lgtm from right account this time
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, simonhatch@google.com Link to the patchset: https://codereview.chromium.org/1410943005/#ps200001 (title: "s/g_browser_process_creation_ticks/g_process_creation_ticks")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410943005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410943005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by gabadie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410943005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410943005/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
https://codereview.chromium.org/1410943005/diff/220001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1410943005/diff/220001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:441: TRACE_EVENT_INSTANT_WITH_TIMESTAMP0( Hmmm, why are these no longer in a helper method? I prefer for them to be grouped together with an explicit comment about Telemetry.
Brought back the helper method. :-) https://codereview.chromium.org/1410943005/diff/220001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1410943005/diff/220001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:441: TRACE_EVENT_INSTANT_WITH_TIMESTAMP0( On 2015/11/18 20:23:02, gab wrote: > Hmmm, why are these no longer in a helper method? I prefer for them to be > grouped together with an explicit comment about Telemetry. Done.
re-lgtm
The CQ bit was checked by gabadie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from simonhatch@chromium.org, simonhatch@google.com Link to the patchset: https://codereview.chromium.org/1410943005/#ps280001 (title: "Fixes miss leading instant events caused by patch set #14")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410943005/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410943005/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/4cec3b7462517d2b8911c131f33d141dd9bd25e0 Cr-Commit-Position: refs/heads/master@{#360592} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
