Changes the eviction algorithm to take the size of the entry into

account. Weighting by size means that large entries will get thrown out
of the cache sooner and the end result is that the cache will have more,
but smaller entries.

Design doc: https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNKCY/edit

BUG=700102, 736437

Review-Url: https://codereview.chromium.org/2918893002
Cr-Commit-Position: refs/heads/master@{#488058}

net/disk_cache/simple/simple_experiment.cc

@@ -15,49 +15,61 @@ namespace disk_cache {
 
 const base::Feature kSimpleSizeExperiment = {"SimpleSizeExperiment",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kSimpleCacheEvictionWithSizeExperiment = {
+    "SimpleCacheEvictionWithSizeExperiment", base::FEATURE_DISABLED_BY_DEFAULT};
+
 const char kSizeMultiplierParam[] = "SizeMultiplier";
+const char kSizeEvictionParam[] = "SizeEviction";
 
 namespace {
 
-// Returns true if the experiment is found and properly defined.
-bool CheckForSimpleSizeExperiment(disk_cache::SimpleExperiment* experiment) {
-  DCHECK_EQ(disk_cache::SimpleExperimentType::NONE, experiment->type);
-  DCHECK_EQ(0u, experiment->param);
-
-  if (!base::FeatureList::IsEnabled(kSimpleSizeExperiment))
-    return false;
-
-  base::FieldTrial* trial =
-      base::FeatureList::GetFieldTrial(kSimpleSizeExperiment);
-  if (!trial)
-    return false;
-
-  std::map<std::string, std::string> params;
-  base::FieldTrialParamAssociator::GetInstance()->GetFieldTrialParams(
-      trial->trial_name(), &params);
-  auto iter = params.find(kSizeMultiplierParam);
-  if (iter == params.end())
-    return false;
-
-  uint32_t param;
-  if (!base::StringToUint(iter->second, &param))
-    return false;
-
-  experiment->type = disk_cache::SimpleExperimentType::SIZE;
-  experiment->param = param;
-  return true;
-}
+struct ExperimentDescription {
+  disk_cache::SimpleExperimentType experiment_type;
+  const base::Feature* feature;
+  const char* param_name;
+};
+
+// List of experimens to be checked for.
+const ExperimentDescription experiments[] = {
+    {disk_cache::SimpleExperimentType::SIZE, &kSimpleSizeExperiment,
+     kSizeMultiplierParam},
+    {disk_cache::SimpleExperimentType::EVICT_WITH_SIZE,
+     &kSimpleCacheEvictionWithSizeExperiment, kSizeEvictionParam},
+};
 
 }  // namespace
 
 // Returns the experiment for the given |cache_type|.
 SimpleExperiment GetSimpleExperiment(net::CacheType cache_type) {
   SimpleExperiment experiment;
-
   if (cache_type != net::DISK_CACHE)
     return experiment;
 
-  CheckForSimpleSizeExperiment(&experiment);
+  for (size_t i = 0; i < arraysize(experiments); i++) {
+    if (!base::FeatureList::IsEnabled(*experiments[i].feature))
+      continue;
+
+    base::FieldTrial* trial =
+        base::FeatureList::GetFieldTrial(*experiments[i].feature);
+    if (!trial)
+      continue;
+
+    std::map<std::string, std::string> params;
+    base::FieldTrialParamAssociator::GetInstance()->GetFieldTrialParams(
+        trial->trial_name(), &params);
+    auto iter = params.find(experiments[i].param_name);
+    if (iter == params.end())
+      continue;
+
+    uint32_t param;
+    if (!base::StringToUint(iter->second, &param))
+      continue;
+
+    experiment.type = experiments[i].experiment_type;
+    experiment.param = param;
+    return experiment;
+  }
+
   return experiment;
 }
 

net/disk_cache/simple/simple_experiment.h

@@ -14,13 +14,20 @@
 namespace disk_cache {
 
 NET_EXPORT_PRIVATE extern const base::Feature kSimpleSizeExperiment;
+NET_EXPORT_PRIVATE extern const base::Feature
+    kSimpleCacheEvictionWithSizeExperiment;
 NET_EXPORT_PRIVATE extern const char kSizeMultiplierParam[];
+NET_EXPORT_PRIVATE extern const char kSizeEvictionParam[];
 
 // This lists the experiment groups for SimpleCache. Only add new groups at
 // the end of the list, and always increase the number.
 enum class SimpleExperimentType : uint32_t {
   NONE = 0,
   SIZE = 1,
+
+  // param = 0 -> control group
+  // param = 1 -> experiment group
+  EVICT_WITH_SIZE = 2,
 };
 
 struct NET_EXPORT_PRIVATE SimpleExperiment {

net/disk_cache/simple/simple_experiment_unittest.cc

@@ -49,6 +49,28 @@ class SimpleExperimentTest : public testing::Test {
     scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
   }
 
+  void ConfigureEvictWithSizeTrial(bool enabled,
+                                   base::Optional<uint32_t> param) {
+    const std::string kTrialName = "EvictWithSizeTrial";
+    const std::string kGroupName = "GroupFoo";  // Value not used
+
+    scoped_refptr<base::FieldTrial> trial =
+        base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);
+
+    if (param) {
+      std::map<std::string, std::string> params;
+      params[kSizeEvictionParam] = base::UintToString(param.value());
+      base::FieldTrialParamAssociator::GetInstance()->AssociateFieldTrialParams(
+          kTrialName, kGroupName, params);
+    }
+
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->RegisterFieldTrialOverride(
+        kSimpleCacheEvictionWithSizeExperiment.name,
+        base::FeatureList::OVERRIDE_ENABLE_FEATURE, trial.get());
+    scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
+  }
+
   std::unique_ptr<base::FieldTrialList> field_trial_list_;
   base::test::ScopedFeatureList scoped_feature_list_;
 };
@@ -88,4 +110,33 @@ TEST_F(SimpleExperimentTest, SizeTrialProperlyConfiguredWrongCacheType) {
   EXPECT_EQ(0u, experiment.param);
 }
 
+TEST_F(SimpleExperimentTest, EvictWithSizeMissingParam) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  ConfigureEvictWithSizeTrial(true, base::Optional<uint32_t>());
+
+  SimpleExperiment experiment = GetSimpleExperiment(net::DISK_CACHE);
+  EXPECT_EQ(SimpleExperimentType::NONE, experiment.type);
+  EXPECT_EQ(0u, experiment.param);
+}
+
+TEST_F(SimpleExperimentTest, EvictWithSizeProperlyConfigured) {
+  const uint32_t kParam = 1u;
+  base::test::ScopedFeatureList scoped_feature_list;
+  ConfigureEvictWithSizeTrial(true, base::Optional<uint32_t>(kParam));
+
+  SimpleExperiment experiment = GetSimpleExperiment(net::DISK_CACHE);
+  EXPECT_EQ(SimpleExperimentType::EVICT_WITH_SIZE, experiment.type);
+  EXPECT_EQ(kParam, experiment.param);
+}
+
+TEST_F(SimpleExperimentTest, EvictWithSizeProperlyConfiguredWrongCacheType) {
+  const uint32_t kParam = 125u;
+  base::test::ScopedFeatureList scoped_feature_list;
+  ConfigureEvictWithSizeTrial(true, base::Optional<uint32_t>(kParam));
+
+  SimpleExperiment experiment = GetSimpleExperiment(net::APP_CACHE);
+  EXPECT_EQ(SimpleExperimentType::NONE, experiment.type);
+  EXPECT_EQ(0u, experiment.param);
+}
+
 }  // namespace disk_cache

net/disk_cache/simple/simple_index.cc

@@ -25,6 +25,7 @@
 #include "base/trace_event/memory_usage_estimator.h"
 #include "net/base/net_errors.h"
 #include "net/disk_cache/simple/simple_entry_format.h"
+#include "net/disk_cache/simple/simple_experiment.h"
 #include "net/disk_cache/simple/simple_histogram_macros.h"
 #include "net/disk_cache/simple/simple_index_delegate.h"
 #include "net/disk_cache/simple/simple_index_file.h"
@@ -49,6 +50,13 @@ const uint32_t kEvictionMarginDivisor = 20;
 
 const uint32_t kBytesInKb = 1024;
 
+// This is added to the size of each entry before using the size
+// to determine which entries to evict first. It's basically an
+// estimate of the filesystem overhead, but it also serves to flatten
+// the curve so that 1-byte entries and 2-byte entries are basically
+// treated the same.
+static const int kEstimatedEntryOverhead = 512;
+
 }  // namespace
 
 namespace disk_cache {
@@ -309,31 +317,37 @@ void SimpleIndex::StartEvictionIfNeeded() {
       static_cast<base::HistogramBase::Sample>(max_size_ / kBytesInKb));
 
   // Flatten for sorting.
-  std::vector<const std::pair<const uint64_t, EntryMetadata>*> entries;
+  std::vector<std::pair<uint64_t, const EntrySet::value_type*>> entries;
   entries.reserve(entries_set_.size());
+  uint32_t now = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds();
+  bool use_size = false;
+  SimpleExperiment experiment = GetSimpleExperiment(cache_type_);
+  if (experiment.type == SimpleExperimentType::EVICT_WITH_SIZE &&
+      experiment.param) {
+    use_size = true;
+  }
   for (EntrySet::const_iterator i = entries_set_.begin();
        i != entries_set_.end(); ++i) {
-    entries.push_back(&*i);
+    uint64_t sort_value = now - i->second.RawTimeForSorting();
+    if (use_size) {
+      // Will not overflow since we're multiplying two 32-bit values and storing
+      // them in a 64-bit variable.
+      sort_value *= i->second.GetEntrySize() + kEstimatedEntryOverhead;
+    }
+    // Subtract so we don't need a custom comparator.
+    entries.emplace_back(std::numeric_limits<uint64_t>::max() - sort_value,
+                         &*i);
   }
 
-  std::sort(entries.begin(), entries.end(),
-            [](const std::pair<const uint64_t, EntryMetadata>* a,
-               const std::pair<const uint64_t, EntryMetadata>* b) -> bool {
-              return a->second.RawTimeForSorting() <
-                     b->second.RawTimeForSorting();
-            });
-
-  // Remove as many entries from the index to get below |low_watermark_|,
-  // collecting least recently used hashes into |entry_hashes|.
-  std::vector<uint64_t> entry_hashes;
-  std::vector<const std::pair<const uint64_t, EntryMetadata>*>::iterator it =
-      entries.begin();
   uint64_t evicted_so_far_size = 0;
-  while (evicted_so_far_size < cache_size_ - low_watermark_) {
-    DCHECK(it != entries.end());
-    entry_hashes.push_back((*it)->first);
-    evicted_so_far_size += (*it)->second.GetEntrySize();
-    ++it;
+  const uint64_t amount_to_evict = cache_size_ - low_watermark_;
+  std::vector<uint64_t> entry_hashes;
+  std::sort(entries.begin(), entries.end());
+  for (const auto& score_metadata_pair : entries) {
+    if (evicted_so_far_size >= amount_to_evict)
+      break;
+    evicted_so_far_size += score_metadata_pair.second->second.GetEntrySize();
+    entry_hashes.push_back(score_metadata_pair.second->first);
   }
 
   SIMPLE_CACHE_UMA(COUNTS_1M,

net/disk_cache/simple/simple_index_unittest.cc

@@ -12,13 +12,18 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/hash.h"
 #include "base/logging.h"
+#include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_param_associator.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner.h"
+#include "base/test/mock_entropy_provider.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "net/base/cache_type.h"
+#include "net/disk_cache/simple/simple_experiment.h"
 #include "net/disk_cache/simple/simple_index_delegate.h"
 #include "net/disk_cache/simple/simple_index_file.h"
 #include "net/disk_cache/simple/simple_test_util.h"
@@ -113,6 +118,29 @@ class SimpleIndexTest  : public testing::Test, public SimpleIndexDelegate {
     index_->Initialize(base::Time());
   }
 
+  void EnableEvictBySize() {
+    // Enable size-based eviction.
+    const std::string kTrialName = "EvictWithSizeTrial";
+    const std::string kGroupName = "GroupFoo";  // Value not used
+
+    field_trial_list_ = base::MakeUnique<base::FieldTrialList>(
+        base::MakeUnique<base::MockEntropyProvider>());
+
+    scoped_refptr<base::FieldTrial> trial =
+        base::FieldTrialList::CreateFieldTrial(kTrialName, kGroupName);
+
+    std::map<std::string, std::string> params;
+    params[kSizeEvictionParam] = "1";
+    base::FieldTrialParamAssociator::GetInstance()->AssociateFieldTrialParams(
+        kTrialName, kGroupName, params);
+
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->RegisterFieldTrialOverride(
+        kSimpleCacheEvictionWithSizeExperiment.name,
+        base::FeatureList::OVERRIDE_ENABLE_FEATURE, trial.get());
+    scoped_feature_list_.InitWithFeatureList(std::move(feature_list));
+  }
+
   void WaitForTimeChange() {
     const base::Time initial_time = base::Time::Now();
     do {
@@ -165,6 +193,9 @@ class SimpleIndexTest  : public testing::Test, public SimpleIndexDelegate {
   std::unique_ptr<SimpleIndex> index_;
   base::WeakPtr<MockSimpleIndexFile> index_file_;
 
+  std::unique_ptr<base::FieldTrialList> field_trial_list_;
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   std::vector<uint64_t> last_doom_entry_hashes_;
   int doom_entries_calls_;
 };
@@ -568,6 +599,74 @@ TEST_F(SimpleIndexTest, BasicEviction) {
   ASSERT_EQ(2u, last_doom_entry_hashes().size());
 }
 
+TEST_F(SimpleIndexTest, EvictBySize) {
+  EnableEvictBySize();
+  // Enabled, now we can run the actual test.
+  base::Time now(base::Time::Now());
+  index()->SetMaxSize(50000);
+  InsertIntoIndexFileReturn(hashes_.at<1>(), now - base::TimeDelta::FromDays(2),
+                            475u);
+  InsertIntoIndexFileReturn(hashes_.at<2>(), now - base::TimeDelta::FromDays(1),
+                            40000u);
+  ReturnIndexFile();
+  WaitForTimeChange();
+
+  index()->Insert(hashes_.at<3>());
+  // Confirm index is as expected: No eviction, everything there.
+  EXPECT_EQ(3, index()->GetEntryCount());
+  EXPECT_EQ(0, doom_entries_calls());
+  EXPECT_TRUE(index()->Has(hashes_.at<1>()));
+  EXPECT_TRUE(index()->Has(hashes_.at<2>()));
+  EXPECT_TRUE(index()->Has(hashes_.at<3>()));
+
+  // Trigger an eviction, and make sure the right things are tossed.
+  // TODO(rdsmith): This is dependent on the innards of the implementation
+  // as to at exactly what point we trigger eviction. Not sure how to fix
+  // that.
+  index()->UpdateEntrySize(hashes_.at<3>(), 40000u);
+  EXPECT_EQ(1, doom_entries_calls());
+  EXPECT_EQ(2, index()->GetEntryCount());
+  EXPECT_TRUE(index()->Has(hashes_.at<1>()));
+  EXPECT_FALSE(index()->Has(hashes_.at<2>()));
+  EXPECT_TRUE(index()->Has(hashes_.at<3>()));
+  ASSERT_EQ(1u, last_doom_entry_hashes().size());
+}
+
+// Same as test above, but using much older entries to make sure that small
+// things eventually get evictied.
+TEST_F(SimpleIndexTest, EvictBySize2) {
+  EnableEvictBySize();
+  // Enabled, now we can run the actual test.
+  base::Time now(base::Time::Now());
+  index()->SetMaxSize(50000);
+  InsertIntoIndexFileReturn(hashes_.at<1>(),
+                            now - base::TimeDelta::FromDays(200), 475u);
+  InsertIntoIndexFileReturn(hashes_.at<2>(), now - base::TimeDelta::FromDays(1),
+                            40000u);
+  ReturnIndexFile();
+  WaitForTimeChange();
+
+  index()->Insert(hashes_.at<3>());
+  // Confirm index is as expected: No eviction, everything there.
+  EXPECT_EQ(3, index()->GetEntryCount());
+  EXPECT_EQ(0, doom_entries_calls());
+  EXPECT_TRUE(index()->Has(hashes_.at<1>()));
+  EXPECT_TRUE(index()->Has(hashes_.at<2>()));
+  EXPECT_TRUE(index()->Has(hashes_.at<3>()));
+
+  // Trigger an eviction, and make sure the right things are tossed.
+  // TODO(rdsmith): This is dependent on the innards of the implementation
+  // as to at exactly what point we trigger eviction. Not sure how to fix
+  // that.
+  index()->UpdateEntrySize(hashes_.at<3>(), 40000u);
+  EXPECT_EQ(1, doom_entries_calls());
+  EXPECT_EQ(1, index()->GetEntryCount());
+  EXPECT_FALSE(index()->Has(hashes_.at<1>()));
+  EXPECT_FALSE(index()->Has(hashes_.at<2>()));
+  EXPECT_TRUE(index()->Has(hashes_.at<3>()));
+  ASSERT_EQ(2u, last_doom_entry_hashes().size());
+}
+
 // Confirm all the operations queue a disk write at some point in the
 // future.
 TEST_F(SimpleIndexTest, DiskWriteQueued) {