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.
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
Inline methods that fold to a constant before the static analysis #2860
Inline methods that fold to a constant before the static analysis #2860
Conversation
Hello Marjana Solajic, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address marjana -(dot)- s1010 -(at)- gmail -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
a42457f
to
6a6bfbc
Compare
@@ -88,5 +96,10 @@ protected boolean tryInvocationPlugin(InvokeKind invokeKind, ValueNode[] args, R | |||
public boolean canDeferPlugin(GeneratedInvocationPlugin plugin) { | |||
return plugin.getSource().equals(Fold.class) || plugin.getSource().equals(Node.NodeIntrinsic.class); | |||
} | |||
|
|||
@Override | |||
protected Invoke createNonInlinedInvoke(ExceptionEdgeAction exceptionEdge, int invokeBci, CallTargetNode callTarget, JavaKind resultType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an unnecessary override. Same in HostedGraphBuilderPhase
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
|
||
public static class Options { | ||
@Option(help = "Inline methods which folds to constant during parsing before the static analysis.")// | ||
public static final HostedOptionKey<Boolean> InlineBeforeAnalysis = new HostedOptionKey<>(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it not enabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
* After analysis we read information from this map to find inline decision we made using | ||
* {@link #getResult}. | ||
*/ | ||
private static final ConcurrentHashMap<AnalysisMethod, ConcurrentHashMap<CallSite, InvocationResult>> dataInline = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use any static
fields. Many image builds can run in the same VM, and no such state must survive a build. The standard solution is to put such information in an object registered in ImageSingletons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
|
||
private static ConcurrentMap<CallSite, InvocationResult> callSitemap(ResolvedJavaMethod method) { | ||
AnalysisMethod key; | ||
if (method instanceof AnalysisMethod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is toAnalysisMethod
as a helper method for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
public static InvocationResult findResult(String methodName, String className) { | ||
InvocationResult result = null; | ||
for (Map.Entry<AnalysisMethod, ConcurrentHashMap<CallSite, InvocationResult>> pair : dataInline.entrySet()) { | ||
if (pair.getKey().format("%n %H").equals(methodName + " " + className)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, don't use string formatting to check for a nested name. Even in a method only used for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, we can use now information from an object registered in ImageSingletons for tests
Collection<InvocationResult> results = pair.getValue().values(); | ||
/* in tests we call method only once */ | ||
assert results.size() == 1; | ||
result = results.iterator().next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if methodName
is not unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -151,7 +151,8 @@ protected void verifyParameters(MethodCallTargetNode callTarget, StructuredGraph | |||
"org.graalvm.compiler.core.test.VerifyDebugUsageTest$InvalidConcatDumpUsagePhase.run", | |||
"org.graalvm.compiler.core.test.VerifyDebugUsageTest$InvalidDumpUsagePhase.run", | |||
"org.graalvm.compiler.hotspot.SymbolicSnippetEncoder.verifySnippetEncodeDecode", | |||
"org.graalvm.compiler.truffle.compiler.phases.inlining.CallTree.dumpBasic")); | |||
"org.graalvm.compiler.truffle.compiler.phases.inlining.CallTree.dumpBasic", | |||
"com.oracle.svm.hosted.phases.TrivialMethodDetector.analyzeMethod")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We certainly don't want to dump anything at level 1 when inlining during analysis. Maybe at level 2, could also be level 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/* Nothing to do, it's ok to read a static or instance field. */ | ||
} else if (node instanceof FrameState) { | ||
/* Nothing to do */ | ||
} else if (node instanceof NewInstanceNode || node instanceof NewArrayNode || node instanceof CallTargetNode || node instanceof StoreFieldNode || node instanceof InvokeNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR says "This plugin inlines methods which folds to a constant". So why allow CallTargetNode
and StoreFieldNode
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, for now, only left NewArrayNode because we need this for the #2500
*/ | ||
VMError.guarantee(previous == null, "Detected previously intrinsified element"); | ||
Object previous = analysisElements.putIfAbsent(new CallSiteDescriptor(method, bci), nonNullElement); | ||
VMError.guarantee(previous == null || previous.equals(nonNullElement), "Newly intrinsified element (" + nonNullElement + ") different than the previous (" + previous + ")"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using equals
here to compare the previous and new element is dangerous. For example, for java.lang.reflect.Method
two methods can be equals
but still separate objects, which will lead to problems.
You must check for previous == nonNullElement
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -320,6 +321,9 @@ private static boolean processGetConstructor(GraphBuilderContext b, ResolvedJava | |||
/* We are analyzing the static initializers and should always intrinsify. */ | |||
return element; | |||
} | |||
if (((SharedGraphBuilderPhase.SharedBytecodeParser) context).getGraphBuilderConfig().getPlugins().getParameterPlugins().length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this check doing? But relying on having a parameter plugin registered is most certainly wrong, because we cannot rely on a side effect like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
76930fb
to
4e4ee4a
Compare
d5a8a6c
to
7ee4c06
Compare
7ee4c06
to
c23b20f
Compare
b993cad
to
71da1a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marjanasolajic I reviewed your PR. Looks good, most of my comments refer to cosmetic changes. The only real question is the one related to the performance impact of this analysis and the fact that it looks like the analysis is repeated for a call site when that could be avoided.
} | ||
|
||
if (callee.getAnnotation(NeverInline.class) != null || callee.getAnnotation(NeverInlineTrivial.class) != null || b.getMethod().getAnnotation(NeverInline.class) != null || | ||
b.getMethod().getAnnotation(NeverInlineTrivial.class) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a @NeverInline
annotation on a caller prevent inlining of any callees?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed for caller, that was a mistake, left only for a callee.
InvocationResult inline; | ||
|
||
if (analysis) { | ||
inline = ImageSingletons.lookup(NativeImageInlineDuringParsingPlugin.class).get(b.getMethod(), b.bci(), callee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ImageSingletons.lookup(...)
line is identical in both branches you can pull it up: InvocationResult inline = ImageSingletons.lookup(...)
and remove else
branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@@ -76,6 +76,8 @@ protected void run(StructuredGraph graph) { | |||
private final boolean explicitExceptionEdges; | |||
private final boolean allowIncompleteClassPath; | |||
|
|||
protected NativeImageInlineDuringParsingPlugin.InvocationResultInline inlineDuringParsingState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document inlineDuringParsingState
. It seems like it is used to store inline data for the currently parsed invoke.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is its purpose. Added.
static final class CallSite { | ||
final AnalysisMethod caller; | ||
final int bci; | ||
final AnalysisMethod callee; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the callee
field needed? Doesn't seem to be used anywhere and it is excluded from the equality check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I forgot to include this field in the check. Fixed.
} | ||
|
||
@Override | ||
public int hashCode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be removed, implementation is identical with super method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
return InvocationResult.ANALYSIS_TOO_COMPLICATED; | ||
} | ||
|
||
MethodNodeTrackingAndInline methodState = new MethodNodeTrackingAndInline(new InvocationResultInline(callSite, method)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MethodNodeTrackingAndInline.result
is not actually used in the class, it is just stored. Why not just allocate the InvocationResultInline
below, in the return
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we use it to store decisions about children inlining.
|
||
CallSite callSite = new CallSite((AnalysisMethod) b.getMethod(), b.bci(), NativeImageInlineDuringParsingPlugin.toAnalysisMethod(callee)); | ||
InvocationResult state = analyzeMethod(callSite, (AnalysisMethod) callee, args); | ||
ImageSingletons.lookup(NativeImageInlineDuringParsingPlugin.class).add(b.getMethod(), b.bci(), (AnalysisMethod) callee, state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this call site is reached by multiple call chains you run the analysis each time, then check if the results match. Wouldn't it be more efficient to run the analysis just once for each call site? What is the performance impact of this analysis on image build time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Thanks for the advice. The way of method analysis is changed. We now remember the whole bci chain of calling context.
* We collect information during graph construction to filter non-trivial methods and inline | ||
* trivial invokes. The result is used in {@link #analyzeMethod}. | ||
*/ | ||
class MethodNodeTrackingAndInline extends NodeEventListener implements InlineInvokePlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be split into two classes one that extends NodeEventListener
and one that implements InlineInvokePlugin
, there is no shared state between the two so I think it is cleaner if they are separated, just like you do with the TrivialMethodDetectorParameterPlugin
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the suggestion.
for (InlineInvokePlugin plugin : inlineInvokePlugins) { | ||
plugin.notifyAfterInline(inlineMethod); | ||
if (notifyGraphBuilderContext == null) { | ||
notifyGraphBuilderContext = new PENonAppendGraphBuilderContext(methodScope, invokeData.invoke); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notifyGraphBuilderContext
can be initialized outside the loop. If you want to init it only when there are some inline plugins installed you can check the length of inlineInvokePlugins
. Seems cleaner that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
InvocationResult inline; | ||
|
||
if (analysis) { | ||
inline = ImageSingletons.lookup(NativeImageInlineDuringParsingPlugin.class).get(b.getMethod(), b.bci(), callee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ImageSingletons.lookup(NativeImageInlineDuringParsingPlugin.class)
pattern repets several times. A NativeImageInlineDuringParsingPlugin.singleton()
method that returns the value would improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the suggestion.
78c43cc
to
9b79049
Compare
c244a65
to
a3beed2
Compare
a3beed2
to
38e0026
Compare
e7aa891
to
47baf31
Compare
47baf31
to
e57c1e4
Compare
@@ -350,6 +350,10 @@ public JSRData copy() { | |||
this.successors = new ArrayList<>(); | |||
} | |||
|
|||
public boolean bciNotUnique() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: avoid negated methods names, so name the method bciUnique
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -3958,6 +3973,11 @@ public int bci() { | |||
return stream.currentBCI(); | |||
} | |||
|
|||
@Override | |||
public boolean hasBciDuplication() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: method name here and in BlockMap
should match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1134,6 +1139,11 @@ public static void registerGraphBuilderPlugins(FeatureHandler featureHandler, Ru | |||
SubstrateReplacements replacements = (SubstrateReplacements) providers.getReplacements(); | |||
plugins.appendInlineInvokePlugin(replacements); | |||
|
|||
if (nativeImageInlineDuringParsingEnabled()) { | |||
NativeImageInlineDuringParsingPlugin nativeImageInlineDuringParsingPlugin = new NativeImageInlineDuringParsingPlugin(analysis, providers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: not having a local variable here actually makes the line shorter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
return b.getCallingContext().stream().map(Pair::getLeft).anyMatch(caller -> caller.equals(callee)); | ||
} | ||
|
||
private static boolean hasInvokeArgument(ValueNode[] args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does an invoke preclude inlining? That needs at least a good comment for documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually unnecessary because we will certainly not inline method if there is an invoke node in the graph. I removed this.
|
||
private static boolean hasInvokeArgument(ValueNode[] args) { | ||
for (ValueNode arg : args) { | ||
if (arg instanceof InvokeNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InvokeNode
is not general enough because there is also InvokeWithExceptionNode
. You need to check for the interface Invoke
to capture both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the whole method.
} catch (Throwable ex) { | ||
debug.dump(DebugContext.VERBOSE_LEVEL, graph, "InlineDuringParsingAnalysis failed with %s", ex); | ||
/* | ||
* Whatever happens during the analysis is non-fatal because we can just not inline that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is certainly a safe policy for production, but also hides stupid programming errors (like a NullPointerException
somewhere in the analysis). So maybe not ignore exceptions when assertions are enabled in the image builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
} else if (node instanceof StoreFieldNode) { | ||
/* | ||
* We don't inline this method but go further in the analysis to check if any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this handling of StoreFieldNode
. Why "go further in the analysis" if in the end the result is "not inline"? analyzeGraph
just returns false
when this point was reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it is unnecessary to go further in the analysis in this case. I removed it.
cbd4ea0
to
02a2c3d
Compare
} | ||
|
||
CallSite callSite = new CallSite(b.getCallingContext(), NativeImageInlineDuringParsingPlugin.toAnalysisMethod(callee)); | ||
InvocationResult inline = analyzeMethod(callSite, (AnalysisMethod) callee, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we call here analyzeMethod
, we can create infinite recursion. I don't think we should make a new graph in this case, but rather reuse only the analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added fix for this.
} | ||
|
||
private static boolean filterStringConcatGeneratedMethods(ResolvedJavaMethod method) { | ||
return method.format("%H.%n").equals("java.lang.String.valueOf") || method.format("%H.%n").equals("java.lang.StringConcatHelper.prepend") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of code is OK during testing, however, it is absolutely not OK in production due to style, performance, and correctness issues. This kind of code, makes people feel bad about the whole approach and the reviewer gets serious doubts about the credibility of the whole approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we cannot really merge that. But the proper fix should be really simple: while we are in a graph builder plugin (or any kind of intrinsic context), BytecodeParser.bciUnique
needs to return false
because any plugin can lead to more than one "replaced invoke" being produced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added fix for this and removed method filterStringConcatGeneratedMethods.
I am OK that this makes it into 21.0 as it is useful for experimenting with reflection, however:
|
|
||
public static class Options { | ||
@Option(help = "Inline methods which folds to constant during parsing before the static analysis.")// | ||
public static final HostedOptionKey<Boolean> InlineBeforeAnalysis = new HostedOptionKey<>(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not enable the option by default in this PR, then the option help text should start with "Experimental: "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Hello marjanasolajic, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address 47460123+marjanasolajic -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
d60f733
to
d2cde83
Compare
35828cd
to
2ecab68
Compare
d270edb
to
cc1b020
Compare
NativeImageInlineDuringParsingPlugin is a plugin that analyses the graph for the resolved Java method and specifies
what should be inlined during graph parsing before the static analysis. This plugin inlines methods that fold to a constant before the static analysis.
The native-image option for this plugin: -H:+InlineBeforeAnalysis.
The native-image option for inlining depth that is used in this plugin: -H:InlineBeforeAnalysisMaxDepth=value.
Default value for inlining depth is 9.
The performance impact of this plugin on analysis time and image size for benchmark suite renaissance is presented in the next documents.