Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2483003004:
Make purge-and-suspend-time finch experiment parameter (Closed)
DescriptionMake purge-and-suspend-time finch experiment parameter.
- Replace command_line.HasSwitch with variations::GetVariationParamValue.
- Since Purge+Suspend is now controlled by
base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't
enable Purge+Suspend by default.
- intent-to-implement-and-ship of background renderer's purge+suspend is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
BUG=607077
Committed: https://crrev.com/b6edd99c51d17c7a8f058f84acefcc7b935dd893
Cr-Commit-Position: refs/heads/master@{#435858}
Patch Set 1 #Patch Set 2 : Use GetVariationParamValue #Patch Set 3 : Rebaselined #
Total comments: 2
Patch Set 4 : Move GetVariationParamValue to initialization phase #
Total comments: 9
Patch Set 5 : Add comment about purge-and-suspend-time #Patch Set 6 : Make purge-and-suspend-time=0 immediately purge+suspend. Instead default value is 30min. #
Total comments: 1
Messages
Total messages: 65 (38 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...
tasak@google.com changed reviewers: + chrisha@chromium.org, haraken@chromium.org
Would you review this CL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
LGTM
Do we want to make the purge and suspend time a Finch parameter? That way we can explore the impact of different values across the user population in a single experiment.
On 2016/11/09 21:41:34, chrisha (slow) wrote: > Do we want to make the purge and suspend time a Finch parameter? That way we can > explore the impact of different values across the user population in a single > experiment. I would like to find the best purge-and-suspend-time... However, currently I have no idea about how to measure purge-and-suspend-time effect. So I'm planning a single experiment.
Configuring it via Finch parameter means we can change the value of the parameter remotely at any time, and not require a software change. For now it would default to 30 minutes in the absence of any parameter value, but that would set us up for experiments in the future. On Thu, Nov 10, 2016, 00:35 <tasak@google.com> wrote: > On 2016/11/09 21:41:34, chrisha (slow) wrote: > > Do we want to make the purge and suspend time a Finch parameter? That > way we > can > > explore the impact of different values across the user population in a > single > > experiment. > > I would like to find the best purge-and-suspend-time... > However, currently I have no idea about how to measure > purge-and-suspend-time > effect. > So I'm planning a single experiment. > > > https://codereview.chromium.org/2483003004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/10 12:53:12, chrisha (slow) wrote: > Configuring it via Finch parameter means we can change the value of the > parameter remotely at any time, and not require a software change. For now > it would default to 30 minutes in the absence of any parameter value, but > that would set us up for experiments in the future. Yeah, firstly I was thinking about this. However, I have one concern: is it ok to see Finch parameter when the feature is disabled? The 30minutes value is used when the feature is enabled and also disabled, because of A/B testing.
On 2016/11/10 12:53:12, chrisha (slow) wrote: > Configuring it via Finch parameter means we can change the value of the > parameter remotely at any time, and not require a software change. For now > it would default to 30 minutes in the absence of any parameter value, but > that would set us up for experiments in the future. Yeah, firstly I was thinking about this. However, I have one concern: is it ok to see Finch parameter when the feature is disabled? The 30minutes value is used when the feature is enabled and also disabled, because of A/B testing.
On 2016/11/16 19:07:22, tasak wrote:
> On 2016/11/10 12:53:12, chrisha (slow) wrote:
> > Configuring it via Finch parameter means we can change the value of the
> > parameter remotely at any time, and not require a software change. For now
> > it would default to 30 minutes in the absence of any parameter value, but
> > that would set us up for experiments in the future.
>
> Yeah, firstly I was thinking about this. However, I have one concern:
> is it ok to see Finch parameter when the feature is disabled?
>
> The 30minutes value is used when the feature is enabled and also disabled,
> because of A/B testing.
I see.
Experiment: 50%, enabled_features: { "PurgeAndSuspend" }, params: {
"purge-and-suspend-time": "30min" }
Control: 50%, disabled_features: { "PurgeAndSuspend" }, params: {
"purge-and-suspend-time": "30min" }
Default: 0%
So if it is ok to use 0% default, I have to replace command_line.HasSwitch with
variations::GetVariationParamValue(kPurgeAndSuspend, "purge-and-suspend-time")?
You can write the code to grab the parameter even if no experiment is active or defined. If the experiment group isn't found, or the parameter value isn't defined you can then use a hard-coded default. You can see some example code here: https://codereview.chromium.org/1130673003/diff/60001/chrome/browser/sessions... On Wed, 16 Nov 2016 at 11:34 <tasak@google.com> wrote: > On 2016/11/16 19:07:22, tasak wrote: > > On 2016/11/10 12:53:12, chrisha (slow) wrote: > > > Configuring it via Finch parameter means we can change the value of the > > > parameter remotely at any time, and not require a software change. For > now > > > it would default to 30 minutes in the absence of any parameter value, > but > > > that would set us up for experiments in the future. > > > > Yeah, firstly I was thinking about this. However, I have one concern: > > is it ok to see Finch parameter when the feature is disabled? > > > > The 30minutes value is used when the feature is enabled and also > disabled, > > because of A/B testing. > > I see. > Experiment: 50%, enabled_features: { "PurgeAndSuspend" }, params: { > "purge-and-suspend-time": "30min" } > Control: 50%, disabled_features: { "PurgeAndSuspend" }, params: { > "purge-and-suspend-time": "30min" } > Default: 0% > > So if it is ok to use 0% default, I have to replace command_line.HasSwitch > with > variations::GetVariationParamValue(kPurgeAndSuspend, > "purge-and-suspend-time")? > > > https://codereview.chromium.org/2483003004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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...
Description was changed from ========== Replace purge-and-suspend-time with 30min. - Replace purge-and-suspend-time with 30min, because the code makes it difficult to do finch-experiment. If we keep this code, we need to specify enable-features=PurgeAndSuspend and also purge-and-suspend-time in finch experiment's config. - Since Purge+Suspend is now controlled by base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't enable Purge+Suspend by default. - intent-to-implement-and-ship of background renderer's purge+suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=607077 ========== to ========== Make purge-and-suspend-time finch experiment parameter. - Replace command_line.HasSwitch with variations::GetVariationParamValue. - Since Purge+Suspend is now controlled by base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't enable Purge+Suspend by default. - intent-to-implement-and-ship of background renderer's purge+suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=607077 ==========
The CQ bit was checked by tasak@google.com to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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 tasak@google.com to run a CQ dry run
Patchset #2 (id:40001) has been deleted
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.
Thank you. I replaced command_line.hasSwitch with GetVariationParamValue. On 2016/11/17 23:17:21, chrisha (slow) wrote: > You can write the code to grab the parameter even if no experiment is > active or defined. If the experiment group isn't found, or the parameter > value isn't defined you can then use a hard-coded default. You can see some > example code here: > > https://codereview.chromium.org/1130673003/diff/60001/chrome/browser/sessions... > > On Wed, 16 Nov 2016 at 11:34 <mailto:tasak@google.com> wrote: > > > On 2016/11/16 19:07:22, tasak wrote: > > > On 2016/11/10 12:53:12, chrisha (slow) wrote: > > > > Configuring it via Finch parameter means we can change the value of the > > > > parameter remotely at any time, and not require a software change. For > > now > > > > it would default to 30 minutes in the absence of any parameter value, > > but > > > > that would set us up for experiments in the future. > > > > > > Yeah, firstly I was thinking about this. However, I have one concern: > > > is it ok to see Finch parameter when the feature is disabled? > > > > > > The 30minutes value is used when the feature is enabled and also > > disabled, > > > because of A/B testing. > > > > I see. > > Experiment: 50%, enabled_features: { "PurgeAndSuspend" }, params: { > > "purge-and-suspend-time": "30min" } > > Control: 50%, disabled_features: { "PurgeAndSuspend" }, params: { > > "purge-and-suspend-time": "30min" } > > Default: 0% > > > > So if it is ok to use 0% default, I have to replace command_line.HasSwitch > > with > > variations::GetVariationParamValue(kPurgeAndSuspend, > > "purge-and-suspend-time")? > > > > > > https://codereview.chromium.org/2483003004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
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.
chrisha, would you review the newest patch?
https://codereview.chromium.org/2483003004/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:734: std::string purge_and_suspend_time = variations::GetVariationParamValue( Maybe do this once in some kind of initialization phase, and save the value to the object? Rather than doing this on every call to PurgeAndSuspendBackgroundedTabs?
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 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. I updated the fixed CL. https://codereview.chromium.org/2483003004/diff/80001/chrome/browser/memory/t... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/80001/chrome/browser/memory/t... chrome/browser/memory/tab_manager.cc:734: std::string purge_and_suspend_time = variations::GetVariationParamValue( On 2016/11/23 19:38:09, chrisha (slow) wrote: > Maybe do this once in some kind of initialization phase, and save the value to > the object? Rather than doing this on every call to > PurgeAndSuspendBackgroundedTabs? I see. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:224: std::string purge_and_suspend_time = variations::GetVariationParamValue( Should we have a default time that this is initialized to in the absence of a variation param? https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:745: if (time_to_first_suspension_.InSeconds() <= 0) Do we need this check? This function simply shouldn't be called if PurgeAndSuspend isn't enabled.
(Let's aim at addressing all review comments in the next cycle!)
https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:224: std::string purge_and_suspend_time = variations::GetVariationParamValue( On 2016/11/24 17:13:09, chrisha (slow) wrote: > Should we have a default time that this is initialized to in the absence of a > variation param? As far as I understand, base::TimeDelta() initializes delta_ as 0. If purge-and-suspend-time is 0, purge-and-suspend is disabled (this is the same behavior as the original purge-and-suspend-time). Looking at minimum_protection_time_, it also depends on base::TimeDelta()'s behavior (initializing delta_ as 0). So do we need to explicitly initialize time_to_first_suspension_? https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:745: if (time_to_first_suspension_.InSeconds() <= 0) On 2016/11/24 17:13:09, chrisha (slow) wrote: > Do we need this check? This function simply shouldn't be called if > PurgeAndSuspend isn't enabled. I would like to confirm. You mean, we should check base::FeatureList::IsEnabled(kPurgeAndSuspend) before calling this function? If so, I would like to explain again. I would like to do A/B testing. A is "recording UMA when purge+suspend is really executed." and B is "recording UMA at almost the same time when purge+suspend is executed but we don't purge+suspend." (So B is "recording UMA when about purge-and-suspend time passes after a renderer is backgrounded.) If we don't call this function when !base::FeatureList::IsEnabled(kPurgeAndSuspend), it is not possible to record B. I'm not sure whether it is ok to always record B or not when we don't start any finch experiment. So currently B's code (i.e. RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when purge-and-suspend-time=0 (and 0 is default value).
https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:745: if (time_to_first_suspension_.InSeconds() <= 0) On 2016/11/25 01:15:24, tasak wrote: > I'm not sure whether it is ok to always record B or not when we don't start any > finch experiment. So currently B's code (i.e. > RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when > purge-and-suspend-time=0 (and 0 is default value). So my first patch is trying to always record B. I thought, if someone wants not to change chrome behavior when !IsEnabled(kPurgeAndSuspend), I will keep purge-and-suspend-time.
Friendly ping.
lgtm, modulo some more comments please https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:224: std::string purge_and_suspend_time = variations::GetVariationParamValue( On 2016/11/25 01:15:24, tasak wrote: > On 2016/11/24 17:13:09, chrisha (slow) wrote: > > Should we have a default time that this is initialized to in the absence of a > > variation param? > > As far as I understand, base::TimeDelta() initializes delta_ as 0. > If purge-and-suspend-time is 0, purge-and-suspend is disabled (this is the same > behavior as the original purge-and-suspend-time). > > Looking at minimum_protection_time_, it also depends on base::TimeDelta()'s > behavior (initializing delta_ as 0). > So do we need to explicitly initialize time_to_first_suspension_? Okay, that makes sense. I was thinking that there'd be a different on/off switch for feature itself, and that only a non-zero purge-and-suspend delay would be configurable via this variation. In which case having a non-zero default value when the variation is not present would make sense. This is fine, but I'd at least cleanly document that a zero purge and suspend time indicates that the feature is disabled? https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:745: if (time_to_first_suspension_.InSeconds() <= 0) On 2016/11/25 01:19:44, tasak wrote: > On 2016/11/25 01:15:24, tasak wrote: > > > I'm not sure whether it is ok to always record B or not when we don't start > any > > finch experiment. So currently B's code (i.e. > > RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when > > purge-and-suspend-time=0 (and 0 is default value). > > So my first patch is trying to always record B. > I thought, if someone wants not to change chrome behavior when > !IsEnabled(kPurgeAndSuspend), I will keep purge-and-suspend-time. I'm not sure I follow this entire chain of logic? This function is only ever called from UpdateTimerCallback. If time_to_first_suspension_ is <= 0, then it immediately returns, without recording anything. Where are these UMAs you speak of? Will you be adding them to this function? Regardless, I'm fine with leaving the code as is but more comments in the code would be very useful here. Something as simple as: // A zero-valued |time_to_first_suspension_| means the feature is disabled, so return immediately.
Thank you for review. On 2016/11/29 14:13:58, chrisha (slow) wrote: > lgtm, modulo some more comments please > > https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... > File chrome/browser/memory/tab_manager.cc (right): > > https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... > chrome/browser/memory/tab_manager.cc:224: std::string purge_and_suspend_time = > variations::GetVariationParamValue( > On 2016/11/25 01:15:24, tasak wrote: > > On 2016/11/24 17:13:09, chrisha (slow) wrote: > > > Should we have a default time that this is initialized to in the absence of > a > > > variation param? > > > > As far as I understand, base::TimeDelta() initializes delta_ as 0. > > If purge-and-suspend-time is 0, purge-and-suspend is disabled (this is the > same > > behavior as the original purge-and-suspend-time). > > > > Looking at minimum_protection_time_, it also depends on base::TimeDelta()'s > > behavior (initializing delta_ as 0). > > So do we need to explicitly initialize time_to_first_suspension_? > > Okay, that makes sense. I was thinking that there'd be a different on/off switch > for feature itself, and that only a non-zero purge-and-suspend delay would be > configurable via this variation. In which case having a non-zero default value > when the variation is not present would make sense. > > This is fine, but I'd at least cleanly document that a zero purge and suspend > time indicates that the feature is disabled? > > https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... > chrome/browser/memory/tab_manager.cc:745: if > (time_to_first_suspension_.InSeconds() <= 0) > On 2016/11/25 01:19:44, tasak wrote: > > On 2016/11/25 01:15:24, tasak wrote: > > > > > I'm not sure whether it is ok to always record B or not when we don't start > > any > > > finch experiment. So currently B's code (i.e. > > > RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when > > > purge-and-suspend-time=0 (and 0 is default value). > > > > So my first patch is trying to always record B. > > I thought, if someone wants not to change chrome behavior when > > !IsEnabled(kPurgeAndSuspend), I will keep purge-and-suspend-time. > > I'm not sure I follow this entire chain of logic? This function is only ever > called from UpdateTimerCallback. If time_to_first_suspension_ is <= 0, then it > immediately returns, without recording anything. > > Where are these UMAs you speak of? Will you be adding them to this function? Yeah. This is what I was saying when I created the first patch. So I was trying to replace the code with just 30min wait. The problem is that I'm trying to do A/B testing in the following way: https://docs.google.com/document/d/1hPHkKtXXBTlsZx9s-9U17XC-ofEIzPo9FYbBEc7PP... So to record UMA under the (almost) same condition, I need to record the UMA in render_thread_impl.cc. > Regardless, I'm fine with leaving the code as is but more comments in the code > would be very useful here. Something as simple as: > > // A zero-valued |time_to_first_suspension_| means the feature is disabled, so > return immediately.
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...
https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:224: std::string purge_and_suspend_time = variations::GetVariationParamValue( On 2016/11/29 14:13:57, chrisha (slow) wrote: > On 2016/11/25 01:15:24, tasak wrote: > > On 2016/11/24 17:13:09, chrisha (slow) wrote: > > > Should we have a default time that this is initialized to in the absence of > a > > > variation param? > > > > As far as I understand, base::TimeDelta() initializes delta_ as 0. > > If purge-and-suspend-time is 0, purge-and-suspend is disabled (this is the > same > > behavior as the original purge-and-suspend-time). > > > > Looking at minimum_protection_time_, it also depends on base::TimeDelta()'s > > behavior (initializing delta_ as 0). > > So do we need to explicitly initialize time_to_first_suspension_? > > Okay, that makes sense. I was thinking that there'd be a different on/off switch > for feature itself, and that only a non-zero purge-and-suspend delay would be > configurable via this variation. In which case having a non-zero default value > when the variation is not present would make sense. > > This is fine, but I'd at least cleanly document that a zero purge and suspend > time indicates that the feature is disabled? Yes... currently the zero means that the purge+suspend feature is disabled. https://codereview.chromium.org/2483003004/diff/100001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:745: if (time_to_first_suspension_.InSeconds() <= 0) On 2016/11/29 14:13:57, chrisha (slow) wrote: > On 2016/11/25 01:19:44, tasak wrote: > > On 2016/11/25 01:15:24, tasak wrote: > > > > > I'm not sure whether it is ok to always record B or not when we don't start > > any > > > finch experiment. So currently B's code (i.e. > > > RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when > > > purge-and-suspend-time=0 (and 0 is default value). > > > > So my first patch is trying to always record B. > > I thought, if someone wants not to change chrome behavior when > > !IsEnabled(kPurgeAndSuspend), I will keep purge-and-suspend-time. > > I'm not sure I follow this entire chain of logic? This function is only ever > called from UpdateTimerCallback. If time_to_first_suspension_ is <= 0, then it > immediately returns, without recording anything. > > Where are these UMAs you speak of? Will you be adding them to this function? > > Regardless, I'm fine with leaving the code as is but more comments in the code > would be very useful here. Something as simple as: > > // A zero-valued |time_to_first_suspension_| means the feature is disabled, so > return immediately. I see.... I'm trying to keep the existing behavior... But now we have purge+suspend feature. We don't need to control whether purge+suspend is enabled or not by purge-and-suspend-time. I also added the code to make default purge-and-suspend-time 30min. Done.
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.
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, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2483003004/#ps140001 (title: "Make purge-and-suspend-time=0 immediately purge+suspend. Instead default value is 30min.")
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": 140001, "attempt_start_ts": 1480653437870880,
"parent_rev": "e1144af45b8cef6eb42a945243da504a9b303e90", "commit_rev":
"3098ce6cd53d8e79cbfc51c9e8cd863c12618039"}
Message was sent while issue was closed.
Description was changed from ========== Make purge-and-suspend-time finch experiment parameter. - Replace command_line.HasSwitch with variations::GetVariationParamValue. - Since Purge+Suspend is now controlled by base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't enable Purge+Suspend by default. - intent-to-implement-and-ship of background renderer's purge+suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=607077 ========== to ========== Make purge-and-suspend-time finch experiment parameter. - Replace command_line.HasSwitch with variations::GetVariationParamValue. - Since Purge+Suspend is now controlled by base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't enable Purge+Suspend by default. - intent-to-implement-and-ship of background renderer's purge+suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=607077 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Make purge-and-suspend-time finch experiment parameter. - Replace command_line.HasSwitch with variations::GetVariationParamValue. - Since Purge+Suspend is now controlled by base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't enable Purge+Suspend by default. - intent-to-implement-and-ship of background renderer's purge+suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=607077 ========== to ========== Make purge-and-suspend-time finch experiment parameter. - Replace command_line.HasSwitch with variations::GetVariationParamValue. - Since Purge+Suspend is now controlled by base::Feature::kPurgeAndSuspend (c.f. @430225), this change doesn't enable Purge+Suspend by default. - intent-to-implement-and-ship of background renderer's purge+suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=607077 Committed: https://crrev.com/b6edd99c51d17c7a8f058f84acefcc7b935dd893 Cr-Commit-Position: refs/heads/master@{#435858} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b6edd99c51d17c7a8f058f84acefcc7b935dd893 Cr-Commit-Position: refs/heads/master@{#435858}
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2483003004/diff/140001/chrome/browser/memory/... File chrome/browser/memory/tab_manager.cc (right): https://codereview.chromium.org/2483003004/diff/140001/chrome/browser/memory/... chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = 108000; There doesn't seem to be any explanation here, or in the linked document, of why 30 hours is a good time-to-first-suspension?
Message was sent while issue was closed.
On 2017/02/15 22:22:52, Wez wrote: > https://codereview.chromium.org/2483003004/diff/140001/chrome/browser/memory/... > File chrome/browser/memory/tab_manager.cc (right): > > https://codereview.chromium.org/2483003004/diff/140001/chrome/browser/memory/... > chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = 108000; > There doesn't seem to be any explanation here, or in the linked document, of why > 30 hours is a good time-to-first-suspension? I have no idea about how long we should wait until we start purging. So I have no strong reason why time-to-first-suspension is 30minutes. (Talking about my use-case, it's ok to suspend all backgrounded renderers and to start purging 5 minutes after the renderers are backgrounded.) I heard that many people want to open each tab (from leftmost to rightmost?) to find some content. So I decided to make time-to-first-suspension some large number.
Message was sent while issue was closed.
Takashi, this constant isn't 30 *minutes*, it is 30 *hours*, which seems way too long to be useful to many users. On 16 February 2017 at 02:50, <tasak@google.com> wrote: > On 2017/02/15 22:22:52, Wez wrote: > > > https://codereview.chromium.org/2483003004/diff/140001/ > chrome/browser/memory/tab_manager.cc > > File chrome/browser/memory/tab_manager.cc (right): > > > > > https://codereview.chromium.org/2483003004/diff/140001/ > chrome/browser/memory/tab_manager.cc#newcode233 > > chrome/browser/memory/tab_manager.cc:233: time_to_first_suspension_sec = > 108000; > > There doesn't seem to be any explanation here, or in the linked > document, of > why > > 30 hours is a good time-to-first-suspension? > > I have no idea about how long we should wait until we start purging. > So I have no strong reason why time-to-first-suspension is 30minutes. > (Talking about my use-case, it's ok to suspend all backgrounded renderers > and to > start purging 5 minutes after the renderers are backgrounded.) > > I heard that many people want to open each tab (from leftmost to > rightmost?) to > find some content. > So I decided to make time-to-first-suspension some large number. > > > https://codereview.chromium.org/2483003004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
