Fix inline compilation benchmarks and Improve searching Graph.Link performance #9520
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| }); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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))); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,10 @@ | ||
| package org.enso.compiler.benchmarks.inline; | ||
|
|
||
| import java.util.Set; | ||
|
|
||
| record InlineSource( | ||
| String source, | ||
| // InlineContextResource for the main method | ||
| InlineContextResourceFactory inlineContextFactory, | ||
| // Local variables in main method | ||
| Set<String> localVarNames) {} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
| 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() | ||
|
There was a problem hiding this comment. There is an implementation of serialization logic for the Please make sure that it works correctly with the newly added fields and probably update the serialization id There was a problem hiding this comment. Should work correctly as I'm reproducing |
||
| 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 = { | ||
hubertp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| sourceLinks = new HashMap() | ||
| targetLinks = new HashMap() | ||
| links.foreach(addSourceTargetLink) | ||
| this.links = links | ||
| } | ||
|
|
||
| def getLinks(): Set[Graph.Link] = links | ||
|
There was a problem hiding this comment. Wouldn't it be better to remove the There was a problem hiding this comment. I kind of like that |
||
|
|
||
| /** 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] = { | ||
| sourceLinks.getOrElse(id, Set.empty[Graph.Link]) ++ targetLinks.getOrElse( | ||
|
There was a problem hiding this comment. Great finding. Good fix. Thank you. |
||
| 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) | ||
hubertp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /** Disassociates this Scope from its parent. | ||
| */ | ||
| def removeScopeFromParent(): Unit = { | ||
| assert(this.parent.nonEmpty) | ||
| this.parent.foreach(_.removeScopeFromParent(this)) | ||
| } | ||
| } | ||
|
|
||
| /** A link in the [[Graph]]. | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
initXyzis better pattern than callingg.rootScope_$eq(rootScope);- thenrootScopecould be made private as well, possibly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried and looks like a lot more stuff depends on it being public. I tried to keep the PR succinct for now.