evict larger entries first

net/disk_cache/simple/simple_index_unittest.cc

Index: net/disk_cache/simple/simple_index_unittest.cc
diff --git a/net/disk_cache/simple/simple_index_unittest.cc b/net/disk_cache/simple/simple_index_unittest.cc
index 9f7d4094e10e6fcded31e3fbb189b8b2334de86d..bfe892cc572bb3b5847ea9b84d5101d8c113cede 100644
--- a/net/disk_cache/simple/simple_index_unittest.cc
+++ b/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"
@@ -568,6 +573,60 @@ TEST_F(SimpleIndexTest, BasicEviction) {
   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) {