Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831433003:
Time out jumplist update for very slow or busy OS (Closed)
DescriptionTime out jumplist update for very slow or busy OS
Notifying OS about the jumplist update consists of 4 steps in order:
BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of
which shouldn't take long.
However, UMA data shows that each step can take up to tens of seconds
for some Chrome users. Among the 4 steps, AddTasks is in the best
condition, while CommitUpdate is in the worst condition.
Since they are mostly Windows API calls, we can assume this is due to
user machines (which can be very slow or busy) with a pretty high
confidence, and also there's not much we can improve on Chrome's side.
For these situations, we should stop jumplist update to avoid making
things worse. Code-wise, we stop the update if BeginUpdate takes more
than 500 ms which is the cutoff for 99.5% Canary JumpList updates, as
the following steps (old icons' deletion, new icons' creation,
AddCustomCategory, CommitUpdate, etc) are very likely to take a
long time too.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2831433003
Cr-Commit-Position: refs/heads/master@{#467009}
Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0f8e87de02932
Patch Set 1 #Patch Set 2 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into timeoutjumplistupdater… #
Total comments: 9
Patch Set 3 : Address comments #Patch Set 4 : Add myself to be an owner of jumplist* files #
Total comments: 6
Patch Set 5 : Mark retired metrics obsolete in histograms.xml #
Total comments: 10
Patch Set 6 : Address comments #
Total comments: 6
Patch Set 7 : Remove myself from being an owner and remove Windows version check #
Messages
Total messages: 62 (44 generated)
Description was changed from ========== Time out jumplist update for users with very slow OS BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks seems to in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by users' machine with a pretty high confidence, and also there's not much we can improve on Chrome's side. We should simply stop jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is likely to take more time. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks seems to in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by users' machine with a pretty high confidence, and also there's not much we can improve on Chrome's side. We should simply stop jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is likely to take more time. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks seems to in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by users' machine with a pretty high confidence, and also there's not much we can improve on Chrome's side. We should simply stop jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is likely to take more time. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks seems to in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by users' machine with a pretty high confidence, and also there's not much we can improve on Chrome's side. We should simply stop jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is likely to take more time. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence,and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even more time. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence,and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even more time. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence,and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence,and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is caused by user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
PTAL~
I'm not seeing the problem. Looking at stats for M60 over a 7d aggregation, the WinJumplistUpdater timings look reasonable even at the 99th %ile. Am I looking at the wrong stats? Should you wait until getting data from a larger population before making code changes? That said, would it help to do the jumplist updating on a background-priority thread? The OS should take care of making sure that a BG thread isn't interfering with the user experience. This could cause large delays before the jumplist is updated, so you may want to coalesce a series of updates in a smart way.
On 2017/04/19 13:39:43, grt (UTC plus 1) wrote: > I'm not seeing the problem. Looking at stats for M60 over a 7d aggregation, the > WinJumplistUpdater timings look reasonable even at the 99th %ile. Am I looking > at the wrong stats? Should you wait until getting data from a larger population > before making code changes? > > That said, would it help to do the jumplist updating on a background-priority > thread? The OS should take care of making sure that a BG thread isn't > interfering with the user experience. This could cause large delays before the > jumplist is updated, so you may want to coalesce a series of updates in a smart > way. M60 is the correct stats. I am looking at this one, which should be similar to what you are looking at. I think this data is enough. I have monitored the total cost of these 4 steps for weeks, and now I break it apart. https://uma.googleplex.com/p/chrome/histograms/?endDate=20170418&dayCount=1&h... The left column is the user count/percentage, and the right column is the sample count/percentage. I would like to look at the user percentile, i.e., the left column. It should be the case that for those users whose jumplist updater takes seconds to run, they contribute much less to the samples count than "healthy" users. If looking at the left column, we can roughly conclude that, for 1% users it takes 1+ second just to run BeginUpdate (3rd histogram), and 2+ seconds just to run CommitUpdate (last histogram). Note that for example, CommitUpdate simply calls one WinAPI (see link below), so there is not much we can do. https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist_updater.cc?t... Maybe these samples with long running time were generated when the OS was busy, and there are samples with normal running time generated from the same OS when it's busy. In this case, timing out the jumplist update with long running time should be okay for these users, as their jumplist can still update when the OS isn't busy. Agree that using a background-priority thread helps the user experience, and I'll make the thread BG finally. I am using USER_VISIBLE priority just to make sure the current performance evaluation is fair, because the FILE thread is with USER_VISIBLE priority. However, the abnormally long running time of for example, CommitUpdate should be avoided anyway IMHO, as we don't want to make jumplist grab too much OS resource. It's a huge waste of power anyway. For this concern, I don't think using a BG thread helps much.
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 ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should simply stop jumplist update for them as it consumes too much machine resources. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we exist the jumplist update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we stop the jumplist update if BeginUpdate and/or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we stop the jumplist update if BeginUpdate and/or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we stop the update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Okay, the rationale seems sane to me. I have a few small nits and a few bigger questions below. Thanks. https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:71: const int kTimeOutForJumplistUpdateInMS = 500; nit: constexpr base::TimeDelta kTimeOutForJumplistUpdate = base::TimeDelta::FromMilliseconds(500); https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:204: if (begin_update_timer.Elapsed().InMilliseconds() >= nit: if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate) https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: return false; don't these early returns skew the data in WinJumplist.UpdateJumpListDuration? should you also not delete the icon files in these early return cases since they're unused? could this be contributing to the explosion of icon files? https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:271: if (add_category_timer.Elapsed().InMilliseconds() >= here, too
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 ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we stop the update if BeginUpdate or AddCustomCategory takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for users with very slow OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these users, we should stop jumplist update for them as it consumes too much machine resource. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following CommitUpdate is very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps are very likely to take even longer. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + isherman@chromium.org
PTAL~ Adding isherman@ for histogram.xml https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:71: const int kTimeOutForJumplistUpdateInMS = 500; On 2017/04/20 12:48:53, grt (UTC plus 1) wrote: > nit: > constexpr base::TimeDelta kTimeOutForJumplistUpdate = > base::TimeDelta::FromMilliseconds(500); Done. https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:204: if (begin_update_timer.Elapsed().InMilliseconds() >= On 2017/04/20 12:48:53, grt (UTC plus 1) wrote: > nit: > if (begin_update_timer.Elapsed() >= kTimeOutForJumplistUpdate) Done. https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: return false; On 2017/04/20 12:48:53, grt (UTC plus 1) wrote: > don't these early returns skew the data in WinJumplist.UpdateJumpListDuration? Re: Since I am logging all its 4 steps' (BeginUpdate, CommitUpdate, etc) running time to UMA, WinJumplist.UpdateJumpListDuration as a whole is not that important now. Actually, I've retired this metric in the new patch set because originally we have CreateIcons1 --> CreateIcons2 --> AddCategory1 --> AddCategory2 Now it's changed to CreateIcons1 --> AddCategory1 --> CreateIcons2 --> AddCategory2 Since if AddCategory1 returns early, there's no need to run CreateIcons2. However after this change, UpdateJumpListDuration is no longer accurate, so it's retired. > Should you also not delete the icon files in these early return cases since > they're unused? Re: I agree this is the correct thing to do, but only if the situation allows to do so which however seems not. Here are more details. CreateIconFiles must be called before AddCustomCategory, which means the the new icons are already created in the jumplist folder. Due to the imperfectness of file deletion, now CreateIconFiles is only called when the JumpListIcons folder is empty, i.e., when all the old icons are deleted. I think this is the safest way to ensure the folder is not bloated. It's really a dilemma. > Could this be contributing to the explosion of icon files? Re: No, it won't. These newly created but unused icons will be deleted in the next jumplist update. If the deletion fails, no more new icons will be created. https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:271: if (add_category_timer.Elapsed().InMilliseconds() >= On 2017/04/20 12:48:53, grt (UTC plus 1) wrote: > here, too I decided to only keep a timer for BeginUpdate for now to see what happens first. So this section of code is gone now.
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 ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps are very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take even longer. BUG=40407, 179576 ==========
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take even longer. BUG=40407, 179576 ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 ==========
https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/OWNERS File chrome/browser/win/OWNERS (right): https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/OWNE... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org Would it make sense to create a jumplist/ directory, rather than adding per-file entries? https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/OWNE... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org Just to double-check: To whom are you planning to send your code reviews if you're added as an OWNER here? Would it make sense to have multiple owners? https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (left): https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:225: most_visited_pages.size() + recently_closed_pages.size()); Should this metric be marked as obsolete as well?
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...
Hi Ilya, PTAL~ https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/OWNERS File chrome/browser/win/OWNERS (right): https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/OWNE... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org On 2017/04/20 21:24:24, Ilya Sherman wrote: > Just to double-check: To whom are you planning to send your code reviews if > you're added as an OWNER here? Would it make sense to have multiple owners? My understanding is that grt@, gab@ and pmonette@ are owners of all the files in this directory. I'll still send my code reviews to them even after I add myself to the owner of jumplist*. https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/OWNE... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org On 2017/04/20 21:24:24, Ilya Sherman wrote: > Would it make sense to create a jumplist/ directory, rather than adding > per-file entries? It makes sense to me, but I'm not sure if it's good to create sub-dirs each of which only contains a few files. https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (left): https://codereview.chromium.org/2831433003/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:225: most_visited_pages.size() + recently_closed_pages.size()); On 2017/04/20 21:24:24, Ilya Sherman wrote: > Should this metric be marked as obsolete as well? Yes, it's obsolete. Now it's fixed in histograms.xml.
histograms.xml lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2831433003/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:259: return false; On 2017/04/20 21:01:36, chengx wrote: > On 2017/04/20 12:48:53, grt (UTC plus 1) wrote: > > don't these early returns skew the data in WinJumplist.UpdateJumpListDuration? > > Re: Since I am logging all its 4 steps' (BeginUpdate, CommitUpdate, etc) running > time to UMA, WinJumplist.UpdateJumpListDuration as a whole is not that important > now. Actually, I've retired this metric in the new patch set because originally > we have > CreateIcons1 --> CreateIcons2 --> AddCategory1 --> AddCategory2 > Now it's changed to > CreateIcons1 --> AddCategory1 --> CreateIcons2 --> AddCategory2 > > Since if AddCategory1 returns early, there's no need to run CreateIcons2. > However after this change, UpdateJumpListDuration is no longer accurate, so it's > retired. > > > Should you also not delete the icon files in these early return cases since > > they're unused? > Re: I agree this is the correct thing to do, but only if the situation allows to > do so which however seems not. Here are more details. > > CreateIconFiles must be called before AddCustomCategory, which means the the new > icons are already created in the jumplist folder. Due to the imperfectness of > file deletion, now CreateIconFiles is only called when the JumpListIcons folder > is empty, i.e., when all the old icons are deleted. I think this is the safest > way to ensure the folder is not bloated. It's really a dilemma. > > > Could this be contributing to the explosion of icon files? > Re: No, it won't. These newly created but unused icons will be deleted in the > next jumplist update. If the deletion fails, no more new icons will be created. > Please leave a doc comment here near the first call to CreateIconFiles explaining that icon files may be left behind on early exit, but that they will be deleted on the next jumplist update. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:68: constexpr base::TimeDelta kDelayForJumplistUpdateInMS = remove "InMS" since this is a TimeDelta (as i proposed previously). same below. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:185: // Updates the application JumpList. please document that any error along the way results in the old jumplist being left as-is, but without icon files. (assuming that's correct.) https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:186: bool UpdateJumpList(const wchar_t* app_id, this return value is undocumented and ignored. how about making this function return void? otherwise, does it make sense to UMA the result? https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:191: // JumpList is implemented only on Windows 7 or later. This comment is out-of-date -- Chrome doesn't run on anything lower than Win7. Please update these comments (and JumpListUpdater::IsEnabled) so that they state the right thing. To be honest, this comment can simply be removed; "if (!IsEnabled()) return;" is self-documenting. feel free to do this in a separate cl. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:235: bool is_dir_existed_and_empty = is_dir_existed_and_empty -> should_create_icons
PTAL~ https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:68: constexpr base::TimeDelta kDelayForJumplistUpdateInMS = On 2017/04/21 10:11:25, grt (UTC plus 2) wrote: > remove "InMS" since this is a TimeDelta (as i proposed previously). same below. Done. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:185: // Updates the application JumpList. On 2017/04/21 10:11:25, grt (UTC plus 2) wrote: > please document that any error along the way results in the old jumplist being > left as-is, but without icon files. (assuming that's correct.) Documentation added. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:186: bool UpdateJumpList(const wchar_t* app_id, On 2017/04/21 10:11:25, grt (UTC plus 2) wrote: > this return value is undocumented and ignored. how about making this function > return void? otherwise, does it make sense to UMA the result? I've updated it to return void. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:191: // JumpList is implemented only on Windows 7 or later. On 2017/04/21 10:11:25, grt (UTC plus 2) wrote: > This comment is out-of-date -- Chrome doesn't run on anything lower than Win7. > Please update these comments (and JumpListUpdater::IsEnabled) so that they state > the right thing. To be honest, this comment can simply be removed; "if > (!IsEnabled()) return;" is self-documenting. feel free to do this in a separate > cl. Comments removed. https://codereview.chromium.org/2831433003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:235: bool is_dir_existed_and_empty = On 2017/04/21 10:11:25, grt (UTC plus 2) wrote: > is_dir_existed_and_empty -> should_create_icons Done.
https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWNERS File chrome/browser/win/OWNERS (right): https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWN... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org While I am very excited about the improvements you've been making in these files, I think you could use some more time absorbing Chromium and Windows styles and techniques. Feel free to ping me, gab@, and/or pmonette@ down the road if you'd like to try again. With luck, the jumplist code will soon reach steady-state thanks to your efforts. Please let any of us know if you think that not being an OWNER is getting in the way of your ability to make the progress you'd like to be making. We'll see what we can do about that. https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist_updater.cc (right): https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist_updater.cc:114: return base::win::GetVersion() >= base::win::VERSION_WIN7 && please remove this check and "#include "base/win/windows_version.h"" above.
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...
PTAL~ https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWNERS File chrome/browser/win/OWNERS (right): https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWN... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org On 2017/04/24 09:15:07, grt (UTC plus 2) wrote: > While I am very excited about the improvements you've been making in these > files, I think you could use some more time absorbing Chromium and Windows > styles and techniques. Feel free to ping me, gab@, and/or pmonette@ down the > road if you'd like to try again. With luck, the jumplist code will soon reach > steady-state thanks to your efforts. Please let any of us know if you think that > not being an OWNER is getting in the way of your ability to make the progress > you'd like to be making. We'll see what we can do about that. You are right. Although I have touched a lot of code in the jumplist files, I need to absorb more chromium/windows technique and code style to be a good reviewer. So maybe I'll try later on. I had the idea of adding myself to be an owner of the jumplist code simply because I wanted to be aware of future changes on the jumplist code. Someone changes my code may not know why I implemented it in that way. Maybe cc'ing me about the CLs changing the jumplist implementation if there is any in the future is a better idea. https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist_updater.cc (right): https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist_updater.cc:114: return base::win::GetVersion() >= base::win::VERSION_WIN7 && On 2017/04/24 09:15:07, grt (UTC plus 2) wrote: > please remove this check and "#include "base/win/windows_version.h"" above. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWNERS File chrome/browser/win/OWNERS (right): https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWN... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org On 2017/04/24 17:24:40, chengx wrote: > On 2017/04/24 09:15:07, grt (UTC plus 2) wrote: > > While I am very excited about the improvements you've been making in these > > files, I think you could use some more time absorbing Chromium and Windows > > styles and techniques. Feel free to ping me, gab@, and/or pmonette@ down the > > road if you'd like to try again. With luck, the jumplist code will soon reach > > steady-state thanks to your efforts. Please let any of us know if you think > that > > not being an OWNER is getting in the way of your ability to make the progress > > you'd like to be making. We'll see what we can do about that. > > You are right. Although I have touched a lot of code in the jumplist files, I > need to absorb more chromium/windows technique and code style to be a good > reviewer. So maybe I'll try later on. > > I had the idea of adding myself to be an owner of the jumplist code simply > because I wanted to be aware of future changes on the jumplist code. Someone > changes my code may not know why I implemented it in that way. Maybe cc'ing me > about the CLs changing the jumplist implementation if there is any in the future > is a better idea. The WATCHLISTS file is handy for being automatically cc'ed on CLs that affect a certain area of the codebase =)
https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWNERS File chrome/browser/win/OWNERS (right): https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWN... chrome/browser/win/OWNERS:12: per-file jumplist*=chengx@chromium.org On 2017/04/24 18:56:05, Ilya Sherman wrote: > On 2017/04/24 17:24:40, chengx wrote: > > On 2017/04/24 09:15:07, grt (UTC plus 2) wrote: > > > While I am very excited about the improvements you've been making in these > > > files, I think you could use some more time absorbing Chromium and Windows > > > styles and techniques. Feel free to ping me, gab@, and/or pmonette@ down the > > > road if you'd like to try again. With luck, the jumplist code will soon > reach > > > steady-state thanks to your efforts. Please let any of us know if you think > > that > > > not being an OWNER is getting in the way of your ability to make the > progress > > > you'd like to be making. We'll see what we can do about that. > > > > You are right. Although I have touched a lot of code in the jumplist files, I > > need to absorb more chromium/windows technique and code style to be a good > > reviewer. So maybe I'll try later on. > > > > I had the idea of adding myself to be an owner of the jumplist code simply > > because I wanted to be aware of future changes on the jumplist code. Someone > > changes my code may not know why I implemented it in that way. Maybe cc'ing me > > about the CLs changing the jumplist implementation if there is any in the > future > > is a better idea. > > The WATCHLISTS file is handy for being automatically cc'ed on CLs that affect a > certain area of the codebase =) That's a good idea! Thanks a lot! I'll defer to grt@ on this.
On 2017/04/24 19:11:39, chengx wrote: > https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWNERS > File chrome/browser/win/OWNERS (right): > > https://codereview.chromium.org/2831433003/diff/100001/chrome/browser/win/OWN... > chrome/browser/win/OWNERS:12: per-file mailto:jumplist*=chengx@chromium.org > On 2017/04/24 18:56:05, Ilya Sherman wrote: > > On 2017/04/24 17:24:40, chengx wrote: > > > On 2017/04/24 09:15:07, grt (UTC plus 2) wrote: > > > > While I am very excited about the improvements you've been making in these > > > > files, I think you could use some more time absorbing Chromium and Windows > > > > styles and techniques. Feel free to ping me, gab@, and/or pmonette@ down > the > > > > road if you'd like to try again. With luck, the jumplist code will soon > > reach > > > > steady-state thanks to your efforts. Please let any of us know if you > think > > > that > > > > not being an OWNER is getting in the way of your ability to make the > > progress > > > > you'd like to be making. We'll see what we can do about that. > > > > > > You are right. Although I have touched a lot of code in the jumplist files, > I > > > need to absorb more chromium/windows technique and code style to be a good > > > reviewer. So maybe I'll try later on. > > > > > > I had the idea of adding myself to be an owner of the jumplist code simply > > > because I wanted to be aware of future changes on the jumplist code. Someone > > > changes my code may not know why I implemented it in that way. Maybe cc'ing > me > > > about the CLs changing the jumplist implementation if there is any in the > > future > > > is a better idea. > > > > The WATCHLISTS file is handy for being automatically cc'ed on CLs that affect > a > > certain area of the codebase =) > > That's a good idea! Thanks a lot! I'll defer to grt@ on this. Being an OWNER doesn't get you CCd on changes automatically. src/WATCHLISTS does, so it's definitely the way to go.
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 Link to the patchset: https://codereview.chromium.org/2831433003/#ps120001 (title: "Remove myself from being an owner and remove Windows version check")
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": 120001, "attempt_start_ts": 1493137018445330,
"parent_rev": "b1e280d1a61f77855418913417c941c7faff176c", "commit_rev":
"250b02089b81116d27bb3732ada0f8e87de02932"}
Message was sent while issue was closed.
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0...
Message was sent while issue was closed.
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time which is set 500 ms now, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ==========
Message was sent while issue was closed.
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes a crazy amount of time which is set 500 ms now, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ==========
Message was sent while issue was closed.
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms which is the cutoff for almost 99% JumpList updates, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ==========
Message was sent while issue was closed.
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms which is the cutoff for almost 99% JumpList updates, as the following steps (delete old icons, create new icons, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms which is the cutoff for almost 99% JumpList updates, as the following steps (old icons' deletion, new icons' creation, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ==========
Message was sent while issue was closed.
Description was changed from ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms which is the cutoff for almost 99% JumpList updates, as the following steps (old icons' deletion, new icons' creation, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ========== to ========== Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms which is the cutoff for 99.5% Canary JumpList updates, as the following steps (old icons' deletion, new icons' creation, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
