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

Jump to bottom

Fix for perf degradation in method calls on polyglot arrays #3781

Merged
merged 3 commits into from
Oct 10, 2022
Merged

Fix for perf degradation in method calls on polyglot arrays #3781

merged 3 commits into from
Oct 10, 2022

Commits on Oct 10, 2022

  1. Fix perf degradation for method calls on polyglot arrays 

    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.
    @hubertp
    hubertp committed Oct 10, 2022
    Configuration menu
    Copy the full SHA
    a5886a8 View commit details
    Browse the repository at this point in the history

  2. Change the signature of getPolyglotCallType 

    Rather than creating a new `UnresolvedSymbol` from the symbol's name
and then overriding `equals` we can just apply the original
`UnresolvedSymbol`.

Also, minor change in Bench to avoid unnecessary transfer to interpreter
as it showed up in the logs.
    @hubertp
    hubertp committed Oct 10, 2022
    Configuration menu
    Copy the full SHA
    cea8338 View commit details
    Browse the repository at this point in the history

  3. changelog

    @hubertp
    hubertp committed Oct 10, 2022
    Configuration menu
    Copy the full SHA
    1e760bc View commit details
    Browse the repository at this point in the history