-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
8295537: Enhance TRACE_METHOD_LINKAGE to show the target MethodHandle #10842
Conversation
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
Webrevs
|
/csr needed |
@dholmes-ora has indicated that a compatibility and specification (CSR) request is needed for this pull request. @iklam please create a CSR request for issue JDK-8295537 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
/csr notneeded These tracing flags are considered "diagnostic" and don't need a CSR request. Sorry for the noise. |
@dholmes-ora usage: |
/csr unneeded |
@dholmes-ora determined that a CSR request is not needed for this pull request. |
if (indentLevel <= 0) { | ||
return ""; | ||
} | ||
return new String(new char[indentLevel*4]).replace('\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.
I guess this could be:
return new String(new char[indentLevel*4]).replace('\0', ' '); | |
return " ".repeat(indentLevel); |
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.
Alternatively, I was gonna suggest not having a prefix/indentLevel and instead calling String::indent
at the use site. But, that method uses streams/lambdas, so there might be boostrap cycle issues.
@@ -116,6 +176,16 @@ abstract sealed class CallSite permits ConstantCallSite, MutableCallSite, Volati | |||
CallSite(MethodHandle target) { | |||
target.type(); // null check | |||
this.target = target; | |||
if (MethodHandleStatics.TRACE_CALLSITE) { |
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 doesn't seem like the right place to put this. Anyone can indirectly extend CallSite, by extending e.g. MutableCallSite, but this code seems to assume that the call is going through MethodHandleNatives.linkCallSite
? I suggest putting any tracing around that area. Both the target method handle, and the BSM are available at that point in the code as well.
I also note that there's already a TRACE_METHOD_LINKAGE
flag as well, which does some tracing of call site linking. Maybe that option could be enhanced instead of adding a new one?
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.
Thanks for the suggestion! I removed the new property and just piggyback on TRACE_METHOD_LINKAGE
. I also moved the tracing code into MethodHandleNatives.linkCallSiteXXX
, and improved the existing code a bit to print out the line number of the call site.
Here's what it looks like now:
$ java -Djava.lang.invoke.MethodHandle.TRACE_METHOD_LINKAGE=true -cp . StrConcat
[...]
linkCallSite StrConcat.main(StrConcat.java:6) java.lang.invoke.StringConcatFactory.makeConcatWithConstants(Lookup,String,MethodType,String,Object[])CallSite/invokeStatic makeConcatWithConstants(String)String/BSA1=World
linkCallSite target class => java.lang.invoke.BoundMethodHandle$Species_LL
linkCallSite target => (String)String : invoke001_LL_L=Lambda(a0:L/SpeciesData[LL => BoundMethodHandle$Species_LL],a1:L)=>{
t2:L=BoundMethodHandle$Species_LL.argL1(a0:L);
t3:L=BoundMethodHandle$Species_LL.argL0(a0:L);
t4:L=MethodHandle.invokeBasic(t3:L,a1:L,t2:L);t4:L}
& BMH=[
0: MethodHandle = {(Object,Object)String : DMH.invokeStatic000_LLL_L=Lambda(a0:L,a1:L,a2:L)=>{
t3:L=DirectMethodHandle.internalMemberName(a0:L);
t4:L=MethodHandle.linkToStatic(a1:L,a2:L,t3:L);t4:L}
& DMH.MN=java.lang.StringConcatHelper.simpleConcat(Object,Object)String/invokeStatic
}
1: ( World )
]
linkCallSite linkage => java.lang.invoke.Invokers$Holder.linkToTargetMethod(Object,Object)Object/invokeStatic + MethodHandle(String)String
HelloWorld
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 changes look good, but I have some more comments.
String callerName = caller.getName(); | ||
String[] callerInfo = new String[] {callerName}; | ||
StackWalker.getInstance().walk(new Function<Stream<StackWalker.StackFrame>, Object>() { | ||
// We use inner classes (instead of stream/lambda) to avoid triggering | ||
// further invokedynamic resolution, which would cause infinite recursion. | ||
// It's OK to use + for string concat, because java.base is compiled without | ||
// the use of indy string concat. | ||
@Override | ||
public Object apply(Stream<StackWalker.StackFrame> s) { | ||
s.forEach(new Consumer<StackWalker.StackFrame>() { | ||
@Override | ||
public void accept(StackWalker.StackFrame f) { | ||
if (!"java.lang.invoke.MethodHandleNatives".equals(f.getClassName()) && callerInfo[0] == callerName) { | ||
callerInfo[0] = f.toStackTraceElement().toString(); | ||
} | ||
} | ||
}); | ||
return 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.
Not sure about this condition. It seems like callerInfo[0] == callerName
will always be true at first, and then it gets overwritten by the stack trace element toString, so in effect it only picks up the first non-MethodHandleNatives caller? I think it could be clearer to use filter
+ findFirst
to get the caller's StackFrame
(that has the additional benefit of short-circuiting), and then convert to a String from there.
Also, please move the code that gets the caller info out-of-line to a helper method, since it's quite wordy.
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.
Alternatively, maybe you could just dump the whole stack trace here (e.g. with Thread.dumpStack()), since it might be useful to see which code path triggers resolution of a call site as well. That would also include the line number of the caller.
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 it possible to use filter/findfirst without using lambdas? I want to avoid recursion inside the tracing code.
I am not sure about dumping the call stack. It seems an overkill and not useful in most cases. It’s easier to rebuild the JDK and add Thread.dumpStack() in the rare occasion that you need to do 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.
Is it possible to use filter/findfirst without using lambdas? I want to avoid recursion inside the tracing code.
You could do this I believe (if I've eye-balled that correctly :)):
String callerName = caller.getName(); | |
String[] callerInfo = new String[] {callerName}; | |
StackWalker.getInstance().walk(new Function<Stream<StackWalker.StackFrame>, Object>() { | |
// We use inner classes (instead of stream/lambda) to avoid triggering | |
// further invokedynamic resolution, which would cause infinite recursion. | |
// It's OK to use + for string concat, because java.base is compiled without | |
// the use of indy string concat. | |
@Override | |
public Object apply(Stream<StackWalker.StackFrame> s) { | |
s.forEach(new Consumer<StackWalker.StackFrame>() { | |
@Override | |
public void accept(StackWalker.StackFrame f) { | |
if (!"java.lang.invoke.MethodHandleNatives".equals(f.getClassName()) && callerInfo[0] == callerName) { | |
callerInfo[0] = f.toStackTraceElement().toString(); | |
} | |
} | |
}); | |
return null; | |
} | |
}); | |
String callerName = caller.getName(); | |
String callerInfo = StackWalker.getInstance().walk(new Function<Stream<StackWalker.StackFrame>, String>() { | |
// We use inner classes (instead of stream/lambda) to avoid triggering | |
// further invokedynamic resolution, which would cause infinite recursion. | |
// It's OK to use + for string concat, because java.base is compiled without | |
// the use of indy string concat. | |
@Override | |
public String apply(Stream<StackWalker.StackFrame> s) { | |
return s.filter(new Predicate<StackWalker.StackFrame>() { | |
@Override | |
public boolean test(StackWalker.StackFrame f) { | |
return callerName.equals(f.getClassName()); | |
} | |
}).findFirst().get().toStackTraceElement().toString(); | |
} | |
}); |
I am not sure about dumping the call stack. It seems an overkill and not useful in most cases. It’s easier to rebuild the JDK and add Thread.dumpStack() in the rare occasion that you need to do this.
Fair enough.
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.
Or, maybe it's easier to use Thread.currentThread().getStackTrace()
and avoid messing around with streams altogether.
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 ended up using Thread.currentThread().getStackTrace()
and the code is much cleaner. Performance isn't important anyway. Stream.filter()
doesn't work because it uses a Lambda in <clinit>
:
java.lang.NullPointerException: Cannot invoke "java.util.stream.TerminalOp.getOpFlags()" because "terminalOp" is null
at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.base/java.util.stream.ReferencePipeline.findFirst(ReferencePipeline.java:647)
at java.base/java.lang.invoke.MethodHandleNatives$1.apply(MethodHandleNatives.java:328)
at java.base/java.lang.invoke.MethodHandleNatives$1.apply(MethodHandleNatives.java:315)
at java.base/java.lang.StackStreamFactory$StackFrameTraverser.consumeFrames(StackStreamFactory.java:586)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.doStackWalk(StackStreamFactory.java:324)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.callStackWalk(Native Method)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.beginStackWalk(StackStreamFactory.java:410)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.walkHelper(StackStreamFactory.java:261)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.walk(StackStreamFactory.java:253)
at java.base/java.lang.StackWalker.walk(StackWalker.java:589)
at java.base/java.lang.invoke.MethodHandleNatives.linkCallSiteTracing(MethodHandleNatives.java:315)
at java.base/java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:275)
at java.base/java.util.stream.FindOps$FindSink$OfRef.<clinit>(FindOps.java:198)
at java.base/java.util.stream.FindOps.makeRef(FindOps.java:60)
at java.base/java.util.stream.ReferencePipeline.findFirst(ReferencePipeline.java:647)
at java.base/java.lang.invoke.MethodHandleNatives$1.apply(MethodHandleNatives.java:328)
at java.base/java.lang.invoke.MethodHandleNatives$1.apply(MethodHandleNatives.java:315)
at java.base/java.lang.StackStreamFactory$StackFrameTraverser.consumeFrames(StackStreamFactory.java:586)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.doStackWalk(StackStreamFactory.java:324)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.callStackWalk(Native Method)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.beginStackWalk(StackStreamFactory.java:410)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.walkHelper(StackStreamFactory.java:261)
at java.base/java.lang.StackStreamFactory$AbstractStackWalker.walk(StackStreamFactory.java:253)
at java.base/java.lang.StackWalker.walk(StackWalker.java:589)
at java.base/java.lang.invoke.MethodHandleNatives.linkCallSiteTracing(MethodHandleNatives.java:315)
at java.base/java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:275)
at java.base/jdk.internal.module.SystemModuleFinders$1.find(SystemModuleFinders.java:216)
at java.base/jdk.internal.module.ModuleBootstrap.boot2(ModuleBootstrap.java:260)
at java.base/jdk.internal.module.ModuleBootstrap.boot(ModuleBootstrap.java:174)
at java.base/java.lang.System.initPhase2(System.java:2214)
System.out.println("linkCallSite "+caller.getName()+" "+ | ||
String callerName = caller.getName(); | ||
String[] callerInfo = new String[] {callerName}; | ||
StackWalker.getInstance().walk(new Function<Stream<StackWalker.StackFrame>, Object>() { |
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 can call StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE)
to retain Class
instance in StackFrame
such that you can compare Class
instance rather than class name.
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 StackWalker
API depends on Streams, which is likely to have lambdas in its implementation. To avoid the possibility of bootstrap problems, I have changed the code to use Thread.currentThread().getStackTrace()
, which is used by Throwable.printStackTrace()
. Since printStackTrace()
is supposed to be useable at very early bootstrap stages, we are sure no lambdas will be used.
Unfortunately getStackTrace()
doesn't return the Class
object, so I have to use the class's name.
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.
Very nice, Thanks!
@iklam This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Thanks @cl4es @JornVernee @mlchung for the review. |
Going to push as commit fd668dc.
Your commit was automatically rebased without conflicts. |
Improve the handling of the
java.lang.invoke.MethodHandle.TRACE_METHOD_LINKAGE
property to print out the full graph of MethodHandles that are used at a CallSite. This helps us understand how invokedynamic call sites are resolved. For example:More complex examples are in the JBS bug report
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10842/head:pull/10842
$ git checkout pull/10842
Update a local copy of the PR:
$ git checkout pull/10842
$ git pull https://git.openjdk.org/jdk pull/10842/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10842
View PR using the GUI difftool:
$ git pr show -t 10842
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10842.diff