evict larger entries first

net/disk_cache/simple/simple_index_unittest.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/disk_cache/simple/simple_index.h"
 
 #include <algorithm>
 #include <functional>
 #include <memory>
 #include <utility>
 
 #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"
 #include "net/disk_cache/simple/simple_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace disk_cache {
 namespace {
 
 const base::Time kTestLastUsedTime =
@@ skipping 529 matching lines @@
   // that.
   index()->UpdateEntrySize(hashes_.at<3>(), 475u);
   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());
 }
 
+TEST_F(SimpleIndexTest, EvictBySize) {
+  // Enable size-based eviction.
+  const std::string kTrialName = "EvictWithSizeTrial";
+  const std::string kGroupName = "GroupFoo";  // Value not used
+
+  std::unique_ptr<base::FieldTrialList> 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());
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitWithFeatureList(std::move(feature_list));
+
+  // 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);

[pasko, 2017/07/18 15:30:01]

Perhaps make it depend on the implementation in a tighter way? like make it just
1 byte bigger than the value that prevents this eviction? At least this would
add some more confidence to me, but maybe it's all too obvious for you :)

Hmm, now thinking more about it, one byte off may be lengthy to explain, in
which case I would lean on the side of ease of explaining why the choice of
sizes is such, maybe even without changing the actual test.

Non-mandatory suggestion, up to you, this is just a test, and I almost left
shopping, etc.

[hubbe, 2017/07/18 18:27:40]

Making the test tightly coupled to the implementation seems like a bad idea to
me.
A few more tests might be helpful, but it's not like the previous implementation
had a lot of tests either...

[pasko, 2017/07/19 14:42:28]

OK

+  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

[pasko, 2017/07/18 15:30:01]

rdsmith: what do you mean? Surely eviction triggering depends on how sizes of
entries evolve. Did you want to expose something like RunEvictionNowForTesting?
Otherwise I cannot imagine this TODO being addressed or removed in 5 years from
now. 

[hubbe, 2017/07/18 18:27:40]

Sure, but what if we PostTask the eviction? Or do it in a separate thread? Or
something else...

[pasko, 2017/07/19 14:42:28]

Ah, so you mean in the proper design the test would need to use an official API
for waiting for eviction to complete? Makes sense. Still not sure there is
enough wood behind this arrow to make us motivated to fix this TODO. Would not
actively object to keeping it, thanks for clarification.

[hubbe, 2017/07/19 17:50:33]

I consider it more of an FYI than a TODO to make sure that whoever ends up
changing when eviction occurs isn't mystified as to why the tests fail... I
didn't write this TODO, but I think keeping it makes sense, even if it never
actually gets fixed.

+  // 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>()));

[pasko, 2017/07/18 15:30:01]

maybe also test the case when the smaller and older entry is evicted first? Up
to you.

[hubbe, 2017/07/18 18:27:40]

Done.

+  EXPECT_TRUE(index()->Has(hashes_.at<3>()));
+  ASSERT_EQ(1u, last_doom_entry_hashes().size());
+}
+
 // Confirm all the operations queue a disk write at some point in the
 // future.
 TEST_F(SimpleIndexTest, DiskWriteQueued) {
   index()->SetMaxSize(1000);
   ReturnIndexFile();
 
   EXPECT_FALSE(index()->write_to_disk_timer_.IsRunning());
 
   const uint64_t kHash1 = hashes_.at<1>();
   index()->Insert(kHash1);
@@ skipping 58 matching lines @@
   WaitForTimeChange();
   EXPECT_EQ(expected_trigger, index()->write_to_disk_timer_.desired_run_time());
   index()->Insert(hashes_.at<2>());
   index()->UpdateEntrySize(hashes_.at<2>(), 40u);
   EXPECT_TRUE(index()->write_to_disk_timer_.IsRunning());
   EXPECT_LT(expected_trigger, index()->write_to_disk_timer_.desired_run_time());
   index()->write_to_disk_timer_.Stop();
 }
 
 }  // namespace disk_cache