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

hubertp · 2024-03-22T10:24:21Z

Pull Request Description

As benchmarks show, a significant amount of time is spent traversing Set of Graph.Links. 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

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)

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.

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.

4e6 · 2024-03-23T12:26:27Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

+  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 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

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).

JaroslavTulach

Glad to see a speedup. Some comments related to the code left inline.

JaroslavTulach · 2024-03-25T08:19:29Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

-  var rootScope: Graph.Scope = new Graph.Scope()
-  var links: Set[Graph.Link] = Set()
-  var nextIdCounter          = 0
+  var rootScope: Graph.Scope                              = new Graph.Scope()


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

I tried and looks like a lot more stuff depends on it being public. I tried to keep the PR succinct for now.

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

JaroslavTulach · 2024-03-25T08:30:46Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

@@ -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(


Great finding. Good fix. Thank you.

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

JaroslavTulach · 2024-03-25T08:33:32Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

+    }
+  }
+
+  def getLinks(): Set[Graph.Link] = links


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?

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.

Treat InlineContext as a closeable resource, to ensure that benchmarks properly cleanup. The problem was that the number of scopes has been steadily increasing with every benchmark run. Traversing such additional scopes took time, hence exponential blow up in run times.

hubertp · 2024-03-27T16:30:06Z

With the last change the inline compilation 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)

...time-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResource.java

...nchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResourceBuilder.java

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala

Improve performance for searching Graph.Link

82a32a1

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.

hubertp requested review from 4e6, JaroslavTulach and Akirathan as code owners March 22, 2024 10:24

hubertp changed the title ~~Improve performance for searching Graph.Link~~ Improve searching Graph.Link performance Mar 22, 2024

hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Mar 22, 2024

nit

7aa8a14

4e6 reviewed Mar 23, 2024

View reviewed changes

adapt tests

86dda53

hubertp requested a review from 4e6 March 23, 2024 19:08

4e6 approved these changes Mar 23, 2024

View reviewed changes

JaroslavTulach approved these changes Mar 25, 2024

View reviewed changes

Akirathan approved these changes Mar 25, 2024

View reviewed changes

PR review

33c2183

enso-bot bot mentioned this pull request Mar 26, 2024

Conversion of UnresolvedConstructor to real constructor isn't instrumented #9381

Closed

hubertp changed the title ~~Improve searching Graph.Link performance~~ Fix inline compilation benchmarks and Improve searching Graph.Link performance Mar 27, 2024

JaroslavTulach reviewed Mar 27, 2024

View reviewed changes

...time-benchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResource.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Mar 27, 2024

View reviewed changes

...nchmarks/src/main/java/org/enso/compiler/benchmarks/inline/InlineContextResourceBuilder.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Mar 27, 2024

View reviewed changes

engine/runtime-compiler/src/main/scala/org/enso/compiler/pass/analyse/alias/Graph.scala Show resolved Hide resolved

nits

6fc958b

hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Mar 28, 2024

Merge branch 'develop' into wip/hubert/compiler-inline-perf

bddd484

hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Mar 28, 2024

mergify bot merged commit 1c724a4 into develop Mar 28, 2024
37 of 41 checks passed

mergify bot deleted the wip/hubert/compiler-inline-perf branch March 28, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hubertp commented Mar 22, 2024 •

edited

Loading

4e6 Mar 23, 2024

hubertp Mar 23, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Mar 25, 2024

hubertp Mar 25, 2024

JaroslavTulach Mar 25, 2024

JaroslavTulach Mar 25, 2024 •

edited

Loading

hubertp Mar 25, 2024

hubertp commented Mar 27, 2024

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

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

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

Conversation

hubertp commented Mar 22, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

4e6 Mar 23, 2024

Choose a reason for hiding this comment

hubertp Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Mar 25, 2024

Choose a reason for hiding this comment

hubertp Mar 25, 2024

Choose a reason for hiding this comment

JaroslavTulach Mar 25, 2024

Choose a reason for hiding this comment

JaroslavTulach Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

hubertp Mar 25, 2024

Choose a reason for hiding this comment

hubertp commented Mar 27, 2024

hubertp commented Mar 22, 2024 •

edited

Loading

hubertp Mar 23, 2024 •

edited

Loading

JaroslavTulach Mar 25, 2024 •

edited

Loading