Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964873002:
Log the ratio of the duration spent adding the two JumpList categories (Closed)
DescriptionLog the ratio of the duration spent adding the two JumpList categories
This CL logs the ratio of the duration spent adding the most-visited
category compared to the duration spent adding the recently-closed
category. In crrev/2965773002, a new timeout threshold is needed after
updating the usage of JumpListUpdater::AddCustomCategory. See more
details in the last paragraph of the CL description. This CL logs the
ratio to help generate a good threshold.
BUG=733034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2964873002
Cr-Commit-Position: refs/heads/master@{#484452}
Committed: https://chromium.googlesource.com/chromium/src/+/1fca937e8656e8b5c5842bf7e99eecfe89b18cad
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments from isherman@ #Patch Set 3 : Remove explicit cast, update comments #
Messages
Total messages: 43 (34 generated)
Description was changed from ========== Log the times the runtime of adding most visited category is that of adding recently closed category BUG= ========== to ========== Log the times the runtime of adding most visited category is that of adding recently closed category BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Log the times the runtime of adding most visited category is that of adding recently closed category BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Log the times the runtime of adding most visited category is that of adding recently closed category BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Log the times the runtime of adding most visited category is that of adding recently closed category BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Log relative runtime of adding JumpList categories BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Log relative runtime of adding JumpList categories BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Log relative runtime of adding JumpList categories In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. This CL is to get UMA data for generating this threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
chengx@chromium.org changed reviewers: + gab@chromium.org, isherman@chromium.org
PTAL~
Metrics LGTM % comments: https://codereview.chromium.org/2964873002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2964873002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:770: (add_category_total_time - most_visited_category_time).InMillisecondsF(); nit: Mebbe move this into the if-stmt? https://codereview.chromium.org/2964873002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:776: (int)(most_visited_over_recently_closed * 10)); Why are you multiplying by 10? It's not clear to me from this code, nor from the histogram description. https://codereview.chromium.org/2964873002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2964873002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:87225: +<histogram name="WinJumplist.AddCategoryTimeComparison"> Please add a units attribute, even if it's just something like units="scaled ratio". https://codereview.chromium.org/2964873002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:87229: + category is that of adding recently-closed category. Optional nit: Perhaps: "The ratio of the duration spent adding the most-visited category compared to the duration spent adding the recently-closed category, multiplied by 10."
The CQ bit was checked by chengx@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...
Description was changed from ========== Log relative runtime of adding JumpList categories In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. This CL is to get UMA data for generating this threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Log the ratio of the duration spent adding the two JumpList categories This CL logs the ratio of the duration spent adding the most-visited category compared to the duration spent adding the recently-closed category. In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. The ratio is logged to help generate a good threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Hi Ilya, I've addressed your comments in the new patch set. Thanks! https://codereview.chromium.org/2964873002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2964873002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:770: (add_category_total_time - most_visited_category_time).InMillisecondsF(); On 2017/07/04 03:41:19, Ilya Sherman wrote: > nit: Mebbe move this into the if-stmt? Done. https://codereview.chromium.org/2964873002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:776: (int)(most_visited_over_recently_closed * 10)); On 2017/07/04 03:41:19, Ilya Sherman wrote: > Why are you multiplying by 10? It's not clear to me from this code, nor from > the histogram description. The ration is typically between 1 and 10. I multiply it by 10 to retain some precision as the UMA histogram casts the floating point to integer. I've added the comment. https://codereview.chromium.org/2964873002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2964873002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:87225: +<histogram name="WinJumplist.AddCategoryTimeComparison"> On 2017/07/04 03:41:19, Ilya Sherman wrote: > Please add a units attribute, even if it's just something like units="scaled > ratio". Done. https://codereview.chromium.org/2964873002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:87229: + category is that of adding recently-closed category. On 2017/07/04 03:41:19, Ilya Sherman wrote: > Optional nit: Perhaps: "The ratio of the duration spent adding the most-visited > category compared to the duration spent adding the recently-closed category, > multiplied by 10." Much more clear. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chengx@chromium.org changed reviewers: + pmonette@chromium.org
+pmonette@ for jumplist.cc
Description was changed from ========== Log the ratio of the duration spent adding the two JumpList categories This CL logs the ratio of the duration spent adding the most-visited category compared to the duration spent adding the recently-closed category. In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. The ratio is logged to help generate a good threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Log the ratio of the duration spent adding the two JumpList categories This CL logs the ratio of the duration spent adding the most-visited category compared to the duration spent adding the recently-closed category. In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. This CL logs the ratio to help generate a good threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
lgtm
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, pmonette@chromium.org Link to the patchset: https://codereview.chromium.org/2964873002/#ps80001 (title: "Remove explicit cast, update 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 commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by chengx@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: This issue passed the CQ dry run.
The CQ bit was checked by chengx@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: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The CQ bit was unchecked by chengx@chromium.org
The CQ bit was checked by chengx@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: This issue passed the CQ dry run.
The CQ bit was checked by chengx@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": 80001, "attempt_start_ts": 1499310449337930,
"parent_rev": "468c6ca853b5f75fc61897c87ee815c18605d407", "commit_rev":
"1fca937e8656e8b5c5842bf7e99eecfe89b18cad"}
Message was sent while issue was closed.
Description was changed from ========== Log the ratio of the duration spent adding the two JumpList categories This CL logs the ratio of the duration spent adding the most-visited category compared to the duration spent adding the recently-closed category. In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. This CL logs the ratio to help generate a good threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Log the ratio of the duration spent adding the two JumpList categories This CL logs the ratio of the duration spent adding the most-visited category compared to the duration spent adding the recently-closed category. In crrev/2965773002, a new timeout threshold is needed after updating the usage of JumpListUpdater::AddCustomCategory. See more details in the last paragraph of the CL description. This CL logs the ratio to help generate a good threshold. BUG=733034 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2964873002 Cr-Commit-Position: refs/heads/master@{#484452} Committed: https://chromium.googlesource.com/chromium/src/+/1fca937e8656e8b5c5842bf7e99e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1fca937e8656e8b5c5842bf7e99e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
