allow to load packages eagerly, and do not compile twice with --scenario (#1248)

fixes #1238 and should help with #1230.

daml-lf/engine/src/main/scala/com/digitalasset/daml/lf/engine/Engine.scala

@@ -3,9 +3,10 @@
 
 package com.digitalasset.daml.lf.engine
 
+import com.digitalasset.daml.lf.CompiledPackages
 import com.digitalasset.daml.lf.command._
 import com.digitalasset.daml.lf.data._
-import com.digitalasset.daml.lf.data.Ref.Party
+import com.digitalasset.daml.lf.data.Ref.{PackageId, Party}
 import com.digitalasset.daml.lf.lfpackage.Ast._
 import com.digitalasset.daml.lf.speedy.Compiler
 import com.digitalasset.daml.lf.speedy.Pretty
@@ -47,8 +48,8 @@ import scala.annotation.tailrec
   * This class is thread safe.
   */
 final class Engine {
-  private[this] val compiledPackages = ConcurrentCompiledPackages()
-  private[this] val commandTranslation = CommandPreprocessor(compiledPackages)
+  private[this] val _compiledPackages = ConcurrentCompiledPackages()
+  private[this] val _commandTranslation = CommandPreprocessor(_compiledPackages)
 
   /**
     * Executes commands `cmds` and returns one of the following:
@@ -70,7 +71,7 @@ final class Engine {
     * </ul>
     */
   def submit(cmds: Commands): Result[Transaction.Transaction] = {
-    commandTranslation
+    _commandTranslation
       .preprocessCommands(cmds)
       .flatMap(interpret(_, cmds.ledgerEffectiveTime))
   }
@@ -94,7 +95,7 @@ final class Engine {
       ledgerEffectiveTime: Time.Timestamp
   ): Result[Transaction.Transaction] = {
     for {
-      commands <- Result.sequence(ImmArray(nodes).map(translateNode(commandTranslation)))
+      commands <- Result.sequence(ImmArray(nodes).map(translateNode(_commandTranslation)))
       result <- interpret(commands, ledgerEffectiveTime)
     } yield result
   }
@@ -116,7 +117,7 @@ final class Engine {
   ): Result[Unit] = {
     //reinterpret
     for {
-      commands <- translateTransactionRoots(commandTranslation, tx)
+      commands <- translateTransactionRoots(_commandTranslation, tx)
       rtx <- interpret(commands.map(_._2), ledgerEffectiveTime)
       validationResult <- if (tx isReplayedBy rtx) {
         ResultDone(())
@@ -167,7 +168,7 @@ final class Engine {
       }
 
       val comps =
-        translateTransactionRoots(commandTranslation, tx)
+        translateTransactionRoots(_commandTranslation, tx)
           .flatMap(
             s =>
               if (s.isEmpty)
@@ -317,7 +318,7 @@ final class Engine {
   ): Result[Transaction.Transaction] = {
 
     val machine =
-      Machine.build(Compiler(compiledPackages.packages).compile(command), compiledPackages)
+      Machine.build(Compiler(_compiledPackages.packages).compile(command), _compiledPackages)
     machine.ptx = machine.ptx.copy(nextNodeId = nodeId)
     interpretLoop(machine, time)
   }
@@ -326,7 +327,7 @@ final class Engine {
       expr: Expr,
       time: Time.Timestamp): Result[Transaction.Transaction] = {
     val machine =
-      Machine.build(Compiler(compiledPackages.packages).compile(expr), compiledPackages)
+      Machine.build(Compiler(_compiledPackages.packages).compile(expr), _compiledPackages)
 
     interpretLoop(machine, time)
   }
@@ -335,8 +336,8 @@ final class Engine {
       commands: ImmArray[(Type, SpeedyCommand)],
       time: Time.Timestamp): Result[Transaction.Transaction] = {
     val machine = Machine.build(
-      Compiler(compiledPackages.packages).compile(commands.map(_._2)),
-      compiledPackages)
+      Compiler(_compiledPackages.packages).compile(commands.map(_._2)),
+      _compiledPackages)
 
     interpretLoop(machine, time)
   }
@@ -363,9 +364,9 @@ final class Engine {
           return Result.needPackage(
             ref.packageId,
             pkg => {
-              compiledPackages.addPackage(ref.packageId, pkg).flatMap {
+              _compiledPackages.addPackage(ref.packageId, pkg).flatMap {
                 case _ =>
-                  callback(compiledPackages)
+                  callback(_compiledPackages)
                   interpretLoop(machine, time)
               }
             }
@@ -418,7 +419,23 @@ final class Engine {
     }
   }
 
-  def clearPackages(): Unit = compiledPackages.clear()
+  def clearPackages(): Unit = _compiledPackages.clear()
+
+  /** Note: it's important we return a [[com.digitalasset.daml.lf.CompiledPackages]],
+    * and not a [[ConcurrentCompiledPackages]], otherwise people would be able
+    * to modify them.
+    */
+  def compiledPackages(): CompiledPackages = _compiledPackages
+
+  /** This function can be used to give a package to the engine pre-emptively,
+    * rather than having the engine to ask about it through
+    * [[ResultNeedPackage]].
+    *
+    * Returns a [[Result]] because the package might need another package to
+    * be loaded.
+    */
+  def preloadPackage(pkgId: PackageId, pkg: Package): Result[Unit] =
+    _compiledPackages.addPackage(pkgId, pkg)
 }
 
 object Engine {

docs/source/support/release-notes.rst

@@ -21,11 +21,22 @@ Java Bindings
 
 - **Bots**: A class called LedgerTestView was added to make bot unit testing possible
 
-Ledger
-~~~~~~
+DAML
+~~~~
 
 - **BREAKING CHANGE - Contract Keys**: Before, maintainers were incorrectly not checked to be a subset of the signatories, now they are. See `issue #1123 <https://github.com/digital-asset/daml/issues/1123>`__
 
+Sandbox
+~~~~~~~
+
+- When loading a scenario with ``--scenario``, the sandbox no longer compiles packages twice, see
+  `issue #1238 <https://github.com/digital-asset/daml/issues/1238>`__.
+- When starting the sandbox, you can now choose to have it load all the ``.dar`` packages immediately
+  with the ``--eager-package-loading`` flag. The default behavior is to load the packages only when
+  a command requires them, which causes a delay for the first command that requires a yet-to-be-compiled
+  package.
+  See `issue #1230 <https://github.com/digital-asset/daml/issues/1230>`__.
+
 .. _release-0-12-18:
 
 0.12.18 - 2019-05-20

ledger/ledger-api-integration-tests/src/test/lib/scala/com/digitalasset/platform/PlatformApplications.scala

@@ -106,7 +106,8 @@ object PlatformApplications {
       scenario = None,
       tlsConfig = None,
       ledgerIdMode = config.ledgerId,
-      jdbcUrl = jdbcUrl
+      jdbcUrl = jdbcUrl,
+      eagerPackageLoading = false,
     )
   }
 }

ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/SandboxServer.scala

@@ -73,14 +73,31 @@ object SandboxServer {
 
   // if requested, initialize the ledger state with the given scenario
   private def createInitialState(config: SandboxConfig, context: SandboxContext)
-    : (ActiveContractsInMemory, ImmArray[LedgerEntryWithLedgerEndIncrement], Option[Instant]) =
+    : (ActiveContractsInMemory, ImmArray[LedgerEntryWithLedgerEndIncrement], Option[Instant]) = {
+    // [[ScenarioLoader]] needs all the packages to be already compiled --
+    // make sure that that's the case
+    if (config.eagerPackageLoading || config.scenario.nonEmpty) {
+      for ((pkgId, pkg) <- context.packageContainer.packages) {
+        engine
+          .preloadPackage(pkgId, pkg)
+          .consume(
+            { _ =>
+              sys.error("Unexpected request of contract")
+            },
+            context.packageContainer.packages.get, { _ =>
+              sys.error("Unexpected request of contract key")
+            }
+          )
+      }
+    }
     config.scenario match {
       case None => (ActiveContractsInMemory.empty, ImmArray.empty, None)
       case Some(scenario) =>
         val (acs, records, ledgerTime) =
-          ScenarioLoader.fromScenario(context.packageContainer, scenario)
+          ScenarioLoader.fromScenario(context.packageContainer, engine.compiledPackages(), scenario)
         (acs, records, Some(ledgerTime))
     }
+  }
 }
 
 class SandboxServer(actorSystemName: String, config: => SandboxConfig) extends AutoCloseable {

ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/cli/Cli.scala

@@ -69,7 +69,8 @@ object Cli {
       .text(
         "If set, the sandbox will execute the given scenario on startup and store all the contracts created by it. " +
           "Note that when using --postgres-backend the scenario will be ran only if starting from a fresh database, _not_ when resuming from an existing one. " +
-          "Two identifier formats are supported: Module.Name:Entity.Name (preferred) and Module.Name.Entity.Name (deprecated, will print a warning when used).")
+          "Two identifier formats are supported: Module.Name:Entity.Name (preferred) and Module.Name.Entity.Name (deprecated, will print a warning when used)." +
+          "Also note that instructing the sandbox to load a scenario will have the side effect of loading _all_ the .dar files provided eagerly (see --eager-package-loading).")
 
     arg[File]("<archive>...")
       .unbounded()
@@ -117,6 +118,11 @@ object Cli {
       .action((id, c) => c.copy(ledgerIdMode = LedgerIdMode.Static(id)))
       .text("Sandbox ledger ID. If missing, a random unique ledger ID will be used. Only useful with persistent stores.")
 
+    opt[Unit]("eager-package-loading")
+        .optional()
+        .text("Whether to load all the packages in the .dar files provided eagerly, rather than when needed as the commands come.")
+        .action( (_, config) => config.copy(eagerPackageLoading = true))
+
     help("help").text("Print the usage text")
 
     checkConfig(c => {

ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/config/SandboxConfig.scala

@@ -30,7 +30,8 @@ final case class SandboxConfig(
     tlsConfig: Option[TlsConfiguration],
     scenario: Option[String],
     ledgerIdMode: LedgerIdMode,
-    jdbcUrl: Option[String]
+    jdbcUrl: Option[String],
+    eagerPackageLoading: Boolean
 )
 
 final case class CommandConfiguration(
@@ -57,7 +58,8 @@ object SandboxConfig {
       tlsConfig = None,
       scenario = None,
       ledgerIdMode = LedgerIdMode.Dynamic(),
-      jdbcUrl = None
+      jdbcUrl = None,
+      eagerPackageLoading = false
     )
 
   lazy val defaultCommandConfig =

ledger/sandbox/src/main/scala/com/digitalasset/platform/sandbox/stores/ledger/ScenarioLoader.scala

@@ -5,7 +5,7 @@ package com.digitalasset.platform.sandbox.stores.ledger
 
 import java.time.Instant
 
-import com.digitalasset.daml.lf.PureCompiledPackages
+import com.digitalasset.daml.lf.CompiledPackages
 import com.digitalasset.daml.lf.data._
 import com.digitalasset.daml.lf.engine.DeprecatedIdentifier
 import com.digitalasset.daml.lf.lfpackage.Ast
@@ -16,8 +16,8 @@ import com.digitalasset.daml.lf.value.Value.AbsoluteContractId
 import com.digitalasset.platform.sandbox.config.DamlPackageContainer
 import com.digitalasset.platform.sandbox.stores.ActiveContractsInMemory
 import org.slf4j.LoggerFactory
-import com.digitalasset.daml.lf.transaction.GenTransaction
 import com.digitalasset.daml.lf.types.Ledger.TransactionId
+import com.digitalasset.daml.lf.transaction.GenTransaction
 import com.digitalasset.platform.sandbox.stores.ledger.LedgerEntry.Transaction
 
 import scala.collection.breakOut
@@ -42,9 +42,22 @@ object ScenarioLoader {
     */
   case class LedgerEntryWithLedgerEndIncrement(entry: LedgerEntry, increment: Long)
 
-  def fromScenario(packages: DamlPackageContainer, scenario: String)
+  /**
+    * @param packages All the packages where we're going to look for the scenario definition.
+    * @param compiledPackages The above packages, compiled. Note that we require _all_
+    *                         packages to be compiled -- this is just for ease of implementation
+    *                         and might be revised in the future.
+    * @param scenario The scenario to run. The scenario will be looked in all the packages above
+    *                 trying both with the old and new identifier syntax (`Foo.Bar.Baz` vs `Foo.Bar:Baz`).
+    *                 This function will crash if the scenario is not found or if there are multiple
+    *                 matching scenarios.
+    */
+  def fromScenario(
+      packages: DamlPackageContainer,
+      compiledPackages: CompiledPackages,
+      scenario: String)
     : (ActiveContractsInMemory, ImmArray[LedgerEntryWithLedgerEndIncrement], Instant) = {
-    val (scenarioLedger, scenarioRef) = buildScenarioLedger(packages, scenario)
+    val (scenarioLedger, scenarioRef) = buildScenarioLedger(packages, compiledPackages, scenario)
     // we store the tx id since later we need to recover how much to bump the
     // ledger end by, and here the transaction id _is_ the ledger end.
     val ledgerEntries =
@@ -78,13 +91,13 @@ object ScenarioLoader {
 
   private def buildScenarioLedger(
       packages: DamlPackageContainer,
+      compiledPackages: CompiledPackages,
       scenario: String): (L.Ledger, Ref.DefinitionRef) = {
     val scenarioQualName = getScenarioQualifiedName(packages, scenario)
     val candidateScenarios: List[(Ref.DefinitionRef, Definition)] =
       getCandidateScenarios(packages, scenarioQualName)
     val (scenarioRef, scenarioDef) = identifyScenario(packages, scenario, candidateScenarios)
     val scenarioExpr = getScenarioExpr(scenarioRef, scenarioDef)
-    val compiledPackages = getCompiledPackages(packages)
     val speedyMachine = getSpeedyMachine(scenarioExpr, compiledPackages)
     val scenarioLedger = getScenarioLedger(scenarioRef, speedyMachine)
     (scenarioLedger, scenarioRef)
@@ -102,20 +115,13 @@ object ScenarioLoader {
 
   private def getSpeedyMachine(
       scenarioExpr: Ast.Expr,
-      compiledPackages: PureCompiledPackages): Speedy.Machine = {
+      compiledPackages: CompiledPackages): Speedy.Machine = {
     Speedy.Machine.newBuilder(compiledPackages) match {
       case Left(err) => throw new RuntimeException(s"Could not build speedy machine: $err")
       case Right(build) => build(scenarioExpr)
     }
   }
 
-  private def getCompiledPackages(packages: DamlPackageContainer): PureCompiledPackages = {
-    PureCompiledPackages(packages.packages) match {
-      case Left(err) => throw new RuntimeException(s"Could not compile packages: $err")
-      case Right(x) => x
-    }
-  }
-
   private def getScenarioExpr(scenarioRef: Ref.DefinitionRef, scenarioDef: Definition): Ast.Expr = {
     scenarioDef match {
       case DValue(_, _, body, _) => body

ledger/sandbox/src/test/lib/scala/com/digitalasset/platform/sandbox/cli/CliSpec.scala

@@ -113,6 +113,9 @@ class CliSpec extends WordSpec with Matchers {
       checkOption(Array(s"--sql-backend-jdbcurl", jdbcUrl), _.copy(jdbcUrl = Some(jdbcUrl)))
     }
 
+    "parse the eager package loading flag when given" in {
+      checkOption(Array("--eager-package-loading"), _.copy(eagerPackageLoading = true))
+    }
   }
 
 }