Skip to content

Commit

Permalink
Fix perf degradation for method calls on polyglot arrays
Browse files Browse the repository at this point in the history
This change stems from the investigation into elusive performance
degradation that became visible when implementing `Builder.to_vector`
with `slice` to not copy the store in
#3744.
We were seeing about 40% drop for a simple `filter` method calls
without any reason.

IGV debugging was fun, but didn't reveal much. Profiling with VisualVM
revealed that we were previously inlining most of nodes, while with the
new version that was no longer the case. Still, it was hard to figure
out why.

Decided to use
```
-Dpolyglot.engine.TraceDeoptimizeFrame=true
-Dpolyglot.engine.BackgroundCompilation=false
-Dpolyglot.engine.TraceCompilation=true
-Dpolyglot.engine.TraceInlining=true
```
which revealed a more than expected number of deoptimizations. With
```
-Dpolyglot.engine.TraceTransferToInterpreter=true
```
we were able to see the source of it:
```
[info] [2022-10-07T13:13:12.98Z] [engine] transferToInterpreter at
  Builder.to_vector<arg-2>(built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso:1091) <split-c6d56df>
    Builder.to_vector(built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso:1091) <split-6500912a>
    Vector.filter(built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso:347) <split-660ff9b4>
    ...
    ...
  org.graalvm.truffle/com.oracle.truffle.api.CompilerDirectives.transferToInterpreterAndInvalidate(CompilerDirectives.java:90)
    org.enso.interpreter.node.callable.resolver.MethodResolverNodeGen.execute(MethodResolverNodeGen.java:47)
    org.enso.interpreter.node.callable.resolver.HostMethodCallNode.getPolyglotCallType(HostMethodCallNode.java:146)
    org.enso.interpreter.node.callable.InvokeMethodNodeGen.execute(InvokeMethodNodeGen.java:86)
    org.enso.interpreter.node.callable.InvokeCallableNode.invokeDynamicSymbol(InvokeCallableNode.java:254)
    org.enso.interpreter.node.callable.InvokeCallableNodeGen.execute(InvokeCallableNodeGen.java:54)
    org.enso.interpreter.node.callable.ApplicationNode.executeGeneric(ApplicationNode.java:99)
    org.enso.interpreter.node.ClosureRootNode.execute(ClosureRootNode.java:90)
```
So the problem was in the new implementation of `to_vector` but not in
`slice`, as we expected, but in `list.size`.

Problem was that `MethodResolver` would always deoptimize for polyglot
array method calls because newly created `UnresolvedSymbol` in
[HostMethodCallNode](https://github.com/enso-org/enso/blob/ea60cd5fab48d9f6b16923cea21620b97216a898/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/HostMethodCallNode.java#L145)
wouldn't `==` to the cached one in
[MethodResolver](https://github.com/enso-org/enso/blob/ea60cd5fab48d9f6b16923cea21620b97216a898/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java#L38).

Fixed by providing a custom `equals` implementation for
`UnresolvedSymbol` and `s/==/equals` in `MethodResolverNode` cache
check.
  • Loading branch information
hubertp committed Oct 7, 2022
1 parent ea60cd5 commit 8fa2eed
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public Function expectNonNull(Object self, Type type, UnresolvedSymbol symbol) {
@Specialization(
guards = {
"!getContext().isInlineCachingDisabled()",
"cachedSymbol == symbol",
"cachedSymbol.equals(symbol)",
"cachedType == type"
},
limit = "CACHE_SIZE")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,15 @@ public boolean isExecutable() {
return true;
}

@Override
public boolean equals(Object other) {
if (this == other) return true;
if (other instanceof UnresolvedSymbol sym) {
return this.name.equals(sym.getName()) && this.scope.equals(sym.getScope());
}
return false;
}

/** Implements the logic of executing {@link UnresolvedSymbol} through the interop library. */
@ExportMessage
@ImportStatic(Constants.CacheSizes.class)
Expand Down

0 comments on commit 8fa2eed

Please sign in to comment.