Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2868303003:
Cancel the next 10 updates if there's a timeout in jumplist updater (Closed)
DescriptionCancel the next 10 updates if there's a timeout in jumplist updater
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 users, especially for steps
Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly
Windows API calls, we can assume this is due to user machines (which
can be very slow) with a pretty high confidence, and also there's not
much we can improve on Chrome's side.
Previously, we cancel the current jumplist update if BeginUpdate takes
more than 500 ms, based on the assumption that the following steps
will also take a long time if BeginUpdate does. The UMA data shows
that this benefits some users but not the majority, which indicates
that the runtime of these steps is not highly correlated. That is,
AddCustomCategory and/or CommitUpdate can still take a long time even
if BeginUpdate finishes quickly. To alleviate this issue, we should
reduce the frequency of jumplist updates if any of those steps times
out. In more details, we cancel the next 10 updates if BeginUpdate or
AddCustomCategory takes more than 500 ms, or takes more than 500 ms,
or CommitUpdate takes more than 1000 ms, which are the cutoffs for
99.5% JumpList update samples in Canary for the three steps,
respectively. If the machine is always slow making every jumplist
update time out, this penalty of cancelling the next 10 updates can
alleviate this issue by 90%.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2868303003
Cr-Commit-Position: refs/heads/master@{#471430}
Committed: https://chromium.googlesource.com/chromium/src/+/143d0a7278a1f2b6ff9173ec78e6755b7100704c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change int/bool logic to int logic only #
Total comments: 6
Patch Set 3 : Update comments and variable names #
Messages
Total messages: 42 (32 generated)
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 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 ========== Implement cancelling 10 updates if timeout BUG= ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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, especially for the steps of Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should reduce the frequency of jumplist updates. Code-wise, we cancel the next 10 updates if BeginUpdate takes more than 500 ms, or AddCustomCategory takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ==========
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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, especially for the steps of Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should reduce the frequency of jumplist updates. Code-wise, we cancel the next 10 updates if BeginUpdate takes more than 500 ms, or AddCustomCategory takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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, especially for the steps of Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For this case, we should reduce the frequency of jumplist updates. Code- wise, we cancel the next 10 updates if BeginUpdate takes more than 500 ms, or AddCustomCategory takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + gab@chromium.org
Hi Gabriel, PTAL this small change. Thanks! I've landed a (slightly) similar one as in crrev.com/2831433003/.
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...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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, especially for the steps of Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For this case, we should reduce the frequency of jumplist updates. Code- wise, we cancel the next 10 updates if BeginUpdate takes more than 500 ms, or AddCustomCategory takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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, especially for the steps of Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For this case, we should reduce the frequency of jumplist updates. Code-wise, we cancel the next 10 updates if BeginUpdate takes more than 500 ms, or AddCustomCategory takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ==========
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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, especially for the steps of Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For this case, we should reduce the frequency of jumplist updates. Code-wise, we cancel the next 10 updates if BeginUpdate takes more than 500 ms, or AddCustomCategory takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. In this case, we should reduce the frequency of jumplist updates. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ==========
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. In this case, we should reduce the frequency of jumplist updates. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. In this case, we should reduce the frequency of jumplist updates. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:69: constexpr int kCancelledUpdates = 10; Feels like we should delay for a period of time, e.g. a few minutes, instead of a count? i.e. simply use a larger timeout for batch updates?
Hi Gabriel, the see my reply to your comment below. Thanks! https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:69: constexpr int kCancelledUpdates = 10; On 2017/05/11 14:48:24, gab wrote: > Feels like we should delay for a period of time, e.g. a few minutes, instead of > a count? i.e. simply use a larger timeout for batch updates? I did think about delaying for a period of time. But eventually I still prefer using a count. As we need to reduce the update frequency of the Jumplist for this small set of Chrome users, using a count makes this frequency reduction certain and the same for all the users in that set independent of time. Btw, we already have a "delay and coalesce" strategy where all tabs closed within 3.5 seconds are collapsed into 1 jumplist update. Maybe I can land this CL to see how it performs, and try another CL with your idea later on.
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. In this case, we should reduce the frequency of jumplist updates. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% Canary JumpList update samples for each of the three steps, respectively. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps are not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for each of the three steps, respectively. If the machine is always slow making the jumplist update always time out, this penalty of cancelling the next 10 updates can alleviate this issue to its 1/10. BUG=40407, 179576 ==========
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps are not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for each of the three steps, respectively. If the machine is always slow making the jumplist update always time out, this penalty of cancelling the next 10 updates can alleviate this issue to its 1/10. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue to its 1/10. BUG=40407, 179576 ==========
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue to its 1/10. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue by 90%. BUG=40407, 179576 ==========
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue by 90%. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue by 90%. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
Adding grt@ for more feedback. Hi Gabriel, I've updated the CL description to better convey my thoughts. Thanks!
Ok, let's try it if you think it will help https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:243: int cancelled_update_count_; Initialize member inline here instead of in constructor initializer list (same for bool) https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:248: bool jumplist_updater_timeout_; Actually you don't need the bool, you can do this with just one int variable. // Number of updates to skip to alleviate the machine when a previous update was too slow. Updates will be resumed when this reaches 0 again. int updates_to_skip_ = 0; I think this will simplify the logic as well.
Patchset #2 (id:60001) has been deleted
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...
I've simplified the int/bool logic to int-only logic as suggested. PTAL, thanks! I've also tested it locally which runs fine. https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:243: int cancelled_update_count_; On 2017/05/12 17:58:47, gab wrote: > Initialize member inline here instead of in constructor initializer list (same > for bool) Done. https://codereview.chromium.org/2868303003/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:248: bool jumplist_updater_timeout_; On 2017/05/12 17:58:47, gab wrote: > Actually you don't need the bool, you can do this with just one int variable. > > // Number of updates to skip to alleviate the machine when a previous update was > too slow. Updates will be resumed when this reaches 0 again. > int updates_to_skip_ = 0; > > I think this will simplify the logic as well. Agreed. I've updated the logic in the new patch set.
lgtm w/ comments https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:69: constexpr int kMaxUpdatesToSkip = 10; kUpdatesToSkipUnderHeavyLoad (i.e. this isn't a "max", it *is* the number of updates you'll skip) https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:733: // time, mark the flag but continue with this update. Explain why you continue with the update in this case Also, it's no longer a "flag" https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:747: // time, mark the flag but continue with this update. ditto
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...
Thanks for the timely review! I've addressed all the comments in the new patch set. https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:69: constexpr int kMaxUpdatesToSkip = 10; On 2017/05/12 18:59:59, gab wrote: > kUpdatesToSkipUnderHeavyLoad > > (i.e. this isn't a "max", it *is* the number of updates you'll skip) Agreed. Done. https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:733: // time, mark the flag but continue with this update. On 2017/05/12 19:00:00, gab wrote: > Explain why you continue with the update in this case > > Also, it's no longer a "flag" I've updated the comments to explain why this update should be continued in particular. Thanks! https://codereview.chromium.org/2868303003/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:747: // time, mark the flag but continue with this update. On 2017/05/12 19:00:00, gab wrote: > ditto Done.
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 patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org Link to the patchset: https://codereview.chromium.org/2868303003/#ps100001 (title: "Update comments and variable names")
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": 100001, "attempt_start_ts": 1494620449661790,
"parent_rev": "fedd10504893bddc12db34e26e8749f8a87a3ec0", "commit_rev":
"143d0a7278a1f2b6ff9173ec78e6755b7100704c"}
Message was sent while issue was closed.
Description was changed from ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue by 90%. BUG=40407, 179576 ========== to ========== Cancel the next 10 updates if there's a timeout in jumplist updater 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 users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue by 90%. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2868303003 Cr-Commit-Position: refs/heads/master@{#471430} Committed: https://chromium.googlesource.com/chromium/src/+/143d0a7278a1f2b6ff9173ec78e6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/143d0a7278a1f2b6ff9173ec78e6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
