Add a finch experiment for different memory pressure management strategies in ChromeOS.

Add a finch experiment for different memory pressure management strategies in ChromeOS.

BUG=455885, 447280
Committed: https://crrev.com/f735c7a31f0c45b71545d06034c220eb976909cd
Cr-Commit-Position: refs/heads/master@{#315907}

[xdai1, 2015-02-06 01:08:19]

Patchset #1 (id:1) has been deleted

[xdai1, 2015-02-06 01:12:41]

xdai@chromium.org changed reviewers:
+ skuhne@chromium.org

[xdai1, 2015-02-06 01:12:42]

Please help to review the CL, thanks!

https://codereview.chromium.org/897413002/diff/20001/base/DEPS
File base/DEPS (right):

https://codereview.chromium.org/897413002/diff/20001/base/DEPS#newcode2
base/DEPS:2: "+chromeos",
I feel bad about adding this include dependency. Maybe we can do something
similar with chrome_switches.h/cc files, to put chromeos_switches.h/cc into a
sub-directory like src/chromeos/common?

[Mr4D (OOO till 08-26), 2015-02-06 03:56:35]

See comments!

https://codereview.chromium.org/897413002/diff/20001/base/DEPS
File base/DEPS (right):

https://codereview.chromium.org/897413002/diff/20001/base/DEPS#newcode2
base/DEPS:2: "+chromeos",
This is not an option.

https://codereview.chromium.org/897413002/diff/20001/base/chromeos/memory_pre...
File base/chromeos/memory_pressure_observer_chromeos.cc (right):

https://codereview.chromium.org/897413002/diff/20001/base/chromeos/memory_pre...
base/chromeos/memory_pressure_observer_chromeos.cc:216:
MemoryPressureObserverChromeOS::GetMemoryPressureThresholds() {
This should remain in chromeos_switches.cc. Having this in base is not an
option.

https://codereview.chromium.org/897413002/diff/20001/base/chromeos/memory_pre...
File base/chromeos/memory_pressure_observer_chromeos.h (right):

https://codereview.chromium.org/897413002/diff/20001/base/chromeos/memory_pre...
base/chromeos/memory_pressure_observer_chromeos.h:54: // FieldTrial.
Not here.

https://codereview.chromium.org/897413002/diff/20001/content/browser/browser_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/897413002/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:494:
base::MemoryPressureObserverChromeOS::GetMemoryPressureThresholds()));
Call the previous function to get the threshold parameters.
Add another function to check if we want this at all and replace 491-492 with
that code (which will then also look at the fieldtrial flags.

[xdai1, 2015-02-06 18:46:02]

Patchset #2 (id:40001) has been deleted

[xdai1, 2015-02-06 19:40:30]

Please take another look at the code, thanks!

It seems to me that we have two switches here:
kDisableMemoryPressureSystemChromeOS and kMemoryPressureThresholds. The former
one decides if we want to turn on/off enhanced memory pressure handling (default
is on), and the latter one decides which threshold we use if we turn the memory
mgmt on (default is THRESHOLD_DEFAULT). 

It makes me confused since in that case, when the memory mgmt enabled, from the
logic of the code, THRESHOLD_DEFAULT is same with THRESHOLD_CONSERVATIVE, which
doesn't make sense. It also makes thing a little complicated since now we need
to do finch experiment for 6 cases (1 disabled case + 5 different threshold
enabled cases) (of course this is fine).

I'm also not quite sure about the priority of switch flags and field trail
flags. Does switches come first than field trail? Or they're independent of each
other? 

The last question is I think I may should not put FieldTrial related code in
chromeos_switches.cc? But this should be easily modified once the former
questions get clear.

Thanks for your help!

[Mr4D (OOO till 08-26), 2015-02-09 00:54:54]

To answer your questions:

1. Order of field trials vs. switches

    see my comment in source, but in the essence: The missing of the switch
indicates that field trials can be used.

2. There are two switches here

   Correct. This is not great, but since this is temporary anyways and we have
already histograms.xml entries created for it, I do not want to change that
again.
As such I thought that the fieldtrial should influence both switches at the same
time (note: A fieldtrial does not need to be an exact match of flags). So my
idea was to introduce another flag function in chromeos_switches.cc which can be
called to check if the advanced memory management should be used. It would first
check if the flag is provided by the user. If it is - ignore the field trial. If
it is not, check if the field trials say that it should be turned off or not.
replace then the if code in Browser_main_loop.cc with this new function. 

3. The logic leads to THRESHOLD_DEFAULT -> THRESHOLD_CONSERVATIVE?

This is exactly how it was meant to be. In case the user has specified nothing,
the default gets picked and the default is conservative. So Default is not
another state - it is only the placeholder for the default (what ever that might
be after this field trial is finished). So there is no need to add a field trial
flag for the threshold "default".

I hope I haven't totally confused you now :)

https://codereview.chromium.org/897413002/diff/60001/chromeos/chromeos_switch...
File chromeos/chromeos_switches.cc (left):

https://codereview.chromium.org/897413002/diff/60001/chromeos/chromeos_switch...
chromeos/chromeos_switches.cc:317: kMemoryPressureThresholds)) {
The field trial code should go into the case that there is no switch specified.

The order is:
1. User settings are most important -> they cannot be overwritten.
2. Trials can be used if 1. does not exist.

[xdai1, 2015-02-09 19:20:50]

Thanks Stefan, that's much clearer! Please take another look at the code,
thanks!

[Mr4D (OOO till 08-26), 2015-02-09 22:16:23]

Please see comment!

(one nit and we are there!)

https://codereview.chromium.org/897413002/diff/80001/chromeos/chromeos_switch...
File chromeos/chromeos_switches.cc (left):

https://codereview.chromium.org/897413002/diff/80001/chromeos/chromeos_switch...
chromeos/chromeos_switches.cc:317: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
As stated earlier - this code should remain and you should move the field trial
cases into this if case (since there is no user setting in this case the field
trials can be used).

[xdai1, 2015-02-09 22:34:07]

On 2015/02/09 22:16:23, Mr4D (TOK time) wrote:
> Please see comment!
> 
> (one nit and we are there!)
> 
>
https://codereview.chromium.org/897413002/diff/80001/chromeos/chromeos_switch...
> File chromeos/chromeos_switches.cc (left):
> 
>
https://codereview.chromium.org/897413002/diff/80001/chromeos/chromeos_switch...
> chromeos/chromeos_switches.cc:317: if
> (!base::CommandLine::ForCurrentProcess()->HasSwitch(
> As stated earlier - this code should remain and you should move the field
trial
> cases into this if case (since there is no user setting in this case the field
> trials can be used).

I think we may not need this if case since we only need to call
GetMomoryPressureThresholds() function when MemoryPressureHandlingEnabled()
returns true, which means the switch for kDisableMemoryPressureSystemChromeOS
doesn't exist and field trail flag for kMemoryPressureThresholds doesn't equal
kMemoryPressureHandlingOff.

Based on your previous answer, I think the logic should be:
if(No switch flag for kDisableMemoryPressureSystemChromeOS)
  if(switch flag for kMemoryPressureThresholds != off) 
     // using switch flag for kMemoryPressureThresholds
  else
    // using filed trails

Since the two if-case are always true when we can GetMemoryPressureThresholds()
function, so I think we don't need to specify them again. 

Correct me if I'm wrong:)

[Mr4D (OOO till 08-26), 2015-02-09 22:51:14]

Actually:

if (no switch flag for kMemoryPressureThresholds exisits) {
   // The user did not specify any thresholds.
   // Check field trials.
   ...
   return DEFUALT;
]
use kMemoryPressureThresholds key to find the user specified flag value.


So this function does not check the kDisabledMemoryPressure... function and it
has no need to since it shouldn't even be called if it is disabled.

[xdai1, 2015-02-09 23:35:07]

On 2015/02/09 22:51:14, Mr4D (TOK time) wrote:
> Actually:
> 
> if (no switch flag for kMemoryPressureThresholds exisits) {
>    // The user did not specify any thresholds.
>    // Check field trials.
>    ...
>    return DEFUALT;
> ]
> use kMemoryPressureThresholds key to find the user specified flag value.
> 
> 
> So this function does not check the kDisabledMemoryPressure... function and it
> has no need to since it shouldn't even be called if it is disabled.

Thanks for your help! Please take another look at the code:)

[Mr4D (OOO till 08-26), 2015-02-09 23:41:56]

LGTM!

Thanks!

[xdai1, 2015-02-11 00:55:00]

xdai@chromium.org changed reviewers:
+ creis@chromium.org

[xdai1, 2015-02-11 00:55:32]

On 2015/02/09 23:41:56, Mr4D (TOK time) wrote:
> LGTM!
> 
> Thanks!

Thanks for the review! creis@, can you help to review the CL, please?

[xdai1, 2015-02-12 00:31:37]

creis@, ping?

[Charlie Reis, 2015-02-12 00:48:07]

Sorry for the delay; today was a bit overwhelming.  content/ LGTM.

[xdai1, 2015-02-12 02:04:31]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2015-02-12 02:05:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/897413002/100001

[commit-bot: I haz the power, 2015-02-12 02:08:42]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-02-12 02:09:27]

Patchset 4 (id:??) landed as
https://crrev.com/f735c7a31f0c45b71545d06034c220eb976909cd
Cr-Commit-Position: refs/heads/master@{#315907}