Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559823004:
Android: Clean up histogram buckets range (Closed)
DescriptionAndroid: Clean up histogram buckets range
Startup.FirstCommitNavigationTime2
BUG=668726
Committed: https://crrev.com/2102529172119d82b237672850732c22d4037ae6
Cr-Commit-Position: refs/heads/master@{#438872}
Patch Set 1 #Patch Set 2 : Use zoomedIn and zoomedOut. #
Total comments: 5
Patch Set 3 : Add comments. #
Messages
Total messages: 31 (18 generated)
wnwen@chromium.org changed reviewers: + pasko@chromium.org
Hi Pasko, Would reducing the number of buckets and increasing the range be acceptable for this startup metric? Is it used as part of another pipeline? Thanks, Peter
On 2016/12/08 20:17:12, Peter Wen wrote: > Hi Pasko, > > Would reducing the number of buckets and increasing the range be acceptable for > this startup metric? Is it used as part of another pipeline? We watch the timeline of the median and other stats of this histogram. By looking at the newly proposed buckets+range I am not convinced that it won't lead to more ups/downs on the timeline because of noise. Each time a jump happens folks get worried and a complex process starts where we try to reason about the likelihood if this even happening because we changed something in our code, or GSA did, and a myriad of other factors. The UMA team suggested a ZoomedIn/ZoomedOut approach that saves buckets without loosing precision in a subrange that we care most. See: https://codereview.chromium.org/2250743002 We could use it for added consistency, WDYT? Also, I am not worried about the overflow bucket: we cannot improve it, regressions are unlikely as well, and even if the tail moves, the users probably won't notice.
wnwen@chromium.org changed reviewers: + asvitkine@chromium.org
@pasko - Nice, done. PTAL. @asvitkine - OWNERS for histogram.xml and question: Should we create a FirstCommitNavigationTime3 with suffixes or can we add suffixes to the existing FirstCommitNavigationTime2? Thanks!
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Friendly ping. :)
lgtm % comment https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:212: 50, TimeUnit.MINUTES.toMillis(10), TimeUnit.MILLISECONDS, 50); Is the 50 for the min value intentional? If so, add a comment.
lgtm, thank you https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:203: // Current median is 550ms, and long tail is very long. ZoomedIn gives good view of the wow, I did not realize that so drastically! yay! Seems like it is mostly coming from AMP (See old timeline for CustomTabs.IntentToFirstCommitNavigationTime), which seems to have coincided with the the rollout of AMP in GSA. https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:212: 50, TimeUnit.MINUTES.toMillis(10), TimeUnit.MILLISECONDS, 50); On 2016/12/15 00:34:37, Alexei Svitkine (OO from 15th) wrote: > Is the 50 for the min value intentional? If so, add a comment. I think the comment just above sufficiently explains it, up to you guys.
The CQ bit was checked by wnwen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:203: // Current median is 550ms, and long tail is very long. ZoomedIn gives good view of the On 2016/12/15 13:36:19, pasko wrote: > wow, I did not realize that so drastically! yay! > > Seems like it is mostly coming from AMP (See old timeline for > CustomTabs.IntentToFirstCommitNavigationTime), which seems to have coincided > with the the rollout of AMP in GSA. Yay! I didn't have context to know whether that was good or bad. :) https://codereview.chromium.org/2559823004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:212: 50, TimeUnit.MINUTES.toMillis(10), TimeUnit.MILLISECONDS, 50); On 2016/12/15 13:36:19, pasko wrote: > On 2016/12/15 00:34:37, Alexei Svitkine (OO from 15th) wrote: > > Is the 50 for the min value intentional? If so, add a comment. > > I think the comment just above sufficiently explains it, up to you guys. Done adding a quick comment and reference to other histogram.
wnwen@chromium.org changed reviewers: + mariakhomenko@chromium.org
Thank you for the reviews, Egor and Alexei! +mariakhomenko@ for OWNERS TabWebContentsObserver.java.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wnwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2559823004/#ps40001 (title: "Add comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by wnwen@chromium.org
lgtm
The CQ bit was checked by wnwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481824510440900,
"parent_rev": "1ce50b5229537ff9388db75ae5cc42c07fc24e23", "commit_rev":
"bd5bf581573a76ea0beaf85569b4e39fc936ea10"}
Message was sent while issue was closed.
Description was changed from ========== Android: Clean up histogram buckets range Startup.FirstCommitNavigationTime2 BUG=668726 ========== to ========== Android: Clean up histogram buckets range Startup.FirstCommitNavigationTime2 BUG=668726 Review-Url: https://codereview.chromium.org/2559823004 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Android: Clean up histogram buckets range Startup.FirstCommitNavigationTime2 BUG=668726 Review-Url: https://codereview.chromium.org/2559823004 ========== to ========== Android: Clean up histogram buckets range Startup.FirstCommitNavigationTime2 BUG=668726 Committed: https://crrev.com/2102529172119d82b237672850732c22d4037ae6 Cr-Commit-Position: refs/heads/master@{#438872} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2102529172119d82b237672850732c22d4037ae6 Cr-Commit-Position: refs/heads/master@{#438872} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
