Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inline compilation benchmarks and Improve searching Graph.Link performance #9520

Merged
merged 7 commits into from
Mar 28, 2024
Merged

Fix inline compilation benchmarks and Improve searching Graph.Link performance #9520

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
82a32a1
Improve performance for searching Graph.Link
hubertp Mar 22, 2024
7aa8a14
nit
hubertp Mar 22, 2024
86dda53
adapt tests
hubertp Mar 23, 2024
33c2183
PR review
hubertp Mar 25, 2024
e34e229
Fix inline benchmarks
hubertp Mar 27, 2024
6fc958b
nits
hubertp Mar 27, 2024
bddd484
Merge branch 'develop' into wip/hubert/compiler-inline-perf
hubertp Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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());
}

Expand Down
37 changes: 33 additions & 4 deletions engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If initXyz is better pattern than calling g.rootScope_$eq(rootScope); - then rootScope could be made private as well, possibly.

Copy link
Collaborator Author

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.

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an implementation of serialization logic for the Graph

enso/engine/runtime-compiler/src/main/java/org/enso/compiler/pass/analyse/PassPersistance.java

Lines 153 to 157 in 6665c22

@org.openide.util.lookup.ServiceProvider(service = Persistance.class)
public static final class PersistAliasAnalysisGraph extends Persistance<Graph> {
public PersistAliasAnalysisGraph() {
super(Graph.class, false, 1268);
}

Please make sure that it works correctly with the newly added fields and probably update the serialization id

Copy link
Collaborator Author

@hubertp hubertp Mar 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should work correctly as I'm reproducing sourceLinks and targetLinks from links when deserialized. I'm not serializing those fields as there is no need for that (links continues to be serialized).

var nextIdCounter = 0

private var globalSymbols: Map[Graph.Symbol, Occurrence.Global] =
Map()
Expand All @@ -24,11 +27,26 @@ 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
links.foreach { link =>
hubertp marked this conversation as resolved.
Show resolved Hide resolved
sourceLinks = sourceLinks.updatedWith(link.source)(v =>
v.map(s => s + link).orElse(Some(Set(link)))
hubertp marked this conversation as resolved.
Show resolved Hide resolved
)
targetLinks = targetLinks.updatedWith(link.target)(v =>
v.map(s => s + link).orElse(Some(Set(link)))
)
}
}

def getLinks(): Set[Graph.Link] = links
Copy link
Member

@JaroslavTulach JaroslavTulach Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to remove the links field all together and just compute the links from sourceLinks and targetLinks? The logic between initLinks and getLinks & co. would be simpler and possibly getLinks() isn't on critical path. Otherwise the inconsistency must have been caught somewhere, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of like that links is the source of truth and sourceLinks/targetLinks serve as aux indexes for quicker access. It also simplifies serde.


/** Registers a requested global symbol in the aliasing scope.
*
* @param sym the symbol occurrence
Expand All @@ -46,6 +64,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

Expand Down Expand Up @@ -85,6 +105,12 @@ sealed class Graph extends Serializable {
): Option[Graph.Link] = {
scopeFor(occurrence.id).flatMap(_.resolveUsage(occurrence).map { link =>
links += link
sourceLinks = sourceLinks.updatedWith(link.source)(v =>
hubertp marked this conversation as resolved.
Show resolved Hide resolved
v.map(s => s + link).orElse(Some(Set(link)))
)
targetLinks = targetLinks.updatedWith(link.target)(v =>
v.map(s => s + link).orElse(Some(Set(link)))
)
link
})
}
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great finding. Good fix. Thank you.

id,
Set()
)
}

/** Finds all links in the graph where `symbol` appears in the role
Expand Down
Loading