Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250743002:
Increase number of buckets for intent-to-first-commit histogram (Closed)
|
Created:
4 years, 4 months ago by pasko Modified:
4 years, 2 months ago CC:
chromium-reviews, lizeb+watch-custom-tabs_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncrease number of buckets for intent-to-first-commit histogram
The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of
buckets tuned for typical commit times of 2000ms, this change introduces
more detail around the typical times (between 500 and 600 ms).
The histogram is split into two versions: zoomed-in (more detail) and zoomed-out
(longer range). On top of the original split for herb/custom-tabs it makes 4
histograms in total (using suffixes reduces it to 2 in histograms.xml).
BUG=none
Committed: https://crrev.com/dd53cb84c484d41c7418c0f1449f4706848c9e8b
Cr-Commit-Position: refs/heads/master@{#425008}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : same renaming for ChromeGeneratedCustomTab. version of the histogram #Patch Set 4 : cleanup in histograms.xml messages #
Total comments: 4
Patch Set 5 : addressed comments by isherman@ #Patch Set 6 : split into zoomed-in and zoomed-out view with 150 buckets total #
Total comments: 3
Patch Set 7 : rebase #Patch Set 8 : shorter histograms.xml with histogram_suffixes, but not too much shorter #
Messages
Total messages: 39 (20 generated)
Description was changed from ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change makes it more granular using this formula: number_of_buckets = ln(60000) / ln (value / value - granularity), where: value = 550ms (our current median) granularity = 10ms BUG=none ========== to ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change makes it more granular using this formula: number_of_buckets = ln(60000) / ln(value / (value - granularity)), where: value = 550ms (our current median) granularity = 10ms BUG=none ==========
Description was changed from ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change makes it more granular using this formula: number_of_buckets = ln(60000) / ln(value / (value - granularity)), where: value = 550ms (our current median) granularity = 10ms BUG=none ========== to ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change makes it more granular using this formula: number_of_buckets = ln(60000) / ln(value / (value - granularity)), where: value = 550ms (our current median) granularity = 10ms BUG=none ==========
pasko@chromium.org changed reviewers: + yusufo@chromium.org
The CQ bit was checked by pasko@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...
Yusuf: PTaL
.. and the formula is explained here: https://codesearch.chromium.org/chromium/src/chrome/android/java/src/org/chro... (and is probably somewhat wrong .. but only a little)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
change lgtm, histograms related changes confused me a bit. We should have two histograms, one for regular CCT, the other for herb with different prefixes. https://codereview.chromium.org/2250743002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2250743002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5405: <histogram name="ChromeGeneratedCustomTab.IntentToPageLoadedTime" units="ms"> is there no ChromeGeneratedCustomTab.IntentToPageLoadedTime2?
pasko@chromium.org changed reviewers: + lizeb@chromium.org
yusufo: PTAL lizeb: I sent this for review when you were out, but since you'd likely be interested, PTaL https://codereview.chromium.org/2250743002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2250743002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:5405: <histogram name="ChromeGeneratedCustomTab.IntentToPageLoadedTime" units="ms"> On 2016/08/17 18:49:23, Yusuf wrote: > is there no ChromeGeneratedCustomTab.IntentToPageLoadedTime2? Ah, I was not careful. Thank you for pointing this out. Done.
On 2016/08/18 09:44:08, pasko wrote: > yusufo: PTAL > lizeb: I sent this for review when you were out, but since you'd likely be > interested, PTaL > > https://codereview.chromium.org/2250743002/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (left): > > https://codereview.chromium.org/2250743002/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:5405: <histogram > name="ChromeGeneratedCustomTab.IntentToPageLoadedTime" units="ms"> > On 2016/08/17 18:49:23, Yusuf wrote: > > is there no ChromeGeneratedCustomTab.IntentToPageLoadedTime2? > > Ah, I was not careful. Thank you for pointing this out. Done. lgtm
pasko@chromium.org changed reviewers: + isherman@chromium.org
isherman: please review changes in histograms.xml
600 buckets is *a lot*. Please note that there are real memory costs -- both client- and server-side, to additional buckets. Could you please explain why you need such a fine-grained resolution? Do you really need the fine-grainedness throughout the histogram, or only in some parts of the range? https://codereview.chromium.org/2250743002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250743002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:5530: + Deprecated 8/2016. No longer tracked. Please document the histogram that replaced it. https://codereview.chromium.org/2250743002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:7529: + Deprecated 8/2016. No longer tracked. Ditto.
On 2016/08/18 21:54:22, Ilya Sherman wrote: > 600 buckets is *a lot*. Please note that there are real memory costs -- both > client- and server-side, to additional buckets. Could you please explain why > you need such a fine-grained resolution? Do you really need the > fine-grainedness throughout the histogram, or only in some parts of the range? The timeline for Android/Beta/50Percentile shows a "regression" in the ballpark of 20ms, which is our current resolution for this value. Also, when I look at the histogram on the dashboard, I see it being quite smooth soon after 50% (on Stable as well), but not so smooth below 50, which makes me suspect that we will be seeing it jumpy from one bucket to another. PM/ENG guesswork is costly, so I thought additional data would help. We could make another histogram that stops at 700ms, which would make about 360 buckets, but we would need to keep the current histogram at ~50 buckets to cover ~1 minute. Overall this would save 200 buckets at inconvenience of looking at two histograms and potentially more changes if we fore some reason fall back to something longer than 700ms (the numbers are largely controlled by 3rd parties, so we cannot be super sure). Does this sound reasonable/worthwhile now?
The CQ bit was checked by pasko@chromium.org to run a CQ dry run
https://codereview.chromium.org/2250743002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250743002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:5530: + Deprecated 8/2016. No longer tracked. On 2016/08/18 21:54:21, Ilya Sherman wrote: > Please document the histogram that replaced it. Done. https://codereview.chromium.org/2250743002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:7529: + Deprecated 8/2016. No longer tracked. On 2016/08/18 21:54:21, Ilya Sherman wrote: > Ditto. Done.
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: Exceeded global retry quota
On 2016/08/19 16:36:24, pasko wrote: > On 2016/08/18 21:54:22, Ilya Sherman wrote: > > 600 buckets is *a lot*. Please note that there are real memory costs -- both > > client- and server-side, to additional buckets. Could you please explain why > > you need such a fine-grained resolution? Do you really need the > > fine-grainedness throughout the histogram, or only in some parts of the range? > > The timeline for Android/Beta/50Percentile shows a "regression" in the ballpark > of 20ms, which is our current resolution for this value. Also, when I look at > the histogram on the dashboard, I see it being quite smooth soon after 50% (on > Stable as well), but not so smooth below 50, which makes me suspect that we will > be seeing it jumpy from one bucket to another. PM/ENG guesswork is costly, so I > thought additional data would help. > > We could make another histogram that stops at 700ms, which would make about 360 > buckets, but we would need to keep the current histogram at ~50 buckets to cover > ~1 minute. Overall this would save 200 buckets at inconvenience of looking at > two histograms and potentially more changes if we fore some reason fall back to > something longer than 700ms (the numbers are largely controlled by 3rd parties, > so we cannot be super sure). > > Does this sound reasonable/worthwhile now? If you believe that you need very fine-grained resolution at a specific range of the histogram, I'd rather that you specify a custom bucket layout, with high-precision buckets exactly where you need them, and lower-precision buckets elsewhere. (Note: I also commented on the doc that you cc'ed me on for context -- feel free to continue the conversation either here or there.)
On 2016/08/22 23:31:25, Ilya Sherman wrote: > If you believe that you need very fine-grained resolution at a specific range of > the histogram, I'd rather that you specify a custom bucket layout, with > high-precision buckets exactly where you need them, and lower-precision buckets > elsewhere. (Note: I also commented on the doc that you cc'ed me on for context > -- feel free to continue the conversation either here or there.) Responded in the doc, where you added more folks, let's continue there, quite an interesting topic.
Description was changed from ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change makes it more granular using this formula: number_of_buckets = ln(60000) / ln(value / (value - granularity)), where: value = 550ms (our current median) granularity = 10ms BUG=none ========== to ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change will introduce more detail around the typical times (between 500 and 600 ms). The histogram is split into two versions: zoomed-in (more detail) and zoomed-out (longer range). On top of the original split for herb/custom-tabs it makes 4 histograms in total. BUG=none ==========
isherman: I reduced the bucket usage to 150 (x2 due to customtabs/herb split) as you suggested. PTAL.
On 2016/10/05 15:51:32, pasko wrote: > isherman: I reduced the bucket usage to 150 (x2 due to customtabs/herb split) as > you suggested. PTAL. isherman: friendly ping :)
Thanks, the zoomed in + zoomed out views lgtm, with one nit inline. And, sorry for the slow review! https://codereview.chromium.org/2250743002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250743002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5747: +</histogram> nit: Mebbe use a histogram_suffixes element to reduce the amount of duplication?
thank you for review https://codereview.chromium.org/2250743002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250743002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5747: +</histogram> On 2016/10/13 00:46:57, Ilya Sherman wrote: > nit: Mebbe use a histogram_suffixes element to reduce the amount of duplication? Thanks. Done. Is there an easy way to debug suffix expansion with histograms.xml?
The CQ bit was checked by pasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org, lizeb@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2250743002/#ps140001 (title: "shorter histograms.xml with histogram_suffixes, but not too much shorter")
The CQ bit was unchecked by pasko@chromium.org
Description was changed from ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change will introduce more detail around the typical times (between 500 and 600 ms). The histogram is split into two versions: zoomed-in (more detail) and zoomed-out (longer range). On top of the original split for herb/custom-tabs it makes 4 histograms in total. BUG=none ========== to ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change introduces more detail around the typical times (between 500 and 600 ms). The histogram is split into two versions: zoomed-in (more detail) and zoomed-out (longer range). On top of the original split for herb/custom-tabs it makes 4 histograms in total (using suffixes reduces it to 2 in histograms.xml). BUG=none ==========
The CQ bit was checked by pasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change introduces more detail around the typical times (between 500 and 600 ms). The histogram is split into two versions: zoomed-in (more detail) and zoomed-out (longer range). On top of the original split for herb/custom-tabs it makes 4 histograms in total (using suffixes reduces it to 2 in histograms.xml). BUG=none ========== to ========== Increase number of buckets for intent-to-first-commit histogram The CustomTabs.IntentToFirstCommitNavigationTime histogram took the number of buckets tuned for typical commit times of 2000ms, this change introduces more detail around the typical times (between 500 and 600 ms). The histogram is split into two versions: zoomed-in (more detail) and zoomed-out (longer range). On top of the original split for herb/custom-tabs it makes 4 histograms in total (using suffixes reduces it to 2 in histograms.xml). BUG=none Committed: https://crrev.com/dd53cb84c484d41c7418c0f1449f4706848c9e8b Cr-Commit-Position: refs/heads/master@{#425008} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/dd53cb84c484d41c7418c0f1449f4706848c9e8b Cr-Commit-Position: refs/heads/master@{#425008}
Message was sent while issue was closed.
https://codereview.chromium.org/2250743002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2250743002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5747: +</histogram> On 2016/10/13 11:47:09, pasko wrote: > On 2016/10/13 00:46:57, Ilya Sherman wrote: > > nit: Mebbe use a histogram_suffixes element to reduce the amount of > duplication? > > Thanks. Done. Is there an easy way to debug suffix expansion with > histograms.xml? I think extract_histograms.py expands out all the suffixes? Otherwise, I'm not sure about an easy way to debug it, as usually it's pretty straightforward. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
