fixes for AdvancedLRU with naive implementtion, LRUCache, and AnyCallback (#144)

Author: apatrida

src/test/kotlin/com/igorwojda/cache/advancedlru/Challenge.kt

@@ -1,52 +1,19 @@
 package com.igorwojda.cache.advancedlru
 
-import org.amshove.kluent.shouldBeEqualTo
-import org.junit.jupiter.api.Test
-import java.util.*
+import java.time.Clock
+import java.time.Duration
 
-class AdvancedLRUCache(private val capacity: Int) {
-    fun put(key: String, value: Int, priority: Int, expiryTime: Long) {
-        TODO("Add your solution here")
-    }
-
-    fun get(key: String): Int? {
-        TODO("Add your solution here")
-    }
-
-    // Returns fixed system time in milliseconds
-    private fun getSystemTimeForExpiry() = 1000
+interface LRUCache<K: Any, V: Any> {
+    fun put(key: K, value: V, priority: Int, ttl: Duration)
+    fun get(key: K): V?
 }
 
-private class Test {
-    @Test
-    fun `add and get`() {
-        val cache = AdvancedLRUCache(2)
-        cache.put("A", 1, 5, 5000)
-
-        cache.get("A") shouldBeEqualTo 1
-    }
-
-    @Test
-    fun `evict by priority`() {
-        val cache = AdvancedLRUCache(2)
-        cache.put("A", 1, 1, 3000)
-        cache.put("B", 2, 3, 4000)
-        cache.put("C", 3, 4, 5000)
-
-        // This should be null because "A" was evicted due to lower priority.
-        cache.get("A") shouldBeEqualTo null
-        cache.get("B") shouldBeEqualTo 2
-        cache.get("C") shouldBeEqualTo 3
+class AdvancedLRUCache<K: Any, V: Any>(private val capacity: Int, private val clock: Clock = Clock.systemDefaultZone()): LRUCache<K, V> {
+    override fun put(key: K, value: V, priority: Int, ttl: Duration) {
+        TODO("Add your solution here")
     }
 
-    @Test
-    fun `evict by expiry`() {
-        val cache = AdvancedLRUCache(2)
-        cache.put("A", 1, 1, 500)
-        cache.put("B", 2, 3, 700)
-
-        // This should be null because "A" was evicted due to expiry.
-        cache.get("A") shouldBeEqualTo null
-        cache.get("B") shouldBeEqualTo null
+    override fun get(key: K): V? {
+        TODO("Add your solution here")
     }
 }

src/test/kotlin/com/igorwojda/cache/advancedlru/README.md

@@ -11,38 +11,39 @@ limit. In cases where the addition of new items exceeds this capacity, ensure th
 following sequence of operations:
 
 - Firstly, discard items that have exceeded their validity period (`expiryTime` > `getSystemTimeForExpiry()`).
-- If there are no items past their validity, identify the items with the lowest priority rating and from these, remove 
+- If there are no items past their validity, identify the items with the earliest expiry time, and from those the items with the lowest priority rating, and from these remove 
 the item that was least recently accessed or used.
 
-To simplify expiry logic testing use provided `getSystemTime()` method (instead of `System.currentTimeMillis()`) that 
-will return fixed system time in milliseconds.
+To simplify expiry logic testing use the provided `Clock` to determine the current time in milliseconds using `clock.millis()`.
 
-[Challenge](Challenge.kt) | [Solution](Solution.kt)
+[Challenge](Challenge.kt) | [Solution](Solution.kt) | [Tests](Tests.kt)
 
 ## Examples
 
 ```kotlin
 val cache = AdvancedLRUCache(2)
-cache.put("A", 1, 5, 5000)
+cache.put("A", 1, 5, Duration.ofMinutes(15))
 cache.get("A") // 1
 ```
 
 ```kotlin
-val cache = AdvancedLRUCache(2)
-cache.put("A", 1, 1, 3000)
-cache.put("B", 2, 3, 4000)
-cache.put("C", 3, 4, 5000)
+val cache = AdvancedLRUCache(2, Clock.fixed(...))  // testing clock, fixed at a moment in time
+cache.put("A", 1, 5, Duration.ofMinutes(15))
+cache.put("B", 2, 1, Duration.ofMinutes(15))
+cache.put("C", 3, 10, Duration.ofMinutes(15))
 
 
-cache.get("A") // null - "A" was evicted due to lower priority.
-cache.get("B") // 2
+cache.get("A") // 1
+cache.get("B") // null - "B" was evicted due to lower priority.
 cache.get("C") // 3
 ```
 
 ```kotlin
-val cache = AdvancedLRUCache(2)
-cache.put("A", 1, 1, 500)
-cache.put("B", 2, 3, 700)
+val cache = AdvancedLRUCache(100)
+cache.put("A", 1, 1, Duration.ofMillis(1))
+cache.put("B", 2, 1, Duration.ofMillis(1))
+
+sleep(100)
 
 cache.get("A") // null - "A" was evicted due to expiry. 
 cache.get("B") // null - "B" was evicted due to expiry.

src/test/kotlin/com/igorwojda/cache/advancedlru/Solution.kt

@@ -1,83 +1,109 @@
 package com.igorwojda.cache.advancedlru
 
+import java.time.Clock
+import java.time.Duration
 import java.util.*
 
-// Implementation is using  combination of HashMap and LinkedList.
-// Time Complexity: O(1)
-private object Solution1 {
-    class AdvancedLRUCache(private val capacity: Int) {
-        private val map: MutableMap<String, CacheItem> = mutableMapOf()
-        private val priorityQueue: PriorityQueue<CacheItem> = PriorityQueue()
+// Implementation is using  combination of HashMap and PriorityQueue.
+// Time Complexity: O(N) (JVM priority queue is O(log(n)) on offer/poll methods and O(N) on remove(item) method)
+internal object Solution1 {
+    class AdvancedLRUCache<K: Any, V: Any>(private val capacity: Int, private val clock: Clock = Clock.systemDefaultZone()) : LRUCache<K, V> {
+        private val map: MutableMap<K, CacheItem<K, V>> = mutableMapOf()
 
-        fun put(key: String, value: Int, priority: Int, expiryTime: Long) {
-            if (map.containsKey(key)) {
-                remove(key)
-            }
+        private val expiryQueue: PriorityQueue<CacheItem<K, V>> = PriorityQueue { item1, item2 ->
+            compareBy<CacheItem<K, V>>({ it.expiryTime }).compare(item1, item2)
+        }
 
-            if (map.size == capacity) {
-                clearCache()
-            }
+        private val priorityQueue: PriorityQueue<CacheItem<K, V>> = PriorityQueue { item1, item2 ->
+            compareBy<CacheItem<K, V>>({ it.priority }, { it.lastUsed }).compare(item1, item2)
+        }
 
-            val item = CacheItem(key, value, priority, expiryTime)
+        override fun put(key: K, value: V, priority: Int, ttl: Duration) {
+            remove(key)
+            checkAndExpireCachedItems()
+
+            val item = CacheItem(key, value, priority, clock.millis() + ttl.toMillis(), clock.millis())
             map[key] = item
-            priorityQueue.add(item)
+
+            expiryQueue.offer(item)
+            priorityQueue.offer(item)
         }
 
-        fun get(key: String): Int? {
+        override fun get(key: K): V? {
             val item = map[key]
 
-            return if (item == null || item.expiryTime < getSystemTimeForExpiry()) {
+            return if (item == null) {
+                null
+            } else if (item.expiryTime < clock.millis()) {
+                expiryQueue.remove(item)
+                priorityQueue.remove(item)
                 null
             } else {
-                item.lastUsed = System.currentTimeMillis()
+                priorityQueue.remove(item)
+                priorityQueue.add(item.touch(clock.millis()))
                 item.value
             }
         }
 
-        private fun remove(key: String) {
+        private fun remove(key: K) {
             val item = map[key]
 
             item?.let {
-                it.expiryTime = 0L // Mark as expired for next eviction
+                expiryQueue.remove(item)
+                priorityQueue.remove(item)
                 map.remove(key)
             }
         }
 
-        private fun clearCache() {
-            while (priorityQueue.isNotEmpty() && priorityQueue.peek().expiryTime < getSystemTimeForExpiry()) {
-                val item = priorityQueue.poll()
+        private fun checkAndExpireCachedItems() {
+            if (map.size < capacity) return
 
-                if (map.containsKey(item.key) && map[item.key] == item) {
-                    map.remove(item.key)
-                }
+            while (expiryQueue.isNotEmpty() && expiryQueue.peek().expiryTime < clock.millis()) {
+                val item = expiryQueue.poll()
+                map.remove(item.key)
+                priorityQueue.remove(item)
             }
 
+            if (map.size < capacity) return
+
             if (priorityQueue.isEmpty()) return
 
             val item = priorityQueue.poll()
+            map.remove(item.key)
+            expiryQueue.remove(item)
+        }
 
-            if (map.containsKey(item.key) && map[item.key] == item) {
-                map.remove(item.key)
+        private class CacheItem<K, V>(
+            val key: K,
+            val value: V,
+            val priority: Int,
+            val expiryTime: Long,
+            val lastUsed: Long
+        ) {
+            // only compare equality by `key`
+            override fun equals(other: Any?): Boolean {
+                if (this === other) return true
+                if (other !is CacheItem<*, *>) return false
+                if (key == other.key) return true
+                return false
             }
-        }
 
-        private data class CacheItem(
-            val key: String,
-            var value: Int,
-            var priority: Int,
-            var expiryTime: Long,
-        ) : Comparable<CacheItem> {
-            var lastUsed: Long = System.currentTimeMillis()
-
-            override fun compareTo(other: CacheItem) = when {
-                expiryTime != other.expiryTime -> expiryTime.compareTo(other.expiryTime)
-                priority != other.priority -> priority.compareTo(other.priority)
-                else -> lastUsed.compareTo(other.lastUsed)
+            override fun hashCode(): Int {
+                return key.hashCode()
+            }
+
+            fun touch(
+                lastUsed: Long = this.lastUsed
+            ) = CacheItem(key, value, priority, expiryTime, lastUsed)
+
+            override fun toString(): String {
+                return "CacheItem(key='$key', value=$value, priority=$priority, expiryTime=$expiryTime, lastUsed=$lastUsed)"
             }
         }
 
-        // Returns fixed system time in milliseconds
-        private fun getSystemTimeForExpiry() = 1000
+        override fun toString(): String {
+            return "AdvancedLRUCache(capacity=$capacity, clock=$clock, map=$map, priorityQueue=$expiryQueue)"
+        }
     }
 }
 

src/test/kotlin/com/igorwojda/cache/advancedlru/Tests.kt

@@ -0,0 +1,120 @@
+package com.igorwojda.cache.advancedlru
+
+import org.amshove.kluent.shouldBeEqualTo
+import org.junit.jupiter.api.Test
+import java.time.Clock
+import java.time.Duration
+import java.time.Instant
+import java.time.ZoneId
+
+class Tests {
+    // Easily switch between a known solution and Challenge code
+    private val classUnderTest: (capacity: Int, clock: Clock)->LRUCache<String, String> = ::AdvancedLRUCache // or SolutionN::AdvancedLRUCache
+
+    private val testClock = object : Clock() {
+        private var testTime = Instant.now()
+        override fun instant(): Instant {
+           return testTime
+        }
+
+        fun incTime(duration: Duration) {
+            testTime += duration
+        }
+
+        override fun withZone(zone: ZoneId?): Clock = TODO("Not yet implemented")
+        override fun getZone(): ZoneId = systemDefaultZone().zone
+    }
+
+    @Test
+    fun `add and get immediately`() {
+        val cache = classUnderTest(2, testClock)
+
+        cache.put("A", "apple", 0, Duration.ofMinutes(15))
+        cache.get("A") shouldBeEqualTo "apple"
+
+        cache.put("B", "bee", 0, Duration.ofMinutes(15))
+        cache.get("B") shouldBeEqualTo "bee"
+
+        cache.put("C", "cat", 0, Duration.ofMinutes(15))
+        cache.get("C") shouldBeEqualTo "cat"
+
+        cache.put("E", "echo", 0, Duration.ofMinutes(15))
+        cache.get("E") shouldBeEqualTo "echo"
+    }
+
+    @Test
+    fun `evict by priority`() {
+        val cache = classUnderTest(4, testClock)
+
+        // all have the same expiry
+        cache.put("B", "bee", 3, Duration.ofMinutes(15))
+        cache.put("A", "apple", 1, Duration.ofMinutes(15)) // lowest priority
+        cache.put("C", "cat", 5, Duration.ofMinutes(15))
+        cache.put("D", "door", 7, Duration.ofMinutes(15))
+        cache.put("E", "echo", 9, Duration.ofMinutes(15)) // causes eviction
+
+        // This should be null because "A" was evicted due to lower priority and no items have reached expiry time
+        cache.get("A") shouldBeEqualTo null
+        cache.get("B") shouldBeEqualTo "bee"
+        cache.get("C") shouldBeEqualTo "cat"
+        cache.get("D") shouldBeEqualTo "door"
+        cache.get("E") shouldBeEqualTo "echo"
+    }
+
+    @Test
+    fun `evict by priority and last used`() {
+        val cache = classUnderTest(4, testClock)
+
+        // some have the same priority
+        cache.put("C", "cat", 1, Duration.ofMinutes(12)) // priority 1
+        cache.put("A", "apple", 1, Duration.ofMinutes(20)) // priority 1
+        cache.put("B", "bee", 1, Duration.ofMinutes(10)) // priority 1
+        cache.put("D", "door", 7, Duration.ofMinutes(5))
+
+        // but are accessed most recently in a different order...
+        testClock.incTime(Duration.ofSeconds(1))
+        cache.get("A")
+        testClock.incTime(Duration.ofSeconds(1))
+        cache.get("C")
+        testClock.incTime(Duration.ofSeconds(1))
+        cache.get("B")
+        testClock.incTime(Duration.ofSeconds(1))
+
+        cache.put("E", "echo", 9, Duration.ofMinutes(15)) // causes eviction
+
+        // This should be null because "A" was evicted due to lower priority.
+        println(cache)
+        cache.get("A") shouldBeEqualTo null
+        cache.get("B") shouldBeEqualTo "bee"
+        cache.get("C") shouldBeEqualTo "cat"
+        cache.get("D") shouldBeEqualTo "door"
+        cache.get("E") shouldBeEqualTo "echo"
+    }
+
+    @Test
+    fun `evict by expiry time`() {
+        val cache = classUnderTest(100, testClock)
+
+        cache.put("A", "apple", 1, Duration.ofMinutes(15))
+        cache.put("B", "bee", 3, Duration.ofMinutes(20))
+
+        testClock.incTime(Duration.ofMinutes(16))
+
+        // This should be null because "A" was evicted due to expiry.
+        cache.get("A") shouldBeEqualTo null
+
+        testClock.incTime(Duration.ofMinutes(20))
+
+        cache.put("C", "cat", 5, Duration.ofMinutes(15)) // causes eviction
+        cache.put("D", "door", 5, Duration.ofMinutes(15)) // causes eviction
+
+        // this should be null because "B" was evicted due to expiry and later inserts
+        cache.get("B") shouldBeEqualTo null
+
+        testClock.incTime(Duration.ofMinutes(14))
+
+        // still here as clock has not moved past expiry
+        cache.get("C") shouldBeEqualTo "cat"
+        cache.get("D") shouldBeEqualTo "door"
+    }
+}