change LRU-K Evict() signature (#731)

* change Evict() signature

* update stand-in cpp

* update lru k tests for new evict signature

Author: connortsui20

src/buffer/lru_k_replacer.cpp

@@ -17,7 +17,7 @@ namespace bustub {
 
 LRUKReplacer::LRUKReplacer(size_t num_frames, size_t k) : replacer_size_(num_frames), k_(k) {}
 
-auto LRUKReplacer::Evict(frame_id_t *frame_id) -> bool { return false; }
+auto LRUKReplacer::Evict() -> std::optional<frame_id_t> { return std::nullopt; }
 
 void LRUKReplacer::RecordAccess(frame_id_t frame_id, [[maybe_unused]] AccessType access_type) {}
 

src/include/buffer/lru_k_replacer.h

@@ -15,6 +15,7 @@
 #include <limits>
 #include <list>
 #include <mutex>  // NOLINT
+#include <optional>
 #include <unordered_map>
 #include <vector>
 
@@ -83,7 +84,7 @@ class LRUKReplacer {
    * @param[out] frame_id id of frame that is evicted.
    * @return true if a frame is evicted successfully, false if no frames can be evicted.
    */
-  auto Evict(frame_id_t *frame_id) -> bool;
+  auto Evict() -> std::optional<frame_id_t>;
 
   /**
    * TODO(P1): Add implementation

test/buffer/lru_k_replacer_test.cpp

@@ -17,9 +17,15 @@
 namespace bustub {
 
 TEST(LRUKReplacerTest, DISABLED_SampleTest) {
+  // Note that comparison with `std::nullopt` always results in `false`, and if the optional type actually does contain
+  // a value, the comparison will compare the inner value.
+  // See: https://devblogs.microsoft.com/oldnewthing/20211004-00/?p=105754
+  std::optional<frame_id_t> frame;
+
+  // Initialize the replacer.
   LRUKReplacer lru_replacer(7, 2);
 
-  // Scenario: add six elements to the replacer. We have [1,2,3,4,5]. Frame 6 is non-evictable.
+  // Add six frames to the replacer. We now have frames [1, 2, 3, 4, 5]. We set frame 6 as non-evictable.
   lru_replacer.RecordAccess(1);
   lru_replacer.RecordAccess(2);
   lru_replacer.RecordAccess(3);
@@ -32,25 +38,24 @@ TEST(LRUKReplacerTest, DISABLED_SampleTest) {
   lru_replacer.SetEvictable(4, true);
   lru_replacer.SetEvictable(5, true);
   lru_replacer.SetEvictable(6, false);
+
+  // The size of the replacer is the number of frames that can be evicted, _not_ the total number of frames entered.
   ASSERT_EQ(5, lru_replacer.Size());
 
-  // Scenario: Insert access history for frame 1. Now frame 1 has two access histories.
-  // All other frames have max backward k-dist. The order of eviction is [2,3,4,5,1].
+  // Record an access for frame 1. Now frame 1 has two accesses total.
   lru_replacer.RecordAccess(1);
+  // All other frames now share the maximum backward k-distance. Since we use timestamps to break ties, where the first
+  // to be evicted is the frame with the oldest timestamp, the order of eviction should be [2, 3, 4, 5, 1].
 
-  // Scenario: Evict three pages from the replacer. Elements with max k-distance should be popped
-  // first based on LRU.
-  int value;
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(2, value);
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(3, value);
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(4, value);
+  // Evict three pages from the replacer.
+  // To break ties, we use LRU with respect to the oldest timestamp, or the least recently used frame.
+  ASSERT_EQ(2, lru_replacer.Evict());
+  ASSERT_EQ(3, lru_replacer.Evict());
+  ASSERT_EQ(4, lru_replacer.Evict());
   ASSERT_EQ(2, lru_replacer.Size());
+  // Now the replacer has the frames [5, 1].
 
-  // Scenario: Now replacer has frames [5,1].
-  // Insert new frames 3, 4, and update access history for 5. We should end with [3,1,5,4]
+  // Insert new frames [3, 4], and update the access history for 5. Now, the ordering is [3, 1, 5, 4].
   lru_replacer.RecordAccess(3);
   lru_replacer.RecordAccess(4);
   lru_replacer.RecordAccess(5);
@@ -59,40 +64,59 @@ TEST(LRUKReplacerTest, DISABLED_SampleTest) {
   lru_replacer.SetEvictable(4, true);
   ASSERT_EQ(4, lru_replacer.Size());
 
-  // Scenario: continue looking for victims. We expect 3 to be evicted next.
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(3, value);
+  // Look for a frame to evict. We expect frame 3 to be evicted next.
+  ASSERT_EQ(3, lru_replacer.Evict());
   ASSERT_EQ(3, lru_replacer.Size());
 
-  // Set 6 to be evictable. 6 Should be evicted next since it has max backward k-dist.
+  // Set 6 to be evictable. 6 Should be evicted next since it has the maximum backward k-distance.
   lru_replacer.SetEvictable(6, true);
   ASSERT_EQ(4, lru_replacer.Size());
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(6, value);
+  ASSERT_EQ(6, lru_replacer.Evict());
   ASSERT_EQ(3, lru_replacer.Size());
 
-  // Now we have [1,5,4]. Continue looking for victims.
+  // Mark frame 1 as non-evictable. We now have [5, 4].
   lru_replacer.SetEvictable(1, false);
+
+  // We expect frame 5 to be evicted next.
   ASSERT_EQ(2, lru_replacer.Size());
-  ASSERT_EQ(true, lru_replacer.Evict(&value));
-  ASSERT_EQ(5, value);
+  ASSERT_EQ(5, lru_replacer.Evict());
   ASSERT_EQ(1, lru_replacer.Size());
 
-  // Update access history for 1. Now we have [4,1]. Next victim is 4.
+  // Update the access history for frame 1 and make it evictable. Now we have [4, 1].
   lru_replacer.RecordAccess(1);
   lru_replacer.RecordAccess(1);
   lru_replacer.SetEvictable(1, true);
   ASSERT_EQ(2, lru_replacer.Size());
-  ASSERT_EQ(true, lru_replacer.Evict(&value));
-  ASSERT_EQ(value, 4);
 
+  // Evict the last two frames.
+  ASSERT_EQ(4, lru_replacer.Evict());
+  ASSERT_EQ(1, lru_replacer.Size());
+  ASSERT_EQ(1, lru_replacer.Evict());
+  ASSERT_EQ(0, lru_replacer.Size());
+
+  // Insert frame 1 again and mark it as non-evictable.
+  lru_replacer.RecordAccess(1);
+  lru_replacer.SetEvictable(1, false);
+  ASSERT_EQ(0, lru_replacer.Size());
+
+  // A failed eviction should not change the size of the replacer.
+  frame = lru_replacer.Evict();
+  ASSERT_EQ(false, frame.has_value());
+
+  // Mark frame 1 as evictable again and evict it.
+  lru_replacer.SetEvictable(1, true);
   ASSERT_EQ(1, lru_replacer.Size());
-  lru_replacer.Evict(&value);
-  ASSERT_EQ(value, 1);
+  ASSERT_EQ(1, lru_replacer.Evict());
   ASSERT_EQ(0, lru_replacer.Size());
 
-  // This operation should not modify size
-  ASSERT_EQ(false, lru_replacer.Evict(&value));
+  // There is nothing left in the replacer, so make sure this doesn't do something strange.
+  frame = lru_replacer.Evict();
+  ASSERT_EQ(false, frame.has_value());
   ASSERT_EQ(0, lru_replacer.Size());
+
+  // Make sure that setting a non-existent frame as evictable or non-evictable doesn't do something strange.
+  lru_replacer.SetEvictable(6, false);
+  lru_replacer.SetEvictable(6, true);
 }
+
 }  // namespace bustub