Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704443006:
Make time_to_first_suspension's default 1800sec (Closed)
DescriptionMake time_to_first_suspension's default 1800sec
BUG=635419
Review-Url: https://codereview.chromium.org/2704443006
Cr-Commit-Position: refs/heads/master@{#452401}
Committed: https://chromium.googlesource.com/chromium/src/+/1d0df94f561379e580d9f0a1999ea9fc1b52f5ff
Patch Set 1 #
Total comments: 4
Patch Set 2 : Modified unittest to test default value #
Total comments: 6
Patch Set 3 : Fixed. #
Total comments: 6
Patch Set 4 : Fixed. #
Total comments: 2
Patch Set 5 : Fixed style. #
Messages
Total messages: 39 (22 generated)
The CQ bit was checked by tasak@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make time_to_first_suspension's default 1800sec BUG= ========== to ========== Make time_to_first_suspension's default 1800sec BUG=635419 ==========
tasak@google.com changed reviewers: + chrisha@chromium.org, haraken@chromium.org, wez@chromium.org
Would you review this CL?
LGTM
It's a shame we don't have any tests that would have caught this, but I think they'd be tricky to add with the implementation as-is - we should consider moving the scheduling logic out to a helper class that would be easier to unit-test. https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = 1800; Please define this as a constant e.g. kDefaultTimeToFirstSuspendSeconds and define it with a comment explaining why we have chosen this value.
The CQ bit was checked by tasak@google.com 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...
Thank you for review. >It's a shame we don't have any tests that would have caught this, but I think they'd be tricky to add with the implementation as-is - we should consider moving the scheduling logic out to a helper class that would be easier to unit-test. Yeah. I updated TabManagerTest.NextPurgeAndSuspendState to test kDefaultTimeToFirstSuspensionSeconds. https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = 1800; On 2017/02/17 07:39:17, Wez wrote: > Please define this as a constant e.g. kDefaultTimeToFirstSuspendSeconds and > define it with a comment explaining why we have chosen this value. Changed to use kDefaultTimeToFirstSuspensionSeconds, and add a comment. However, I think, there is no strong reason why the default value is 30 minutes...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = 1800; On 2017/02/17 12:36:06, tasak wrote: > On 2017/02/17 07:39:17, Wez wrote: > > Please define this as a constant e.g. kDefaultTimeToFirstSuspendSeconds and > > define it with a comment explaining why we have chosen this value. > > Changed to use kDefaultTimeToFirstSuspensionSeconds, and add a comment. > However, I think, there is no strong reason why the default value is 30 > minutes... There doesn't need to be a _string_ reason, just some rationale at all - e.g. did we choose the value to purge "soon" after a tab becomes idle, but not so soon that simply switching tabs will cause jank? https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:182: // There is no strong reason why its default value is 30 minutes. nit: No need to say "no strong reason"; sufficient to say "this initial value was chosen to <reason>", for example. https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:185: static const int kDefaultTimeToFirstSuspensionSeconds = 1800; nit: I'd suggest using "Suspend" rather than "Suspension" here, so that it's easy for someone doing a dumb search through the code for "Suspend" to find the constant. However, I'd also recommend naming it according to what it actually does, so kDefaultTimeToFirstPurgeInSeconds, for example. https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:185: static const int kDefaultTimeToFirstSuspensionSeconds = 1800; Note that you'll also need to fix this in the Finch configuration.
https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2704443006/diff/1/chrome/browser/memory/tab_m... chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = 1800; On 2017/02/17 19:03:14, Wez wrote: > On 2017/02/17 12:36:06, tasak wrote: > > On 2017/02/17 07:39:17, Wez wrote: > > > Please define this as a constant e.g. kDefaultTimeToFirstSuspendSeconds and > > > define it with a comment explaining why we have chosen this value. > > > > Changed to use kDefaultTimeToFirstSuspensionSeconds, and add a comment. > > However, I think, there is no strong reason why the default value is 30 > > minutes... > > There doesn't need to be a _string_ reason, just some rationale at all - e.g. > did we choose the value to purge "soon" after a tab becomes idle, but not so > soon that simply switching tabs will cause jank? s/string/strong ;)
The CQ bit was checked by tasak@google.com 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 found Tabs.StateTransfer.Time_Inactive_Active. Looking at the data, 94% of users activate inactive tabs within 30 minutes. https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:182: // There is no strong reason why its default value is 30 minutes. On 2017/02/17 19:03:14, Wez wrote: > nit: No need to say "no strong reason"; sufficient to say "this initial value > was chosen to <reason>", for example. Done. https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:185: static const int kDefaultTimeToFirstSuspensionSeconds = 1800; On 2017/02/17 19:03:14, Wez wrote: > Note that you'll also need to fix this in the Finch configuration. Acknowledged. https://codereview.chromium.org/2704443006/diff/20001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:185: static const int kDefaultTimeToFirstSuspensionSeconds = 1800; On 2017/02/17 19:03:14, Wez wrote: > nit: I'd suggest using "Suspend" rather than "Suspension" here, so that it's > easy for someone doing a dumb search through the code for "Suspend" to find the > constant. However, I'd also recommend naming it according to what it actually > does, so kDefaultTimeToFirstPurgeInSeconds, for example. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wez@: Would you review this CL? We want to land this and collect numbers :)
On 2017/02/22 00:32:07, haraken wrote: > wez@: Would you review this CL? We want to land this and collect numbers :) Happy to!
LGTM w/ a couple of suggestions. :) https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:184: static const int kDefaultTimeToFirstPurgeInSeconds = 1800; nit: base::TimeDelta now has constexpr FromSeconds() helpers - could this be: constexpr base::TimeDelta kDefaultTimeToFirstPurge = base::TimeDelta::FromMinutes(30); instead? Otherwise, let's at least write it as 30 * 60 for clarity. https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_unittest.cc:694: // Wait 30 minutes. Should be still RUNNING. nit: Comment style is to prefer complete sentences, e.g. "Wait 30 minutes and verify that the tab is still RUNNING." and then "Wait another second and verify that it is now SUSPENDED." https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_unittest.cc:700: // > 30 minutes. Should be still SUSPENDED. Do you mean "Should _now_ be SUSPENDED."?
Thank you for review. https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.h (right): https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.h:184: static const int kDefaultTimeToFirstPurgeInSeconds = 1800; On 2017/02/22 01:25:10, Wez wrote: > nit: base::TimeDelta now has constexpr FromSeconds() helpers - could this be: > > constexpr base::TimeDelta kDefaultTimeToFirstPurge = > base::TimeDelta::FromMinutes(30); > > instead? > > Otherwise, let's at least write it as 30 * 60 for clarity. Done. https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager_unittest.cc (right): https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_unittest.cc:694: // Wait 30 minutes. Should be still RUNNING. On 2017/02/22 01:25:10, Wez wrote: > nit: Comment style is to prefer complete sentences, e.g. "Wait 30 minutes and > verify that the tab is still RUNNING." and then "Wait another second and verify > that it is now SUSPENDED." Done. https://codereview.chromium.org/2704443006/diff/40001/chrome/browser/memory/t... chrome/browser/memory/tab_manager_unittest.cc:700: // > 30 minutes. Should be still SUSPENDED. On 2017/02/22 01:25:10, Wez wrote: > Do you mean "Should _now_ be SUSPENDED."? Yes, thanks. Done.
chrisha@, would you review this CL?
https://codereview.chromium.org/2704443006/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2704443006/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:231: unsigned time_to_first_purge_sec; nit: Even though this is about to be set by StringToUint(), it should be initialised per style-guide. It should also be "unsigned int", not just "unsigned".
lgtm beyond Wez's nits.
The CQ bit was checked by tasak@google.com 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...
Thank you for review. https://codereview.chromium.org/2704443006/diff/60001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2704443006/diff/60001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:231: unsigned time_to_first_purge_sec; On 2017/02/22 17:57:55, Wez wrote: > nit: Even though this is about to be set by StringToUint(), it should be > initialised per style-guide. > > It should also be "unsigned int", not just "unsigned". 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 tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, wez@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2704443006/#ps80001 (title: "Fixed style.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487826389673710,
"parent_rev": "0846083cfdbea5d902953540a4220599eb0fe9e1", "commit_rev":
"1d0df94f561379e580d9f0a1999ea9fc1b52f5ff"}
Message was sent while issue was closed.
Description was changed from ========== Make time_to_first_suspension's default 1800sec BUG=635419 ========== to ========== Make time_to_first_suspension's default 1800sec BUG=635419 Review-Url: https://codereview.chromium.org/2704443006 Cr-Commit-Position: refs/heads/master@{#452401} Committed: https://chromium.googlesource.com/chromium/src/+/1d0df94f561379e580d9f0a1999e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1d0df94f561379e580d9f0a1999e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
