evict larger entries first

net/disk_cache/simple/simple_experiment.cc

Index: net/disk_cache/simple/simple_experiment.cc
diff --git a/net/disk_cache/simple/simple_experiment.cc b/net/disk_cache/simple/simple_experiment.cc
index 38ea589322f97df405f9b48bcaa349510847ce52..f48b9a31e2e661eeca321d61b208bfe712a2d660 100644
--- a/net/disk_cache/simple/simple_experiment.cc
+++ b/net/disk_cache/simple/simple_experiment.cc
@@ -15,10 +15,24 @@ namespace disk_cache {
 
 const base::Feature kSimpleSizeExperiment = {"SimpleSizeExperiment",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kSimpleCacheEvictionWithSize = {
+    "SimpleCacheEvictionWithSize", base::FEATURE_DISABLED_BY_DEFAULT};
+
 const char kSizeMultiplierParam[] = "SizeMultiplier";
 
 namespace {
 
+// Updates experiment if the experiment is active.
+void CheckForSimpleCacheEvictionWithSizeExperiment(

[pasko, 2017/06/29 15:27:25]

nit: "SimpleCache" looks redundant here, I'd suggest s/SimpleCache//

Another option is just to remove "Simple" to be consistent with
"CheckForSimpleSizeExperiment", but I'd better remove "Simple" from the latter.

[hubbe, 2017/06/29 18:55:27]

Done.

+    disk_cache::SimpleExperiment* experiment) {
+  if (experiment->type != disk_cache::SimpleExperimentType::NONE)
+    return;
+
+  if (base::FeatureList::GetFieldTrial(kSimpleCacheEvictionWithSize)) {

[pasko, 2017/06/29 15:27:25]

nit: the common convention is not to put curly braces around a single line

[hubbe, 2017/06/29 18:55:26]

Done.

+    experiment->type = SimpleExperimentType::EVICT_WITH_SIZE;

[jkarlin, 2017/06/29 16:26:11]

You should also parse the parameter and use the parameter as your enable/disable
switch. That way both control and experiment have EVICT_WITH_SIZE cache types
(causing the caches to reset) but only if the param is 1 will you actually
enable the feature.

[hubbe, 2017/06/29 18:55:26]

My intention was to have the control group disable the
SimpleCacheEvictionWithSize feature so that the experiment is associated with
the feature for both groups.

If I follow your suggestion, how do I enable the new cache eviction from the
command line?

[jkarlin, 2017/06/29 19:17:01]

But then you wouldn't clear the control's cache? Seems better to clear it to get
apples-to-apples comparisons.
According to the experiment params portion of the field trial docs:

chrome.exe --enable-features="MyFeatureName<MyStudyName"
  --force-fieldtrials=MyStudyName/Group1
  --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test

[hubbe, 2017/06/29 19:26:48]

I don't understand, why wouldn't it?
My intention was to have the cache cleared for both groups.
Wouldn't the experiment be associated with the feature for the control group as
well?
So the control group needs to have
--disable-features="SimpleCacheEvictionWithSize<Study" or possibly
--enable-features="*SimpleCacheEvictionWithSize<Study" and then it should work,
right?

[jkarlin, 2017/06/30 11:57:27]

Ah, I think I understand now. You want to use the same field trial config file
as the size study, and you want to use the same control group. 

First, note that the size study expires at the end of today, the 30th. So there
is no issue with starting a whole new field trial experiment.

Second, if it didn't expire today, you still wouldn't want to use that control
group in your new experiment as it wouldn't be apples-to-apples since it started
months ago. 

[hubbe, 2017/06/30 17:30:34]

No, I wasn't planning on using the same field trial.
As far as I can tell, in your experiment, the Feature controls if the experiment
is on or off, and the parameter controls what to actually do.

In my experiment, I want the Feature to directly control weather we evict by
size or not so that I can still enable that on the command line for my own
testing. The experiment would simply turn the feature on or off, and in doing
so, the experiment gets associated with the feature.

Maybe I've misunderstood how something works?

[jkarlin, 2017/06/30 18:18:48]

Correct. And setting the parameter to 100 is a noop, and behaves as a control.
You can do that, but then you don't have a control. If the control is simply to
disable the feature, then the control group will just be the normal user
population which doesn't create a new cache.
The SimpleExperiment works by writing the experiment type and param to the
SimpleCache's metadata on disk. If either the type or param change, the old
cache is destroyed and a new one is built with the new type and param. So if you
want a control group, you have to change the feature, param, or both, from the
default.

[hubbe, 2017/06/30 19:00:43]

I just change experiment->type.

The control group will have the experiment explicitly disabling the feature,
which will associate the experiment with the feature. This will make this code
set the experiment type, which will clear the cache for the control group. If
this doesn't work, then I am misunderstanding how something works.

I didn't realize the param was also written into the index, so I fixed that.

+  }
+}
+
 // Returns true if the experiment is found and properly defined.
 bool CheckForSimpleSizeExperiment(disk_cache::SimpleExperiment* experiment) {
   DCHECK_EQ(disk_cache::SimpleExperimentType::NONE, experiment->type);
@@ -58,6 +72,7 @@ SimpleExperiment GetSimpleExperiment(net::CacheType cache_type) {
     return experiment;
 
   CheckForSimpleSizeExperiment(&experiment);

[pasko, 2017/06/29 15:27:25]

jkarlin: why are we ignoring the return value here?

[jkarlin, 2017/06/29 16:26:12]

You're right, we should early return if it returns true.

+  CheckForSimpleCacheEvictionWithSizeExperiment(&experiment);

[pasko, 2017/06/29 15:27:24]

this has the ability to drop the cache in the experiment group, but afaict it
cannot drop cache in the control group. Is it intentional?

[jkarlin, 2017/06/29 16:26:11]

Discussed in a comment above.

[hubbe, 2017/06/29 18:55:26]

Acknowledged.

[hubbe, 2017/06/29 18:55:27]

See comment on line 32.

   return experiment;
 }