From b8903760f12ab901ff3bd0d492a9e99138bd495e Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 3 Apr 2024 16:46:45 +0200 Subject: [PATCH 1/5] Improve perf of Graph.Scope.scopeFor hotspot `scopeFor` appears to be a hotspot of the compiler. By choosing a more suitable data structure that indexes on the occurrence's id we seem to gain about 25% on some benchmarks. --- .../pass/analyse/PassPersistance.java | 4 +- .../enso/compiler/context/LocalScope.scala | 4 +- .../compiler/pass/analyse/AliasAnalysis.scala | 4 +- .../compiler/pass/analyse/alias/Graph.scala | 38 +++++++++++-------- 4 files changed, 28 insertions(+), 22 deletions(-) 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 index 9695be2851e7..8b3cb21137fe 100644 --- 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 @@ -121,14 +121,14 @@ protected TailCall.TailPosition readObject(Input in) @org.openide.util.lookup.ServiceProvider(service = Persistance.class) public static final class PersistAliasAnalysisGraphScope extends Persistance { public PersistAliasAnalysisGraphScope() { - super(Graph.Scope.class, false, 1267); + super(Graph.Scope.class, false, 1269); } @Override @SuppressWarnings("unchecked") protected Graph.Scope readObject(Input in) throws IOException { var childScopes = in.readInline(scala.collection.immutable.List.class); - var occurrences = (scala.collection.immutable.Set) in.readObject(); + var occurrences = (scala.collection.immutable.Map) in.readObject(); var allDefinitions = in.readInline(scala.collection.immutable.List.class); var parent = new Graph.Scope(childScopes, occurrences, allDefinitions); var optionParent = Option.apply(parent); diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/context/LocalScope.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/context/LocalScope.scala index 1f6218a880bb..dc70212d5c1e 100644 --- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/context/LocalScope.scala +++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/context/LocalScope.scala @@ -140,10 +140,10 @@ class LocalScope( .getOrElse(Map()) scope.occurrences.foreach { - case x: AliasGraph.Occurrence.Def => + case (id, x: AliasGraph.Occurrence.Def) => parentResult += x.symbol -> new FramePointer( level, - allFrameSlotIdxs(x.id) + allFrameSlotIdxs(id) ) case _ => } diff --git a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala index 1f0dd21ffd5f..8484d3875e97 100644 --- a/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala +++ b/engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/AliasAnalysis.scala @@ -602,12 +602,12 @@ case object AliasAnalysis extends IRPass { ) ) } else { - val f = scope.occurrences.collectFirst { + val f = scope.occurrences.values.collectFirst { case x if x.symbol == name.name => x } arg .copy( - ascribedType = Some(new Redefined.Arg(name, arg.location)) + ascribedType = Some(Redefined.Arg(name, arg.location)) ) .updateMetadata( new MetadataPair( 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 index adfc3cfb15a8..76bc77862ca1 100644 --- 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 @@ -272,11 +272,11 @@ sealed class Graph extends Serializable { def getShadowedIds( scope: Graph.Scope ): Set[Graph.Occurrence] = { - scope.occurrences.collect { + scope.occurrences.values.collect { case d: Occurrence.Def if d.symbol == definition.symbol => d case g: Occurrence.Global if g.symbol == definition.symbol => g } ++ scope.parent.map(getShadowedIds).getOrElse(Set()) - } + }.toSet definition match { case d: Occurrence.Def => @@ -343,13 +343,13 @@ object Graph { /** A representation of a local scope in Enso. * * @param childScopes all scopes that are _direct_ children of `this` - * @param occurrences all symbol occurrences in `this` scope + * @param occurrences all symbol occurrences in `this` scope indexed by the identifier of the name * @param allDefinitions all definitions in this scope, including synthetic ones. * Note that there may not be a link for all these definitions. */ sealed class Scope( var childScopes: List[Scope] = List(), - var occurrences: Set[Occurrence] = Set(), + var occurrences: Map[Id, Occurrence] = HashMap(), var allDefinitions: List[Occurrence.Def] = List() ) extends Serializable { @@ -437,7 +437,15 @@ object Graph { * @param occurrence the occurrence to add */ def add(occurrence: Occurrence): Unit = { - occurrences += occurrence + occurrences = occurrences.updatedWith(occurrence.id)(current => + current + .map(_ => + throw new CompilerError( + s"Multiple occurrences found for ID ${occurrence.id}." + ) + ) + .orElse(Some(occurrence)) + ) } /** Adds a definition, including a definition with synthetic name, without @@ -456,7 +464,7 @@ object Graph { * @return the occurrence for `id`, if it exists */ def getOccurrence(id: Graph.Id): Option[Occurrence] = { - occurrences.find(o => o.id == id) + occurrences.get(id) } /** Finds any occurrences for the provided symbol in the current scope, if @@ -469,9 +477,9 @@ object Graph { def getOccurrences[T <: Occurrence: ClassTag]( symbol: Graph.Symbol ): Set[Occurrence] = { - occurrences.collect { + occurrences.values.collect { case o: T if o.symbol == symbol => o - } + }.toSet } /** Unsafely gets the occurrence for the provided ID in the current scope. @@ -496,7 +504,7 @@ object Graph { def hasSymbolOccurrenceAs[T <: Occurrence: ClassTag]( symbol: Graph.Symbol ): Boolean = { - occurrences.collect { case x: T if x.symbol == symbol => x }.nonEmpty + occurrences.collect { case (_, x: T) if x.symbol == symbol => x }.nonEmpty } /** Resolves usages of symbols into links where possible, creating an edge @@ -511,7 +519,7 @@ object Graph { occurrence: Graph.Occurrence.Use, parentCounter: Int = 0 ): Option[Graph.Link] = { - val definition = occurrences.find { + val definition = occurrences.values.find { case Graph.Occurrence.Def(_, name, _, _, _) => name == occurrence.symbol case _ => false @@ -530,7 +538,7 @@ object Graph { * @return a string representation of `this` */ override def toString: String = - s"Scope(occurrences = $occurrences, childScopes = $childScopes)" + s"Scope(occurrences = ${occurrences.values}, childScopes = $childScopes)" /** Counts the number of scopes in this scope. * @@ -555,7 +563,7 @@ object Graph { * @return the scope where `id` occurs */ def scopeFor(id: Graph.Id): Option[Scope] = { - val possibleCandidates = occurrences.filter(o => o.id == id) + val possibleCandidates = occurrences.get(id) if (possibleCandidates.isEmpty) { if (childScopes.isEmpty) { @@ -584,10 +592,8 @@ object Graph { Some(childCandidate) } } - } else if (possibleCandidates.size == 1) { - Some(this) } else { - throw new CompilerError(s"Multiple occurrences found for ID $id.") + Some(this) } } @@ -629,7 +635,7 @@ object Graph { * @return the set of symbols */ def symbols: Set[Graph.Symbol] = { - val symbolsInThis = occurrences.map(_.symbol) + val symbolsInThis = occurrences.values.map(_.symbol).toSet val symbolsInChildScopes = childScopes.flatMap(_.symbols) symbolsInThis ++ symbolsInChildScopes From 31206212daa4c2ad0e88902dd8a3ed64e1bbb671 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 3 Apr 2024 17:15:19 +0200 Subject: [PATCH 2/5] nit --- .../compiler/pass/analyse/alias/Graph.scala | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) 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 index 76bc77862ca1..2a43f913adde 100644 --- 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 @@ -437,15 +437,13 @@ object Graph { * @param occurrence the occurrence to add */ def add(occurrence: Occurrence): Unit = { - occurrences = occurrences.updatedWith(occurrence.id)(current => - current - .map(_ => - throw new CompilerError( - s"Multiple occurrences found for ID ${occurrence.id}." - ) - ) - .orElse(Some(occurrence)) - ) + if (occurrences.contains(occurrence.id)) { + throw new CompilerError( + s"Multiple occurrences found for ID ${occurrence.id}." + ) + } else { + occurrences += ((occurrence.id, occurrence)) + } } /** Adds a definition, including a definition with synthetic name, without @@ -504,7 +502,9 @@ object Graph { def hasSymbolOccurrenceAs[T <: Occurrence: ClassTag]( symbol: Graph.Symbol ): Boolean = { - occurrences.collect { case (_, x: T) if x.symbol == symbol => x }.nonEmpty + occurrences.values.collectFirst { + case x: T if x.symbol == symbol => x + }.nonEmpty } /** Resolves usages of symbols into links where possible, creating an edge @@ -563,9 +563,7 @@ object Graph { * @return the scope where `id` occurs */ def scopeFor(id: Graph.Id): Option[Scope] = { - val possibleCandidates = occurrences.get(id) - - if (possibleCandidates.isEmpty) { + if (!occurrences.contains(id)) { if (childScopes.isEmpty) { None } else { From ea1ad09944915c006213d6b1dd5371455475ae78 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Wed, 3 Apr 2024 22:20:37 +0200 Subject: [PATCH 3/5] Missing Persistance for Integer --- .../enso/compiler/core/ir/IrPersistance.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java b/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java index 6c754620e391..2de2545e7998 100644 --- a/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java +++ b/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java @@ -192,6 +192,24 @@ protected Double readObject(Input in) throws IOException, ClassNotFoundException } } + @ServiceProvider(service = Persistance.class) + public static final class PersistInteger extends Persistance { + public PersistInteger() { + super(Integer.class, true, 4441); + } + + @Override + protected void writeObject(Integer obj, Output out) throws IOException { + out.writeDouble(obj); + } + + @Override + protected Integer readObject(Input in) throws IOException, ClassNotFoundException { + var obj = in.readInt(); + return obj; + } + } + @ServiceProvider(service = Persistance.class) public static final class PersistScalaList extends Persistance { public PersistScalaList() { From fd874ad960c7de6194a5d36952c9f6f5b62054dd Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Thu, 4 Apr 2024 10:55:13 +0200 Subject: [PATCH 4/5] PR review --- .../compiler/pass/analyse/PassPersistance.java | 11 ++++++++--- .../enso/compiler/core/ir/IrPersistance.java | 18 ------------------ 2 files changed, 8 insertions(+), 21 deletions(-) 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 index 8b3cb21137fe..82c1d843d120 100644 --- 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 @@ -23,7 +23,10 @@ import org.enso.persist.Persistable; import org.enso.persist.Persistance; import org.openide.util.lookup.ServiceProvider; +import scala.$less$colon$less; import scala.Option; +import scala.Tuple2; +import scala.Tuple2$; @Persistable(clazz = CachePreferenceAnalysis.WeightInfo.class, id = 1111) @Persistable(clazz = DataflowAnalysis.DependencyInfo.class, id = 1112) @@ -121,14 +124,16 @@ protected TailCall.TailPosition readObject(Input in) @org.openide.util.lookup.ServiceProvider(service = Persistance.class) public static final class PersistAliasAnalysisGraphScope extends Persistance { public PersistAliasAnalysisGraphScope() { - super(Graph.Scope.class, false, 1269); + super(Graph.Scope.class, false, 1267); } @Override @SuppressWarnings("unchecked") protected Graph.Scope readObject(Input in) throws IOException { var childScopes = in.readInline(scala.collection.immutable.List.class); - var occurrences = (scala.collection.immutable.Map) in.readObject(); + var occurrencesValues = (scala.collection.immutable.Set) in.readObject(); + var ev = ($less$colon$less>) null; + var occurrences = occurrencesValues.map(v -> Tuple2$.MODULE$.apply(v.id(), v)).toMap(ev); var allDefinitions = in.readInline(scala.collection.immutable.List.class); var parent = new Graph.Scope(childScopes, occurrences, allDefinitions); var optionParent = Option.apply(parent); @@ -145,7 +150,7 @@ protected Graph.Scope readObject(Input in) throws IOException { @SuppressWarnings("unchecked") protected void writeObject(Graph.Scope obj, Output out) throws IOException { out.writeInline(scala.collection.immutable.List.class, obj.childScopes()); - out.writeObject(obj.occurrences()); + out.writeObject(obj.occurrences().values().toSet()); out.writeInline(scala.collection.immutable.List.class, obj.allDefinitions()); } } diff --git a/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java b/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java index 2de2545e7998..6c754620e391 100644 --- a/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java +++ b/engine/runtime-parser/src/main/java/org/enso/compiler/core/ir/IrPersistance.java @@ -192,24 +192,6 @@ protected Double readObject(Input in) throws IOException, ClassNotFoundException } } - @ServiceProvider(service = Persistance.class) - public static final class PersistInteger extends Persistance { - public PersistInteger() { - super(Integer.class, true, 4441); - } - - @Override - protected void writeObject(Integer obj, Output out) throws IOException { - out.writeDouble(obj); - } - - @Override - protected Integer readObject(Input in) throws IOException, ClassNotFoundException { - var obj = in.readInt(); - return obj; - } - } - @ServiceProvider(service = Persistance.class) public static final class PersistScalaList extends Persistance { public PersistScalaList() { From 6c50c9c89adeea8381606be1d94c7469a9581360 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Thu, 4 Apr 2024 14:40:54 +0200 Subject: [PATCH 5/5] simplify --- .../java/org/enso/compiler/pass/analyse/PassPersistance.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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 index 82c1d843d120..d73d4f89e742 100644 --- 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 @@ -23,9 +23,7 @@ import org.enso.persist.Persistable; import org.enso.persist.Persistance; import org.openide.util.lookup.ServiceProvider; -import scala.$less$colon$less; import scala.Option; -import scala.Tuple2; import scala.Tuple2$; @Persistable(clazz = CachePreferenceAnalysis.WeightInfo.class, id = 1111) @@ -132,8 +130,7 @@ public PersistAliasAnalysisGraphScope() { protected Graph.Scope readObject(Input in) throws IOException { var childScopes = in.readInline(scala.collection.immutable.List.class); var occurrencesValues = (scala.collection.immutable.Set) in.readObject(); - var ev = ($less$colon$less>) null; - var occurrences = occurrencesValues.map(v -> Tuple2$.MODULE$.apply(v.id(), v)).toMap(ev); + var occurrences = occurrencesValues.map(v -> Tuple2$.MODULE$.apply(v.id(), v)).toMap(null); var allDefinitions = in.readInline(scala.collection.immutable.List.class); var parent = new Graph.Scope(childScopes, occurrences, allDefinitions); var optionParent = Option.apply(parent);