Merge pull request #1162 from matthughes/semispaceCache

Fix bug in semispace cache that allowed gen1 to contain values in evi…

Author: mpilquist

modules/core/shared/src/main/scala/data/SemispaceCache.scala

@@ -16,12 +16,12 @@ sealed abstract case class SemispaceCache[K, V](gen0: Map[K, V], gen1: Map[K, V]
   assert(gen0.size <= max)
   assert(gen1.size <= max)
 
-  def insert(k: K, v: V): SemispaceCache[K, V] =
-    if (max == 0)        SemispaceCache(gen0, gen1, max, evicted + v)                      // immediately evict
-    else if (gen0.size < max) SemispaceCache(gen0 + (k -> v), gen1, max, evicted - v)      // room in gen0, done!
-    else                 SemispaceCache(Map(k -> v), gen0, max, evicted ++ gen1.values - v)// no room in gen0, slide it down
+  def insert(k: K, v: V): SemispaceCache[K, V] = 
+    if (max == 0)        SemispaceCache(gen0, gen1, max, evicted + v)                       // immediately evict
+    else if (gen0.size < max) SemispaceCache(gen0 + (k -> v), gen1 - k, max, evicted - v)   // room in gen0, remove from gen1, done!
+    else                 SemispaceCache(Map(k -> v), gen0, max, evicted ++ gen1.values - v) // no room in gen0, slide it down
 
-  def lookup(k: K): Option[(SemispaceCache[K, V], V)] =
+  def lookup(k: K): Option[(SemispaceCache[K, V], V)] = 
     gen0.get(k).tupleLeft(this) orElse       // key is in gen0, done!
     gen1.get(k).map(v => (insert(k, v), v))  // key is in gen1, copy to gen0
 
@@ -40,8 +40,14 @@ sealed abstract case class SemispaceCache[K, V](gen0: Map[K, V], gen1: Map[K, V]
 
 object SemispaceCache {
 
-  private def apply[K, V](gen0: Map[K, V], gen1: Map[K, V], max: Int, evicted: EvictionSet[V]): SemispaceCache[K, V] =
-    new SemispaceCache[K, V](gen0, gen1, max, evicted) {}
+  private def apply[K, V](gen0: Map[K, V], gen1: Map[K, V], max: Int, evicted: EvictionSet[V]): SemispaceCache[K, V] = {
+    val r = new SemispaceCache[K, V](gen0, gen1, max, evicted) {}
+    val gen0Intersection: Set[V] = (gen0.values.toSet intersect evicted.toList.toSet)
+    val gen1Intersection: Set[V] = (gen1.values.toSet intersect evicted.toList.toSet)
+    assert(gen0Intersection.isEmpty, s"gen0 has overlapping values in evicted: ${gen0Intersection}")
+    assert(gen1Intersection.isEmpty, s"gen1 has overlapping values in evicted: ${gen1Intersection}")
+    r
+  }
 
   def empty[K, V](max: Int, trackEviction: Boolean): SemispaceCache[K, V] =
     SemispaceCache[K, V](Map.empty, Map.empty, max max 0, if (trackEviction) EvictionSet.empty else new EvictionSet.ZeroEvictionSet)

modules/tests/shared/src/test/scala/PrepareCacheTest.scala

@@ -8,6 +8,7 @@ import skunk.implicits._
 import skunk.codec.numeric.int8
 import skunk.codec.text
 import skunk.codec.boolean
+import cats.syntax.all.*
 
 class PrepareCacheTest extends SkunkTest {
 
@@ -16,7 +17,15 @@ class PrepareCacheTest extends SkunkTest {
   private val pgStatementsCountByStatement = sql"select count(*) from pg_prepared_statements where statement = ${text.text}".query(int8)
   private val pgStatementsCount = sql"select count(*) from pg_prepared_statements".query(int8)
   private val pgStatements = sql"select statement from pg_prepared_statements order by prepare_time".query(text.text)
-    
+  
+  pooledTest("concurrent prepare cache should close evicted prepared statements at end of session", max = 1, parseCacheSize = 2) { p =>
+    List.fill(4)(
+      p.use { s =>
+        s.execute(pgStatementsByName)("foo").void >> s.execute(pgStatementsByStatement)("bar").void >> s.execute(pgStatementsCountByStatement)("baz").void
+      }
+    ).sequence
+  }
+  
   pooledTest("prepare cache should close evicted prepared statements at end of session", max = 1, parseCacheSize = 1) { p =>
     p.use { s =>
       s.execute(pgStatementsByName)("foo").void >>

modules/tests/shared/src/test/scala/data/SemispaceCacheTest.scala

@@ -12,6 +12,36 @@ class SemispaceCacheTest extends ScalaCheckSuite {
 
   val genEmpty: Gen[SemispaceCache[Int, String]] =
     Gen.choose(-1, 10).map(SemispaceCache.empty(_, true))
+  
+  test("eviction should never contain values in gen0/gen1") {
+    val cache = SemispaceCache.empty(2, true).insert("one", 1)
+
+    val i1 = cache.insert("one", 1)
+    // Two doesn't exist; space in gen0, insert
+    val i2 = i1.lookup("two").map(_._1).getOrElse(i1.insert("two", 2))
+    assertEquals(i2.gen0, Map("one" -> 1, "two" -> 2))
+    assertEquals(i2.gen1, Map.empty[String, Int])
+    assertEquals(i2.evicted.toList, Nil)
+    
+    // Three doesn't exist, hit max; slide gen0 -> gen1 and add to gen0
+    val i3 = i2.lookup("three").map(_._1).getOrElse(i2.insert("three", 3))
+    assertEquals(i3.gen0, Map("three" -> 3))
+    assertEquals(i3.gen1, Map("one" -> 1, "two" -> 2))
+    assertEquals(i3.evicted.toList, Nil)
+    
+    // One exists in gen1; pull up to gen0 and REMOVE from gen1
+    val i4 = i3.lookup("one").map(_._1).getOrElse(i3.insert("one", 1))
+    assertEquals(i4.gen0, Map("one" -> 1, "three" -> 3))
+    assertEquals(i4.gen1, Map("two" -> 2))
+    assertEquals(i4.evicted.toList, Nil)
+    
+    // Four doesn't exist; gen0 is full so push to gen1
+    // insert four to gen0 and evict gen1
+    val i5 = i4.lookup("four").map(_._1).getOrElse(i4.insert("four", 4))
+    assertEquals(i5.gen0, Map("four" -> 4))
+    assertEquals(i5.gen1, Map("one" -> 1, "three" -> 3))
+    assertEquals(i5.evicted.toList, List(2))
+  }
 
   test("insert on empty cache results in eviction") {
     val cache = SemispaceCache.empty(0, true).insert("one", 1)
@@ -73,7 +103,7 @@ class SemispaceCacheTest extends ScalaCheckSuite {
       val max = c.max
 
       // Load up the cache such that it overflows by 1
-      val cʹ = (0 to max).foldLeft(c) { case (c, n) => c.insert(n, "x") }
+      val cʹ = (0 to max).foldLeft(c) { case (c, n) => c.insert(n, n.toString) }
       assertEquals(cʹ.gen0.size, 1 min max)
       assertEquals(cʹ.gen1.size, max)
 
@@ -82,7 +112,9 @@ class SemispaceCacheTest extends ScalaCheckSuite {
         case None => assertEquals(max, 0)
         case Some((cʹʹ, _)) =>
           assertEquals(cʹʹ.gen0.size, 2 min max)
-          assertEquals(cʹʹ.gen1.size, max)
+          // When we promote 0 to gen0, we remove it from gen1
+          assertEquals(cʹʹ.gen1.size, max-1 max 1)
+          assertEquals(cʹʹ.evicted.toList, Nil)
       }
 
     }