Fix inline compilation benchmarks and Improve searching Graph.Link pe…

…rformance (#9520) As benchmarks show, a significant amount of time is spent traversing `Set` of `Graph.Link`s. That's unfortunate and unnecessary. We can equally keep helper maps that make search constant time. Fixed inline compilation benchmarks by properly cleaning up scopes after runs. Closes #9237. # Important Notes Things like ![Screenshot from 2024-03-22 11-23-01](https://github.com/enso-org/enso/assets/292128/7c1e220a-6e33-4396-a9b2-0e788f615323) ![Screenshot from 2024-03-22 11-13-19](https://github.com/enso-org/enso/assets/292128/0272b1cb-252e-4662-b539-174844941c8e) are all gone. There is plenty of it those are just samples. Benchmarks are back in order: ``` [info] # Warmup Iteration 1: 2.702 ms/op [info] # Warmup Iteration 2: 3.080 ms/op [info] # Warmup Iteration 3: 2.818 ms/op [info] # Warmup Iteration 4: 3.334 ms/op [info] # Warmup Iteration 5: 2.448 ms/op [info] # Warmup Iteration 6: 2.583 ms/op [info] Iteration 1: 2.908 ms/op [info] Iteration 2: 2.915 ms/op [info] Iteration 3: 2.774 ms/op [info] Iteration 4: 2.601 ms/op [info] Result "org.enso.compiler.benchmarks.inline.InlineCompilerBenchmark.longExpression": [info] 2.799 ±(99.9%) 0.953 ms/op [Average] [info] (min, avg, max) = (2.601, 2.799, 2.915), stdev = 0.148 [info] CI (99.9%): [1.846, 3.753] (assumes normal distribution) ```
enso-org · Mar 28, 2024 · 1c724a4 · 1c724a4
1 parent 207c7af
commit 1c724a4
Show file tree

Hide file tree

Showing 9 changed files with 153 additions and 58 deletions.
diff --git a/...benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineCompilerBenchmark.java b/...benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineCompilerBenchmark.java
@@ -3,10 +3,10 @@
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 import org.enso.compiler.Compiler;
 import org.enso.compiler.benchmarks.Utils;
-import org.enso.compiler.context.InlineContext;
 import org.graalvm.polyglot.Context;
 import org.openjdk.jmh.annotations.Benchmark;
 import org.openjdk.jmh.annotations.BenchmarkMode;
@@ -46,19 +46,18 @@ public class InlineCompilerBenchmark {
   private final OutputStream out = new ByteArrayOutputStream();
   private Compiler compiler;
   private Context ctx;
-  private InlineContext mainInlineContext;
+  private InlineSource inlineSource;
   private String longExpression;
+  private Set<String> localVarNames;
 
   @Setup
   public void setup() throws IOException {
     ctx = Utils.createDefaultContextBuilder().out(out).err(out).logHandler(out).build();
     var ensoCtx = Utils.leakEnsoContext(ctx);
     compiler = ensoCtx.getCompiler();
-
-    var inlineSource = InlineContextUtils.createMainMethodWithLocalVars(ctx, LOCAL_VARS_CNT);
-    mainInlineContext = inlineSource.mainInlineContext();
-    longExpression =
-        InlineContextUtils.createLongExpression(inlineSource.localVarNames(), LONG_EXPR_SIZE);
+    localVarNames = InlineContextUtils.localVarNames(LOCAL_VARS_CNT);
+    longExpression = InlineContextUtils.createLongExpression(localVarNames, LONG_EXPR_SIZE);
+    inlineSource = InlineContextUtils.createMainMethodWithLocalVars(ctx, localVarNames);
   }
 
   @TearDown
@@ -75,11 +74,13 @@ public void tearDown() {
   }
 
   @Benchmark
-  public void longExpression(Blackhole blackhole) {
-    var tuppleOpt = compiler.runInline(longExpression, mainInlineContext);
-    if (tuppleOpt.isEmpty()) {
-      throw new AssertionError("Unexpected: inline compilation should succeed");
+  public void longExpression(Blackhole blackhole) throws IOException {
+    try (InlineContextResource resource = inlineSource.inlineContextFactory().create()) {
+      var tuppleOpt = compiler.runInline(longExpression, resource.inlineContext());
+      if (tuppleOpt.isEmpty()) {
+        throw new AssertionError("Unexpected: inline compilation should succeed");
+      }
+      blackhole.consume(tuppleOpt);
     }
-    blackhole.consume(tuppleOpt);
   }
 }
diff --git a/...marks/src/main/java/org/enso/compiler/benchmarks/inline/InlineCompilerErrorBenchmark.java b/...marks/src/main/java/org/enso/compiler/benchmarks/inline/InlineCompilerErrorBenchmark.java
@@ -6,7 +6,6 @@
 import java.util.concurrent.TimeUnit;
 import org.enso.compiler.Compiler;
 import org.enso.compiler.benchmarks.Utils;
-import org.enso.compiler.context.InlineContext;
 import org.enso.polyglot.RuntimeOptions;
 import org.graalvm.polyglot.Context;
 import org.openjdk.jmh.annotations.Benchmark;
@@ -48,7 +47,7 @@ public class InlineCompilerErrorBenchmark {
   private Compiler compiler;
 
   private Context ctx;
-  private InlineContext mainInlineContext;
+  private InlineContextResourceFactory mainInlineContextResourceFactory;
   private String expressionWithErrors;
 
   @Setup
@@ -63,11 +62,12 @@ public void setup() throws IOException {
     var ensoCtx = Utils.leakEnsoContext(ctx);
     compiler = ensoCtx.getCompiler();
 
-    var inlineSource = InlineContextUtils.createMainMethodWithLocalVars(ctx, LOCAL_VARS_CNT);
+    var localVarNames = InlineContextUtils.localVarNames(LOCAL_VARS_CNT);
+    var inlineSource = InlineContextUtils.createMainMethodWithLocalVars(ctx, localVarNames);
     var longExpression =
         InlineContextUtils.createLongExpression(inlineSource.localVarNames(), LONG_EXPR_SIZE);
     expressionWithErrors = "UNDEFINED * " + longExpression + " * UNDEFINED";
-    mainInlineContext = inlineSource.mainInlineContext();
+    mainInlineContextResourceFactory = inlineSource.inlineContextFactory();
   }
 
   @TearDown
@@ -79,8 +79,10 @@ public void teardown() {
   }
 
   @Benchmark
-  public void expressionWithErrors(Blackhole blackhole) {
-    var tuppleOpt = compiler.runInline(expressionWithErrors, mainInlineContext);
-    blackhole.consume(tuppleOpt);
+  public void expressionWithErrors(Blackhole blackhole) throws IOException {
+    try (InlineContextResource resource = mainInlineContextResourceFactory.create()) {
+      var tuppleOpt = compiler.runInline(expressionWithErrors, resource.inlineContext());
+      blackhole.consume(tuppleOpt);
+    }
   }
 }
diff --git a/...e-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResource.java b/...e-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResource.java
@@ -0,0 +1,23 @@
+package org.enso.compiler.benchmarks.inline;
+
+import java.io.IOException;
+import org.enso.compiler.context.InlineContext;
+
+/**
+ * InlineContextResource ensures that the underlying InlineContext is properly cleaned up after
+ * usage.
+ *
+ * @param inlineContext InlineContext for the main method
+ */
+public record InlineContextResource(InlineContext inlineContext) implements AutoCloseable {
+  @Override
+  public void close() throws IOException {
+    inlineContext
+        .localScope()
+        .foreach(
+            s -> {
+              s.scope().removeScopeFromParent();
+              return null;
+            });
+  }
+}
diff --git a/...marks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResourceFactory.java b/...marks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResourceFactory.java
@@ -0,0 +1,28 @@
+package org.enso.compiler.benchmarks.inline;
+
+import org.enso.compiler.PackageRepository;
+import org.enso.compiler.context.InlineContext;
+import org.enso.interpreter.node.MethodRootNode;
+import org.enso.interpreter.runtime.EnsoContext;
+import org.enso.interpreter.runtime.data.Type;
+import org.enso.interpreter.runtime.scope.ModuleScope;
+
+public record InlineContextResourceFactory(
+    ModuleScope moduleScope,
+    Type assocTypeReceiver,
+    EnsoContext ensoCtx,
+    PackageRepository pkgRepository) {
+
+  public InlineContextResource create() {
+    var mainFunc = moduleScope.getMethodForType(assocTypeReceiver, "main");
+    var mainFuncRootNode = (MethodRootNode) mainFunc.getCallTarget().getRootNode();
+    var mainLocalScope = mainFuncRootNode.getLocalScope();
+    return new InlineContextResource(
+        InlineContext.fromJava(
+            mainLocalScope.createChild(),
+            moduleScope.getModule().asCompilerModule(),
+            scala.Option.apply(false),
+            ensoCtx.getCompilerConfig(),
+            scala.Option.apply(pkgRepository)));
+  }
+}
diff --git a/...time-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextUtils.java b/...time-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextUtils.java
@@ -5,8 +5,6 @@
 import java.util.Set;
 import org.enso.compiler.benchmarks.CodeGenerator;
 import org.enso.compiler.benchmarks.Utils;
-import org.enso.compiler.context.InlineContext;
-import org.enso.interpreter.node.MethodRootNode;
 import org.enso.interpreter.runtime.data.Type;
 import org.enso.polyglot.LanguageInfo;
 import org.enso.polyglot.MethodNames;
@@ -16,25 +14,32 @@
 class InlineContextUtils {
   private InlineContextUtils() {}
 
+  static Set<String> localVarNames(int localVarsCnt) {
+    Set<String> localVarNames = new HashSet<>();
+
+    for (int i = 0; i < localVarsCnt; i++) {
+      var varName = "loc_var_" + i;
+      localVarNames.add(varName);
+    }
+    return localVarNames;
+  }
+
   /**
    * Creates a main method, generates some local variables, and fills their identifiers in the given
    * set.
    *
    * @param localVarsCnt How many local variables should be initialized in the main method
    * @return Body of the main method
    */
-  static InlineSource createMainMethodWithLocalVars(Context ctx, int localVarsCnt)
+  static InlineSource createMainMethodWithLocalVars(Context ctx, Set<String> localVarNames)
       throws IOException {
     var sb = new StringBuilder();
     sb.append("main = ").append(System.lineSeparator());
     var codeGen = new CodeGenerator();
-    Set<String> localVarNames = new HashSet<>();
-    for (int i = 0; i < localVarsCnt; i++) {
-      var varName = "loc_var_" + i;
-      localVarNames.add(varName);
+    for (String localVarName : localVarNames) {
       var literal = codeGen.nextLiteral();
       sb.append("    ")
-          .append(varName)
+          .append(localVarName)
           .append(" = ")
           .append(literal)
           .append(System.lineSeparator());
@@ -51,18 +56,11 @@ static InlineSource createMainMethodWithLocalVars(Context ctx, int localVarsCnt)
     var moduleAssocType = module.invokeMember(MethodNames.Module.GET_ASSOCIATED_TYPE);
     var assocTypeReceiver = (Type) Utils.unwrapReceiver(ctx, moduleAssocType);
     var moduleScope = assocTypeReceiver.getDefinitionScope();
-    var mainFunc = moduleScope.getMethodForType(assocTypeReceiver, "main");
-    var mainFuncRootNode = (MethodRootNode) mainFunc.getCallTarget().getRootNode();
-    var mainLocalScope = mainFuncRootNode.getLocalScope();
     var compiler = ensoCtx.getCompiler();
-    var mainInlineContext =
-        InlineContext.fromJava(
-            mainLocalScope,
-            moduleScope.getModule().asCompilerModule(),
-            scala.Option.apply(false),
-            ensoCtx.getCompilerConfig(),
-            scala.Option.apply(compiler.packageRepository()));
-    return new InlineSource(sb.toString(), mainInlineContext, localVarNames);
+    var inlineCtxMeta =
+        new InlineContextResourceFactory(
+            moduleScope, assocTypeReceiver, ensoCtx, compiler.packageRepository());
+    return new InlineSource(sb.toString(), inlineCtxMeta, localVarNames);
   }
 
   static String createLongExpression(Set<String> localVars, int exprSize) {

diff --git a/...ne/runtime-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineSource.java b/...ne/runtime-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineSource.java
@@ -1,11 +1,10 @@
 package org.enso.compiler.benchmarks.inline;
 
 import java.util.Set;
-import org.enso.compiler.context.InlineContext;
 
 record InlineSource(
     String source,
-    // InlineContext for the main method
-    InlineContext mainInlineContext,
+    // InlineContextResource for the main method
+    InlineContextResourceFactory inlineContextFactory,
     // Local variables in main method
     Set<String> localVarNames) {}
diff --git a/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java b/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java
@@ -166,7 +166,7 @@ protected Graph readObject(Input in) throws IOException {
 
       var links =
           (scala.collection.immutable.Set) in.readInline(scala.collection.immutable.Set.class);
-      g.links_$eq(links);
+      g.initLinks(links);
 
       var nextIdCounter = in.readInt();
       g.nextIdCounter_$eq(nextIdCounter);
@@ -178,7 +178,7 @@ protected Graph readObject(Input in) throws IOException {
     @Override
     protected void writeObject(Graph obj, Output out) throws IOException {
       out.writeObject(obj.rootScope());
-      out.writeInline(scala.collection.immutable.Set.class, obj.links());
+      out.writeInline(scala.collection.immutable.Set.class, obj.getLinks());
       out.writeInt(obj.nextIdCounter());
     }
 

diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala
@@ -5,14 +5,17 @@ import org.enso.syntax.text.Debug
 import org.enso.compiler.pass.analyse.alias.Graph.{Occurrence, Scope}
 
 import java.util.UUID
+import scala.collection.immutable.HashMap
 import scala.collection.mutable
 import scala.reflect.ClassTag
 
 /** A graph containing aliasing information for a given root scope in Enso. */
 sealed class Graph extends Serializable {
-  var rootScope: Graph.Scope = new Graph.Scope()
-  var links: Set[Graph.Link] = Set()
-  var nextIdCounter          = 0
+  var rootScope: Graph.Scope                              = new Graph.Scope()
+  private var links: Set[Graph.Link]                      = Set()
+  private var sourceLinks: Map[Graph.Id, Set[Graph.Link]] = new HashMap()
+  private var targetLinks: Map[Graph.Id, Set[Graph.Link]] = new HashMap()
+  var nextIdCounter                                       = 0
 
   private var globalSymbols: Map[Graph.Symbol, Occurrence.Global] =
     Map()
@@ -24,11 +27,22 @@ sealed class Graph extends Serializable {
     val copy = new Graph
     copy.rootScope     = this.rootScope.deepCopy(scope_mapping)
     copy.links         = this.links
+    copy.sourceLinks   = this.sourceLinks
+    copy.targetLinks   = this.targetLinks
     copy.globalSymbols = this.globalSymbols
     copy.nextIdCounter = this.nextIdCounter
     copy
   }
 
+  def initLinks(links: Set[Graph.Link]): Unit = {
+    sourceLinks = new HashMap()
+    targetLinks = new HashMap()
+    links.foreach(addSourceTargetLink)
+    this.links = links
+  }
+
+  def getLinks(): Set[Graph.Link] = links
+
   /** Registers a requested global symbol in the aliasing scope.
     *
     * @param sym the symbol occurrence
@@ -46,6 +60,8 @@ sealed class Graph extends Serializable {
   def copy: Graph = {
     val graph = new Graph
     graph.links         = links
+    graph.sourceLinks   = sourceLinks
+    graph.targetLinks   = targetLinks
     graph.rootScope     = rootScope.deepCopy(mutable.Map())
     graph.nextIdCounter = nextIdCounter
 
@@ -85,10 +101,20 @@ sealed class Graph extends Serializable {
   ): Option[Graph.Link] = {
     scopeFor(occurrence.id).flatMap(_.resolveUsage(occurrence).map { link =>
       links += link
+      addSourceTargetLink(link)
       link
     })
   }
 
+  private def addSourceTargetLink(link: Graph.Link): Unit = {
+    sourceLinks = sourceLinks.updatedWith(link.source)(v =>
+      v.map(s => s + link).orElse(Some(Set(link)))
+    )
+    targetLinks = targetLinks.updatedWith(link.target)(v =>
+      v.map(s => s + link).orElse(Some(Set(link)))
+    )
+  }
+
   /** Resolves any links for the given usage of a symbol, assuming the symbol
     * is global (i.e. method, constructor etc.)
     *
@@ -129,7 +155,10 @@ sealed class Graph extends Serializable {
     * @return a list of links in which `id` occurs
     */
   def linksFor(id: Graph.Id): Set[Graph.Link] = {
-    links.filter(l => l.source == id || l.target == id)
+    sourceLinks.getOrElse(id, Set.empty[Graph.Link]) ++ targetLinks.getOrElse(
+      id,
+      Set()
+    )
   }
 
   /** Finds all links in the graph where `symbol` appears in the role
@@ -646,6 +675,17 @@ object Graph {
 
       isDirectChildOf || isChildOfChildren
     }
+
+    private def removeScopeFromParent(scope: Scope): Unit = {
+      childScopes = childScopes.filter(_ != scope)
+    }
+
+    /** Disassociates this Scope from its parent.
+      */
+    def removeScopeFromParent(): Unit = {
+      assert(this.parent.nonEmpty)
+      this.parent.foreach(_.removeScopeFromParent(this))
+    }
   }
 
   /** A link in the [[Graph]].