From a5886a8f972d146d1aa35c16f2a2f2fb2dd34eda Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Fri, 7 Oct 2022 17:42:04 +0200 Subject: [PATCH 1/3] 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 https://github.com/enso-org/enso/pull/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(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) 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) 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) ... ... 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. --- .../node/callable/resolver/MethodResolverNode.java | 2 +- .../interpreter/runtime/callable/UnresolvedSymbol.java | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java index 0b9e4d71094e..a1f2c65cef98 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java @@ -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") diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java index cbffe835f40f..4927fecd7cf5 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java @@ -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) From cea8338c8851f7788b318837e7b8cced2527c1e5 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Mon, 10 Oct 2022 16:42:06 +0200 Subject: [PATCH 2/3] 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. --- .../Standard/Test/0.0.0-dev/src/Bench.enso | 2 +- .../callable/IndirectInvokeMethodNode.java | 8 ++++---- .../node/callable/InvokeMethodNode.java | 20 +++++++++---------- .../callable/resolver/HostMethodCallNode.java | 12 +++++------ .../callable/resolver/MethodResolverNode.java | 2 +- .../runtime/callable/UnresolvedSymbol.java | 9 --------- 6 files changed, 22 insertions(+), 31 deletions(-) diff --git a/distribution/lib/Standard/Test/0.0.0-dev/src/Bench.enso b/distribution/lib/Standard/Test/0.0.0-dev/src/Bench.enso index 42593908a298..9b6bfe9cd405 100644 --- a/distribution/lib/Standard/Test/0.0.0-dev/src/Bench.enso +++ b/distribution/lib/Standard/Test/0.0.0-dev/src/Bench.enso @@ -21,7 +21,7 @@ import Standard.Base.Runtime.Ref Bench.measure Examples.get_boolean "foo" iter_size=2 num_iters=1 measure : Any -> Text -> Integer -> Integer -> Nothing measure = ~act -> label -> iter_size -> num_iters -> - result = Ref.new 0 + result = Ref.new 0.0 single_call = _ -> x1 = System.nano_time Runtime.no_inline act diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeMethodNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeMethodNode.java index 7bdb2d3f257c..03eef4c2146e 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeMethodNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeMethodNode.java @@ -172,7 +172,7 @@ Stateful doPolyglot( int thisArgumentPosition, @CachedLibrary(limit = "10") TypesLibrary methods, @CachedLibrary(limit = "10") InteropLibrary interop, - @Bind("getPolyglotCallType(self, symbol.getName(), interop)") + @Bind("getPolyglotCallType(self, symbol, interop)") HostMethodCallNode.PolyglotCallType polyglotCallType, @Cached ThunkExecutorNode argExecutor, @Cached HostMethodCallNode hostMethodCallNode, @@ -194,7 +194,7 @@ Stateful doPolyglot( guards = { "!methods.hasType(self)", "!methods.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_TEXT" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_TEXT" }) Stateful doConvertText( MaterializedFrame frame, @@ -237,7 +237,7 @@ Stateful doConvertText( guards = { "!methods.hasType(self)", "!methods.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == NOT_SUPPORTED" + "getPolyglotCallType(self, symbol, interop) == NOT_SUPPORTED" }) Stateful doFallback( MaterializedFrame frame, @@ -252,7 +252,7 @@ Stateful doFallback( int thisArgumentPosition, @CachedLibrary(limit = "10") TypesLibrary methods, @CachedLibrary(limit = "10") InteropLibrary interop, - @Bind("getPolyglotCallType(self, symbol.getName(), interop)") + @Bind("getPolyglotCallType(self, symbol, interop)") HostMethodCallNode.PolyglotCallType polyglotCallType, @Cached MethodResolverNode methodResolverNode, @Cached IndirectInvokeFunctionNode invokeFunctionNode) { diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java index 6aac516d90ec..4907ec7b983a 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/InvokeMethodNode.java @@ -177,7 +177,7 @@ Stateful doPolyglot( @CachedLibrary(limit = "10") TypesLibrary methods, @CachedLibrary(limit = "10") InteropLibrary interop, @Cached MethodResolverNode preResolveMethod, - @Bind("getPolyglotCallType(self, symbol.getName(), interop, preResolveMethod)") + @Bind("getPolyglotCallType(self, symbol, interop, preResolveMethod)") HostMethodCallNode.PolyglotCallType polyglotCallType, @Cached(value = "buildExecutors()") ThunkExecutorNode[] argExecutors, @Cached(value = "buildProfiles()", dimensions = 1) BranchProfile[] profiles, @@ -214,7 +214,7 @@ Stateful doPolyglot( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_TEXT" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_TEXT" }) Stateful doConvertText( VirtualFrame frame, @@ -242,7 +242,7 @@ Stateful doConvertText( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_ARRAY", + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_ARRAY", }, rewriteOn = AbstractMethodError.class) Stateful doConvertArray( @@ -272,7 +272,7 @@ Stateful doConvertArray( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop, methodResolverNode) == CONVERT_TO_ARRAY" + "getPolyglotCallType(self, symbol, interop, methodResolverNode) == CONVERT_TO_ARRAY" }, replaces = "doConvertArray") Stateful doConvertArrayWithCheck( @@ -295,7 +295,7 @@ Stateful doConvertArrayWithCheck( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_DATE" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_DATE" }) Stateful doConvertDate( VirtualFrame frame, @@ -323,7 +323,7 @@ Stateful doConvertDate( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_DATE_TIME" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_DATE_TIME" }) Stateful doConvertDateTime( VirtualFrame frame, @@ -363,7 +363,7 @@ private ZonedDateTime dateTime(LocalDate date, LocalTime time, ZoneId zone) { guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_ZONED_DATE_TIME" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_ZONED_DATE_TIME" }) Stateful doConvertZonedDateTime( VirtualFrame frame, @@ -393,7 +393,7 @@ Stateful doConvertZonedDateTime( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_TIME_ZONE" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_TIME_ZONE" }) Stateful doConvertZone( VirtualFrame frame, @@ -421,7 +421,7 @@ Stateful doConvertZone( guards = { "!types.hasType(self)", "!types.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == CONVERT_TO_TIME_OF_DAY" + "getPolyglotCallType(self, symbol, interop) == CONVERT_TO_TIME_OF_DAY" }) Stateful doConvertTimeOfDay( VirtualFrame frame, @@ -449,7 +449,7 @@ Stateful doConvertTimeOfDay( guards = { "!methods.hasType(self)", "!methods.hasSpecialDispatch(self)", - "getPolyglotCallType(self, symbol.getName(), interop) == NOT_SUPPORTED" + "getPolyglotCallType(self, symbol, interop) == NOT_SUPPORTED" }) Stateful doFallback( VirtualFrame frame, diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/HostMethodCallNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/HostMethodCallNode.java index ce52d9bdb33d..9b416894a1bb 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/HostMethodCallNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/HostMethodCallNode.java @@ -98,13 +98,13 @@ public boolean isInteropLibrary() { * used. * * @param self the method call target - * @param methodName the method name + * @param symbol symbol representing method to be resolved * @param library an instance of interop library to use for interacting with the target * @return a {@link PolyglotCallType} to use for this target and method */ public static PolyglotCallType getPolyglotCallType( - Object self, String methodName, InteropLibrary library) { - return getPolyglotCallType(self, methodName, library, null); + Object self, UnresolvedSymbol symbol, InteropLibrary library) { + return getPolyglotCallType(self, symbol, library, null); } /** @@ -112,14 +112,14 @@ public static PolyglotCallType getPolyglotCallType( * used. * * @param self the method call target - * @param methodName the method name + * @param symbol symbol representing method to be resolved * @param library an instance of interop library to use for interacting with the target * @param methodResolverNode {@code null} or real instances of the node to resolve methods * @return a {@link PolyglotCallType} to use for this target and method */ public static PolyglotCallType getPolyglotCallType( Object self, - String methodName, + UnresolvedSymbol symbol, InteropLibrary library, MethodResolverNode methodResolverNode) { if (library.isDate(self)) { @@ -142,7 +142,6 @@ public static PolyglotCallType getPolyglotCallType( if (methodResolverNode != null) { var ctx = Context.get(library); var arrayType = ctx.getBuiltins().array(); - var symbol = UnresolvedSymbol.build(methodName, ctx.getBuiltins().getScope()); var fn = methodResolverNode.execute(arrayType, symbol); if (fn != null) { return PolyglotCallType.CONVERT_TO_ARRAY; @@ -152,6 +151,7 @@ public static PolyglotCallType getPolyglotCallType( } } + String methodName = symbol.getName(); if (library.isMemberInvocable(self, methodName)) { return PolyglotCallType.CALL_METHOD; } else if (library.isMemberReadable(self, methodName)) { diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java index a1f2c65cef98..0b9e4d71094e 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/callable/resolver/MethodResolverNode.java @@ -35,7 +35,7 @@ public Function expectNonNull(Object self, Type type, UnresolvedSymbol symbol) { @Specialization( guards = { "!getContext().isInlineCachingDisabled()", - "cachedSymbol.equals(symbol)", + "cachedSymbol == symbol", "cachedType == type" }, limit = "CACHE_SIZE") diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java index 4927fecd7cf5..cbffe835f40f 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/UnresolvedSymbol.java @@ -97,15 +97,6 @@ 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) From 1e760bc68e1f53cd420cc6d2440f5831956d3cd7 Mon Sep 17 00:00:00 2001 From: Hubert Plociniczak Date: Mon, 10 Oct 2022 16:46:52 +0200 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1372ecb5c67e..d71b0a6dbed1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -380,6 +380,7 @@ - [Notify node status to the IDE][3729] - [Distinguish static and instance methods][3740] - [By-type pattern matching][3742] +- [Fix performance of method calls on polyglot arrays][3781] [3227]: https://github.com/enso-org/enso/pull/3227 [3248]: https://github.com/enso-org/enso/pull/3248 @@ -431,6 +432,7 @@ [3729]: https://github.com/enso-org/enso/pull/3729 [3740]: https://github.com/enso-org/enso/pull/3740 [3742]: https://github.com/enso-org/enso/pull/3742 +[3781]: https://github.com/enso-org/enso/pull/3781 # Enso 2.0.0-alpha.18 (2021-10-12)