Make lazy initialization of static Val.Obj thread-safe (#136)

szeiger · web-flow · commit 541b00b005cf · 2021-11-30T18:26:16.000+01:00
Static Val.Obj instances are created by the optimizer and will end up in the parse cache. If the cache is shared by multiple threads, initialization must be sufficiently safe, i.e. computing a value multiple times in race conditions is allowed (and cheaper than ensuring that it doesn't happen), but object must never get into an invalid intermediate state.
diff --git a/bench/src/main/scala/sjsonnet/MainBenchmark.scala b/bench/src/main/scala/sjsonnet/MainBenchmark.scala
@@ -31,6 +31,12 @@ object MainBenchmark {
     interp.interpret0(interp.resolver.read(path).get, path, renderer).getOrElse(???)
     (parseCache.keySet.toIndexedSeq, interp.evaluator)
   }
+
+  def createDummyOut = new PrintStream(new OutputStream {
+    def write(b: Int): Unit = ()
+    override def write(b: Array[Byte]): Unit = ()
+    override def write(b: Array[Byte], off: Int, len: Int): Unit = ()
+  })
 }
 
 @BenchmarkMode(Array(Mode.AverageTime))
@@ -42,11 +48,7 @@ object MainBenchmark {
 @State(Scope.Benchmark)
 class MainBenchmark {
 
-  val dummyOut = new PrintStream(new OutputStream {
-    def write(b: Int): Unit = ()
-    override def write(b: Array[Byte]): Unit = ()
-    override def write(b: Array[Byte], off: Int, len: Int): Unit = ()
-  })
+  val dummyOut = MainBenchmark.createDummyOut
 
   @Benchmark
   def main(bh: Blackhole): Unit = {
diff --git a/bench/src/main/scala/sjsonnet/MultiThreadedBenchmark.scala b/bench/src/main/scala/sjsonnet/MultiThreadedBenchmark.scala
@@ -0,0 +1,56 @@
+package sjsonnet
+
+import java.util.concurrent.{ExecutorService, Executors, TimeUnit}
+
+import org.openjdk.jmh.annotations._
+import org.openjdk.jmh.infra._
+
+import scala.collection.mutable
+
+@BenchmarkMode(Array(Mode.AverageTime))
+@Fork(4)
+@Threads(1)
+@Warmup(iterations = 30)
+@Measurement(iterations = 40)
+@OutputTimeUnit(TimeUnit.MILLISECONDS)
+@State(Scope.Benchmark)
+class MultiThreadedBenchmark {
+
+  val threads = 8
+
+  @Benchmark
+  def main(bh: Blackhole): Unit = {
+    val cache: ParseCache = new ParseCache {
+      val map = new mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]]()
+      override def getOrElseUpdate(key: (Path, String), defaultValue: => Either[Error, (Expr, FileScope)]): Either[Error, (Expr, FileScope)] = {
+        var v = map.synchronized(map.getOrElse(key, null))
+        if(v == null) {
+          v = defaultValue
+          map.synchronized(map.put(key, v))
+        }
+        v
+      }
+    }
+
+    val pool: ExecutorService = Executors.newFixedThreadPool(threads)
+    val futs = (1 to threads).map { _ =>
+      pool.submit { (() =>
+        if(SjsonnetMain.main0(
+          MainBenchmark.mainArgs,
+          cache, // new DefaultParseCache
+          System.in,
+          MainBenchmark.createDummyOut,
+          System.err,
+          os.pwd,
+          None
+        ) != 0) throw new Exception): Runnable
+      }
+    }
+    var err: Throwable = null
+    bh.consume(futs.map { f =>
+      try f.get() catch { case e: Throwable => err = e }
+    })
+    pool.shutdown()
+    if(err != null) throw err
+  }
+}
diff --git a/sjsonnet/src/sjsonnet/Val.scala b/sjsonnet/src/sjsonnet/Val.scala
@@ -155,10 +155,12 @@ object Val{
 
     private[this] def getValue0: util.LinkedHashMap[String, Obj.Member] = {
       if(value0 == null) {
-        value0 = new java.util.LinkedHashMap[String, Val.Obj.Member]
+        val value0 = new java.util.LinkedHashMap[String, Val.Obj.Member]
         allKeys.forEach { (k, _) =>
           value0.put(k, new Val.Obj.ConstMember(false, Visibility.Normal, valueCache(k)))
         }
+        // Only assign to field after initialization is complete to allow unsynchronized multi-threaded use:
+        this.value0 = value0
       }
       value0
     }

Original file line number	Diff line number	Diff line change
`@@ -155,10 +155,12 @@ object Val{`
`155`	`155`
`156`	`156`	`private[this] def getValue0: util.LinkedHashMap[String, Obj.Member] = {`
`157`	`157`	`if(value0 == null) {`
`158`		`- value0 = new java.util.LinkedHashMap[String, Val.Obj.Member]`
	`158`	`+ val value0 = new java.util.LinkedHashMap[String, Val.Obj.Member]`
`159`	`159`	`allKeys.forEach { (k, _) =>`
`160`	`160`	`value0.put(k, new Val.Obj.ConstMember(false, Visibility.Normal, valueCache(k)))`
`161`	`161`	`}`
	`162`	`+ // Only assign to field after initialization is complete to allow unsynchronized multi-threaded use:`
	`163`	`+ this.value0 = value0`
`162`	`164`	`}`
`163`	`165`	`value0`
`164`	`166`	`}`