From 861724e4fd011fe906f7007d2e245563745d3156 Mon Sep 17 00:00:00 2001 From: Remy Date: Thu, 7 May 2020 11:44:19 +0200 Subject: [PATCH] Engine: cleanup SEVal (#5859) CHANGELOG_BEGIN CHANGELOG_END --- .../daml/lf/ReplServiceMain.scala | 3 +-- .../daml/lf/speedy/Compiler.scala | 4 ++-- .../digitalasset/daml/lf/speedy/Pretty.scala | 2 +- .../digitalasset/daml/lf/speedy/SExpr.scala | 22 ++++++++++++++++--- .../digitalasset/daml/lf/speedy/Speedy.scala | 4 ++-- .../daml/lf/engine/script/Runner.scala | 2 +- 6 files changed, 26 insertions(+), 11 deletions(-) diff --git a/compiler/repl-service/server/src/main/scala/com/digitalasset/daml/lf/ReplServiceMain.scala b/compiler/repl-service/server/src/main/scala/com/digitalasset/daml/lf/ReplServiceMain.scala index ef19f8c52ea..16c8631c84c 100644 --- a/compiler/repl-service/server/src/main/scala/com/digitalasset/daml/lf/ReplServiceMain.scala +++ b/compiler/repl-service/server/src/main/scala/com/digitalasset/daml/lf/ReplServiceMain.scala @@ -201,8 +201,7 @@ class ReplService( var scriptExpr: SExpr = SEVal( LfDefRef( - Identifier(homePackageId, QualifiedName(mod.name, DottedName.assertFromString("expr")))), - None) + Identifier(homePackageId, QualifiedName(mod.name, DottedName.assertFromString("expr"))))) if (!results.isEmpty) { scriptExpr = SEApp(scriptExpr, results.map(SEValue(_)).toArray) } diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Compiler.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Compiler.scala index db7342e087f..0f7f7688e3b 100644 --- a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Compiler.scala +++ b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Compiler.scala @@ -209,7 +209,7 @@ private[lf] final case class Compiler(packages: PackageId PartialFunction Packag private def translate(expr0: Expr): SExpr = expr0 match { case EVar(name) => SEVar(env.lookUpExprVar(name)) - case EVal(ref) => SEVal(LfDefRef(ref), None) + case EVal(ref) => SEVal(LfDefRef(ref)) case EBuiltin(bf) => bf match { case BFoldl => SEBuiltinRecursiveDefinition.FoldL @@ -1236,7 +1236,7 @@ private[lf] final case class Compiler(packages: PackageId PartialFunction Packag case Some(actors) => SEApp(SEBuiltin(SBSome), Array(actors)) } SEApp( - SEVal(ChoiceDefRef(tmplId, choiceId), None), + SEVal(ChoiceDefRef(tmplId, choiceId)), Array(SEValue.bool(byKey), actors, contractId, argument)) } diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala index 6415a00167e..717453275d0 100644 --- a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala +++ b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Pretty.scala @@ -457,7 +457,7 @@ object Pretty { def prettySExpr(index: Int)(e: SExpr): Doc = e match { case SEVar(i) => char('@') + str(index - i) - case SEVal(defId, _) => + case SEVal(defId) => str(defId) case SEValue(lit) => lit match { diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SExpr.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SExpr.scala index fd08f7f6a6f..3faf3ad9ab5 100644 --- a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SExpr.scala +++ b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/SExpr.scala @@ -51,11 +51,27 @@ object SExpr { */ final case class SEVal( ref: SDefinitionRef, - var cached: Option[(SValue, List[Location])], ) extends SExpr { - def execute(machine: Machine): Unit = { + + // The variable `_cached` is used to cache the evaluation of the + // LF value defined by `ref` once it has been computed. Hence we + // avoid both the lookup in the package definition HashMap and the + // full reevaluation of the body of the definition. + // Here we take advantage of the Java memory model + // (https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.7) + // that guarantees the write of `_cache` (in the method + // `setCached`) is done atomically. + // This is similar how hashcode evaluation is cached in String + // http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/String.java + private var _cached: Option[(SValue, List[Location])] = None + + def cached: Option[(SValue, List[Location])] = _cached + + def setCached(sValue: SValue, stack_trace: List[Location]): Unit = + _cached = Some((sValue, stack_trace)) + + def execute(machine: Machine): Unit = machine.lookupVal(this) - } } /** Reference to a builtin function */ diff --git a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala index 386c3465a5f..7b85072f023 100644 --- a/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala +++ b/daml-lf/interpreter/src/main/scala/com/digitalasset/daml/lf/speedy/Speedy.scala @@ -685,9 +685,9 @@ object Speedy { * updates this solves the blow-up which would happen when a large record is * updated multiple times. */ final case class KCacheVal(v: SEVal, stack_trace: List[Location]) extends Kont { - def execute(sv: SValue, machine: Machine) = { + def execute(sv: SValue, machine: Machine): Unit = { machine.pushStackTrace(stack_trace) - v.cached = Some((sv, stack_trace)) + v.setCached(sv, stack_trace) machine.returnValue = sv } } diff --git a/daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/Runner.scala b/daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/Runner.scala index 4db580a5f62..6089da71392 100644 --- a/daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/Runner.scala +++ b/daml-script/runner/src/main/scala/com/digitalasset/daml/lf/engine/script/Runner.scala @@ -114,7 +114,7 @@ object Script { def fromIdentifier( compiledPackages: CompiledPackages, scriptId: Identifier): Either[String, Script] = { - val scriptExpr = SEVal(LfDefRef(scriptId), None) + val scriptExpr = SEVal(LfDefRef(scriptId)) val scriptTy = compiledPackages .getPackage(scriptId.packageId) .flatMap(_.lookupIdentifier(scriptId.qualifiedName).toOption) match {