Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2811003002:
Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ (Closed)
DescriptionExperiment with USER_VISIBLE priority for update_jumplisticons_task_runner_
It is preferable to use BACKGROUND priority for jumplist update thread
as it is now. This CL is to see how long it takes for jumplist update if
we assign the thread USER_VISIBLE priority.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2811003002
Cr-Commit-Position: refs/heads/master@{#463812}
Committed: https://chromium.googlesource.com/chromium/src/+/a70a095814dfc2530cc653d92962f1bc0b83987d
Patch Set 1 #Patch Set 2 : Update function names temporarily for UMA metric tracking. #
Total comments: 3
Messages
Total messages: 23 (13 generated)
Description was changed from ========== Experiment with USER_VISIBLE priority. BUG=40407, 179576 ========== to ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update tasks as it is now. This CL is to see how long it takes for jumplist update if we assign the task USER_VISIBLE priority. BUG=40407, 179576 ==========
Description was changed from ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update tasks as it is now. This CL is to see how long it takes for jumplist update if we assign the task USER_VISIBLE priority. BUG=40407, 179576 ========== to ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. If it still takes longer time than before migrating jumplist tasks from FILE thread to base/task_scheduler, we may need to revert to FILE thread for further investigation. BUG=40407, 179576 ==========
Description was changed from ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. If it still takes longer time than before migrating jumplist tasks from FILE thread to base/task_scheduler, we may need to revert to FILE thread for further investigation. BUG=40407, 179576 ========== to ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. If it still takes longer time than before migrating the jumplist tasks from the FILE thread to base/task_scheduler, we may need to revert to FILE thread for further investigation. BUG=40407, 179576 ==========
Description was changed from ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. If it still takes longer time than before migrating the jumplist tasks from the FILE thread to base/task_scheduler, we may need to revert to FILE thread for further investigation. BUG=40407, 179576 ========== to ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. If it still takes a longer time than before migrating the jumplist tasks from the FILE thread to base/task_scheduler, we may need to revert to FILE thread for further investigation. BUG=40407, 179576 ==========
Description was changed from ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. If it still takes a longer time than before migrating the jumplist tasks from the FILE thread to base/task_scheduler, we may need to revert to FILE thread for further investigation. BUG=40407, 179576 ========== to ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + gab@chromium.org
A small change for experimental purpose. PTAL~
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.
Priority experiment lgtm (eager to know result), don't bother renaming all methods though. https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:234: void RunUpdateJumpListUserVisiblePriority( Don't rename methods just for this (same for others)
https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:234: void RunUpdateJumpListUserVisiblePriority( On 2017/04/11 20:25:31, gab wrote: > Don't rename methods just for this (same for others) This CL will be included in the Canary build tomorrow morning and start to hit Canary users gradually, probably 60% tomorrow. If I don't rename it, it is hard for me to tell the improvement precisely. I will revert them in a day or two. How do you think?
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": 20001, "attempt_start_ts": 1491948603105660,
"parent_rev": "6801f7c548cf389776a64dabc3cc55d7fb333117", "commit_rev":
"a70a095814dfc2530cc653d92962f1bc0b83987d"}
Message was sent while issue was closed.
Description was changed from ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. BUG=40407, 179576 ========== to ========== Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_ It is preferable to use BACKGROUND priority for jumplist update thread as it is now. This CL is to see how long it takes for jumplist update if we assign the thread USER_VISIBLE priority. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2811003002 Cr-Commit-Position: refs/heads/master@{#463812} Committed: https://chromium.googlesource.com/chromium/src/+/a70a095814dfc2530cc653d92962... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a70a095814dfc2530cc653d92962...
Message was sent while issue was closed.
https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:234: void RunUpdateJumpListUserVisiblePriority( On 2017/04/11 21:09:28, chengx wrote: > On 2017/04/11 20:25:31, gab wrote: > > Don't rename methods just for this (same for others) > > This CL will be included in the Canary build tomorrow morning and start to hit > Canary users gradually, probably 60% tomorrow. If I don't rename it, it is hard > for me to tell the improvement precisely. I will revert them in a day or two. > How do you think? I disagree. 1) UMA has version filters, you should use those. 2) You're renaming the methods, not the histograms so what does that change?
Message was sent while issue was closed.
On 2017/04/12 19:25:47, gab wrote: > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > File chrome/browser/win/jumplist.cc (right): > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > chrome/browser/win/jumplist.cc:234: void RunUpdateJumpListUserVisiblePriority( > On 2017/04/11 21:09:28, chengx wrote: > > On 2017/04/11 20:25:31, gab wrote: > > > Don't rename methods just for this (same for others) > > > > This CL will be included in the Canary build tomorrow morning and start to hit > > Canary users gradually, probably 60% tomorrow. If I don't rename it, it is > hard > > for me to tell the improvement precisely. I will revert them in a day or two. > > How do you think? > > I disagree. > > 1) UMA has version filters, you should use those. > 2) You're renaming the methods, not the histograms so what does that change? The execution time of the task will be recorded as Profiler, rather than the histogram. For the Profiler, I don't see any version filters. This is the UMA link for RunUpdateJumplist task's execution time. https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver... With the method renamed, there will be a new one showing up tomorrow reflecting the Canary users who have got the new build. The old one will still exist tomorrow for those users who haven't got the new Canary build.
Message was sent while issue was closed.
On 2017/04/12 20:06:50, chengx wrote: > On 2017/04/12 19:25:47, gab wrote: > > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > > File chrome/browser/win/jumplist.cc (right): > > > > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > > chrome/browser/win/jumplist.cc:234: void RunUpdateJumpListUserVisiblePriority( > > On 2017/04/11 21:09:28, chengx wrote: > > > On 2017/04/11 20:25:31, gab wrote: > > > > Don't rename methods just for this (same for others) > > > > > > This CL will be included in the Canary build tomorrow morning and start to > hit > > > Canary users gradually, probably 60% tomorrow. If I don't rename it, it is > > hard > > > for me to tell the improvement precisely. I will revert them in a day or > two. > > > How do you think? > > > > I disagree. > > > > 1) UMA has version filters, you should use those. > > 2) You're renaming the methods, not the histograms so what does that change? > > The execution time of the task will be recorded as Profiler, rather than the > histogram. For the Profiler, I don't see any version filters. > This is the UMA link for RunUpdateJumplist task's execution time. > > https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver... > > With the method renamed, there will be a new one showing up tomorrow reflecting > the Canary users who have got the new build. The old one will still exist > tomorrow for those users who haven't got the new Canary build. You can filter to e.g. 59.0.3068.1-W-CANARY (you'll want 59.0.3969.0 tomorrow of course): https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver...
Message was sent while issue was closed.
On 2017/04/12 20:37:41, gab wrote: > On 2017/04/12 20:06:50, chengx wrote: > > On 2017/04/12 19:25:47, gab wrote: > > > > > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > > > File chrome/browser/win/jumplist.cc (right): > > > > > > > > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > > > chrome/browser/win/jumplist.cc:234: void > RunUpdateJumpListUserVisiblePriority( > > > On 2017/04/11 21:09:28, chengx wrote: > > > > On 2017/04/11 20:25:31, gab wrote: > > > > > Don't rename methods just for this (same for others) > > > > > > > > This CL will be included in the Canary build tomorrow morning and start to > > hit > > > > Canary users gradually, probably 60% tomorrow. If I don't rename it, it is > > > hard > > > > for me to tell the improvement precisely. I will revert them in a day or > > two. > > > > How do you think? > > > > > > I disagree. > > > > > > 1) UMA has version filters, you should use those. > > > 2) You're renaming the methods, not the histograms so what does that change? > > > > The execution time of the task will be recorded as Profiler, rather than the > > histogram. For the Profiler, I don't see any version filters. > > This is the UMA link for RunUpdateJumplist task's execution time. > > > > > https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver... > > > > With the method renamed, there will be a new one showing up tomorrow > reflecting > > the Canary users who have got the new build. The old one will still exist > > tomorrow for those users who haven't got the new Canary build. > > You can filter to e.g. 59.0.3068.1-W-CANARY (you'll want 59.0.3969.0 tomorrow of > course): > > https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver... Ahhhhhh... You saved my life! Thanks!!!
Message was sent while issue was closed.
Np :), also, assuming you know of these but in case you don't : On web: http://omahaproxy.appspot.com : active and previous release on each channel http://omahaproxy.appspot.com/history : full release history Locally: git find-releases <sha-1> are all very useful when working with commits vs versions. Cheers, Gab Le mer. 12 avr. 2017 17:01, <chengx@chromium.org> a écrit : > On 2017/04/12 20:37:41, gab wrote: > > On 2017/04/12 20:06:50, chengx wrote: > > > On 2017/04/12 19:25:47, gab wrote: > > > > > > > > > > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > > > > File chrome/browser/win/jumplist.cc (right): > > > > > > > > > > > > > > > https://codereview.chromium.org/2811003002/diff/20001/chrome/browser/win/jump... > > > > chrome/browser/win/jumplist.cc:234: void > > RunUpdateJumpListUserVisiblePriority( > > > > On 2017/04/11 21:09:28, chengx wrote: > > > > > On 2017/04/11 20:25:31, gab wrote: > > > > > > Don't rename methods just for this (same for others) > > > > > > > > > > This CL will be included in the Canary build tomorrow morning and > start > to > > > hit > > > > > Canary users gradually, probably 60% tomorrow. If I don't rename > it, it > is > > > > hard > > > > > for me to tell the improvement precisely. I will revert them in a > day or > > > two. > > > > > How do you think? > > > > > > > > I disagree. > > > > > > > > 1) UMA has version filters, you should use those. > > > > 2) You're renaming the methods, not the histograms so what does that > change? > > > > > > The execution time of the task will be recorded as Profiler, rather > than the > > > histogram. For the Profiler, I don't see any version filters. > > > This is the UMA link for RunUpdateJumplist task's execution time. > > > > > > > > > > https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver... > > > > > > With the method renamed, there will be a new one showing up tomorrow > > reflecting > > > the Canary users who have got the new build. The old one will still > exist > > > tomorrow for those users who haven't got the new Canary build. > > > > You can filter to e.g. 59.0.3068.1-W-CANARY (you'll want 59.0.3969.0 > tomorrow > of > > course): > > > > > > https://uma.googleplex.com/p/chrome/profiler/?end_date=20170411&ui_chrome_ver... > > Ahhhhhh... You saved my life! Thanks!!! > > https://codereview.chromium.org/2811003002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
