wip - address feedback, todo - await on zinc callback in backend

Author: bishabosha

compiler/src/dotty/tools/backend/jvm/ClassfileWriters.scala

@@ -20,7 +20,11 @@ import dotty.tools.io.JarArchive
 
 import scala.language.unsafeNulls
 
-
+/** !!! This file is now copied in `dotty.tools.io.FileWriters` in a more general way that does not rely upon
+ * `PostProcessorFrontendAccess`, this should probably be changed to wrap that class instead.
+ *
+ * Until then, any changes to this file should be copied to `dotty.tools.io.FileWriters` as well.
+ */
 class ClassfileWriters(frontendAccess: PostProcessorFrontendAccess) {
   type NullableFile =  AbstractFile | Null
   import frontendAccess.{compilerSettings, backendReporting}

compiler/src/dotty/tools/backend/jvm/GenBCode.scala

@@ -12,7 +12,7 @@ import scala.collection.mutable
 import scala.compiletime.uninitialized
 import java.util.concurrent.TimeoutException
 
-import scala.concurrent.duration.given
+import scala.concurrent.duration.Duration
 import scala.concurrent.Await
 
 class GenBCode extends Phase { self =>
@@ -95,19 +95,9 @@ class GenBCode extends Phase { self =>
       val result = super.runOn(units)
       generatedClassHandler.complete()
       for holder <- ctx.asyncTastyPromise do
-        try
-          val asyncState = Await.result(holder.promise.future, 5.seconds)
-          for reporter <- asyncState.pending do
-            reporter.relayReports(frontendAccess.backendReporting)
-        catch
-          case _: TimeoutException =>
-            report.error(
-              """Timeout (5s) in backend while waiting for async writing of TASTy files to -Yearly-tasty-output,
-                |  this may be a bug in the compiler.
-                |
-                |Alternatively consider turning off pipelining for this project.""".stripMargin
-            )
-      end for
+        val asyncState = Await.result(holder.promise.future, Duration.Inf) // TODO: add another optional promise for Zinc callback, and await both.
+        for reporter <- asyncState.pending do
+          reporter.relayReports(frontendAccess.backendReporting)
       result
     finally
       // frontendAccess and postProcessor are created lazilly, clean them up only if they were initialized

compiler/src/dotty/tools/dotc/transform/Pickler.scala

@@ -26,7 +26,7 @@ import scala.concurrent.Promise
 import dotty.tools.dotc.transform.Pickler.writeSigFilesAsync
 
 import scala.util.chaining.given
-import dotty.tools.io.FileWriters.BufferingDelayedReporting
+import dotty.tools.io.FileWriters.{EagerReporter, BufferingReporter}
 
 object Pickler {
   val name: String = "pickler"
@@ -39,7 +39,7 @@ object Pickler {
   inline val ParallelPickling = true
 
   class AsyncTastyHolder(val earlyOut: AbstractFile, val promise: Promise[AsyncTastyState])
-  class AsyncTastyState(val hasErrors: Boolean, val pending: Option[BufferingDelayedReporting])
+  class AsyncTastyState(val hasErrors: Boolean, val pending: Option[BufferingReporter])
 
   // Why we only write to early output in the first run?
   // ===================================================
@@ -71,24 +71,35 @@ object Pickler {
       tasks: List[(String, Array[Byte])],
       writer: EarlyFileWriter,
       promise: Promise[AsyncTastyState])(using ctx: ReadOnlyContext): Unit = {
+
+    def logException(ex: Exception): Unit =
+      ctx.reporter.error({
+        val trace = ex.getStackTrace().nn.mkString("\n  ")
+        em"Error writing TASTy to early output:${ex}\n  $trace"
+      })
+
     try
-      for (internalName, pickled) <- tasks do
-        val _ = writer.writeTasty(internalName, pickled)
-    finally
       try
-        writer.close()
+        for (internalName, pickled) <- tasks do
+          val _ = writer.writeTasty(internalName, pickled)
+      catch
+        case ex: Exception => logException(ex)
       finally
-        promise.success(
-          AsyncTastyState(
-            hasErrors = ctx.reporter.hasErrors,
-            pending = (
-              ctx.reporter match
-                case buffered: BufferingDelayedReporting => Some(buffered)
-                case _ => None
-            )
+        writer.close()
+      end try
+    catch
+      case ex: Exception => logException(ex)
+    finally
+      promise.success(
+        AsyncTastyState(
+          hasErrors = ctx.reporter.hasErrors,
+          pending = (
+            ctx.reporter match
+              case buffered: BufferingReporter => Some(buffered)
+              case _: EagerReporter => None // already reported
           )
         )
-      end try
+      )
     end try
   }
 
@@ -115,7 +126,8 @@ class Pickler extends Phase {
 
   override def description: String = Pickler.description
 
-  // No need to repickle trees coming from TASTY
+  // No need to repickle trees coming from TASTY, however in the case that we need to write TASTy to early-output,
+  // then we need to run this phase to send the tasty from compilation units to the early-output.
   override def isRunnable(using Context): Boolean =
     super.isRunnable && (!ctx.settings.fromTasty.value || doAsyncTasty)
 
@@ -289,19 +301,19 @@ class Pickler extends Phase {
   }
 
   override def runOn(units: List[CompilationUnit])(using Context): List[CompilationUnit] = {
-    val isConcurrent = useExecutor
+    val useExecutor = this.useExecutor
 
     val writeTask: Option[() => Unit] = ctx.asyncTastyPromise.map: holder =>
       () =>
-        given ReadOnlyContext = if isConcurrent then ReadOnlyContext.buffered else ReadOnlyContext.eager
+        given ReadOnlyContext = if useExecutor then ReadOnlyContext.buffered else ReadOnlyContext.eager
         val writer = Pickler.EarlyFileWriter(holder.earlyOut)
         writeSigFilesAsync(serialized.result(), writer, holder.promise)
 
     def runPhase(writeCB: (doWrite: () => Unit) => Unit) =
       super.runOn(units).tap(_ => writeTask.foreach(writeCB))
 
     val result =
-      if isConcurrent then
+      if useExecutor then
         executor.start()
         try
           runPhase: doWrite =>

compiler/src/dotty/tools/io/FileWriters.scala

@@ -36,15 +36,21 @@ import dotty.tools.dotc.report
 
 import dotty.tools.backend.jvm.PostProcessorFrontendAccess.BackendReporting
 import scala.annotation.constructorOnly
-
-/** Copied from `dotty.tools.backend.jvm.ClassfileWriters` but no `PostProcessorFrontendAccess` needed */
+import java.util.concurrent.atomic.AtomicReference
+import java.util.concurrent.atomic.AtomicBoolean
+
+/** !!!Copied from `dotty.tools.backend.jvm.ClassfileWriters` but no `PostProcessorFrontendAccess` needed.
+ * this should probably be changed to wrap that class instead.
+ *
+ * Until then, any changes to this file should be copied to `dotty.tools.backend.jvm.ClassfileWriters` as well.
+ */
 object FileWriters {
   type InternalName = String
   type NullableFile =  AbstractFile | Null
 
   inline def ctx(using ReadOnlyContext): ReadOnlyContext = summon[ReadOnlyContext]
 
-  sealed trait DelayedReporting {
+  sealed trait DelayedReporter {
     def hasErrors: Boolean
     def error(message: Context ?=> Message, position: SourcePosition): Unit
     def warning(message: Context ?=> Message, position: SourcePosition): Unit
@@ -54,7 +60,7 @@ object FileWriters {
     def warning(message: Context ?=> Message): Unit = warning(message, NoSourcePosition)
   }
 
-  final class EagerDelayedReporting(using captured: Context) extends DelayedReporting:
+  final class EagerReporter(using captured: Context) extends DelayedReporter:
     private var _hasErrors = false
 
     def hasErrors: Boolean = _hasErrors
@@ -68,62 +74,86 @@ object FileWriters {
 
     def log(message: String): Unit = report.echo(message)
 
-  final class BufferingDelayedReporting extends DelayedReporting {
+  final class BufferingReporter extends DelayedReporter {
     // We optimise access to the buffered reports for the common case - that there are no warning/errors to report
     // We could use a listBuffer etc - but that would be extra allocation in the common case
-    // Note - all access is externally synchronized, as this allow the reports to be generated in on thread and
-    // consumed in another
-    private var bufferedReports = List.empty[Report]
-    private var _hasErrors = false
+    // buffered logs are updated atomically.
+
+    private val _bufferedReports = AtomicReference(List.empty[Report])
+    private val _hasErrors = AtomicBoolean(false)
+
     enum Report(val relay: Context ?=> BackendReporting => Unit):
       case Error(message: Context => Message, position: SourcePosition) extends Report(ctx ?=> _.error(message(ctx), position))
       case Warning(message: Context => Message, position: SourcePosition) extends Report(ctx ?=> _.warning(message(ctx), position))
       case Log(message: String) extends Report(_.log(message))
 
-    def hasErrors: Boolean = synchronized:
-      _hasErrors
+    /** Atomically record that an error occurred */
+    private def recordError(): Unit =
+      while
+        val old = _hasErrors.get
+        !old && !_hasErrors.compareAndSet(old, true)
+      do ()
+
+    /** Atomically add a report to the log */
+    private def recordReport(report: Report): Unit =
+      while
+        val old = _bufferedReports.get
+        !_bufferedReports.compareAndSet(old, report :: old)
+      do ()
+
+    /** atomically extract and clear the buffered reports */
+    private def resetReports(): List[Report] =
+      while
+        val old = _bufferedReports.get
+        if _bufferedReports.compareAndSet(old, Nil) then
+          return old
+        else
+          true
+      do ()
+      throw new AssertionError("Unreachable")
+
+    def hasErrors: Boolean = _hasErrors.get()
 
-    def error(message: Context ?=> Message, position: SourcePosition): Unit = synchronized:
-      bufferedReports ::= Report.Error({case given Context => message}, position)
-      _hasErrors = true
+    def error(message: Context ?=> Message, position: SourcePosition): Unit =
+      recordReport(Report.Error({case given Context => message}, position))
+      recordError()
 
-    def warning(message: Context ?=> Message, position: SourcePosition): Unit = synchronized:
-      bufferedReports ::= Report.Warning({case given Context => message}, position)
+    def warning(message: Context ?=> Message, position: SourcePosition): Unit =
+      recordReport(Report.Warning({case given Context => message}, position))
 
-    def log(message: String): Unit = synchronized:
-      bufferedReports ::= Report.Log(message)
+    def log(message: String): Unit =
+      recordReport(Report.Log(message))
 
     /** Should only be called from main compiler thread. */
-    def relayReports(toReporting: BackendReporting)(using Context): Unit = synchronized:
-      if bufferedReports.nonEmpty then
-        bufferedReports.reverse.foreach(_.relay(toReporting))
-        bufferedReports = Nil
+    def relayReports(toReporting: BackendReporting)(using Context): Unit =
+      val reports = resetReports()
+      if reports.nonEmpty then
+        reports.reverse.foreach(_.relay(toReporting))
   }
 
-  trait ReadSettings:
+  trait ReadOnlySettings:
     def jarCompressionLevel: Int
     def debug: Boolean
 
   trait ReadOnlyContext:
-
-    val settings: ReadSettings
-    val reporter: DelayedReporting
+    val settings: ReadOnlySettings
+    val reporter: DelayedReporter
 
   trait BufferedReadOnlyContext extends ReadOnlyContext:
-    val reporter: BufferingDelayedReporting
+    val reporter: BufferingReporter
 
   object ReadOnlyContext:
-    def readSettings(using ctx: Context): ReadSettings = new:
+    def readSettings(using ctx: Context): ReadOnlySettings = new:
       val jarCompressionLevel = ctx.settings.YjarCompressionLevel.value
       val debug = ctx.settings.Ydebug.value
 
     def buffered(using Context): BufferedReadOnlyContext = new:
       val settings = readSettings
-      val reporter = BufferingDelayedReporting()
+      val reporter = BufferingReporter()
 
     def eager(using Context): ReadOnlyContext = new:
       val settings = readSettings
-      val reporter = EagerDelayedReporting()
+      val reporter = EagerReporter()
 
   /**
    * The interface to writing classfiles. GeneratedClassHandler calls these methods to generate the