Skip to content

8367949: JFR: MethodTrace double-counts methods that catch their own exceptions #28947

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

Open
egahlin wants to merge 4 commits into
base: master
Choose a base branch
from
+360 −30
Open

8367949: JFR: MethodTrace double-counts methods that catch their own exceptions #28947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
144fd66
Initial
Dec 21, 2025
34f35ff
Fix whitespaces
Dec 21, 2025
0e862cc
Fix trailing whitespaec
Dec 21, 2025
6b2473d
Trailing whitespace
Dec 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/Instrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,13 @@ public Instrumentation(ClassLoader classLoader, String internalClassName, byte[]
}

public void addMethod(long methodId, String name, String signature, int modification) {
modificationMap.put(name + signature, new Method(methodId, Modification.valueOf(modification), className + "::" + name));
Method method = new Method(
methodId,
Modification.valueOf(modification),
name.equals("<init>"),
className + "::" + name
);
modificationMap.put(name + signature, method);
}

public List<Method> getMethods() {
Expand All @@ -71,7 +77,7 @@ public byte[] generateBytecode() {
ClassModel classModel = classFile.parse(bytecode);
byte[] generated = classFile.build(classModel.thisClass().asSymbol(), classBuilder -> {
for (var ce : classModel) {
if (modifyClassElement(classBuilder, ce)) {
if (modifyClassElement(classModel,classBuilder, ce)) {
modified[0] = true;
} else {
classBuilder.with(ce);
Expand All @@ -93,7 +99,7 @@ private ClassHierarchyResolver resolver() {
}
}

private boolean modifyClassElement(ClassBuilder classBuilder, ClassElement ce) {
private boolean modifyClassElement(ClassModel classModel, ClassBuilder classBuilder, ClassElement ce) {
if (ce instanceof MethodModel mm) {
String method = mm.methodName().stringValue();
String signature = mm.methodType().stringValue();
Expand All @@ -102,25 +108,25 @@ private boolean modifyClassElement(ClassBuilder classBuilder, ClassElement ce) {
if (tm != null) {
Modification m = tm.modification();
if (m.tracing() || m.timing()) {
return modifyMethod(classBuilder, mm, tm);
return modifyMethod(classModel,classBuilder, mm, tm);
}
}
}
return false;
}

private boolean modifyMethod(ClassBuilder classBuilder, MethodModel m, Method method) {
var code = m.code();
private boolean modifyMethod(ClassModel classModel, ClassBuilder classBuilder, MethodModel methodModel, Method method) {
var code = methodModel.code();
if (code.isPresent()) {
if (classLoader == null && ExcludeList.containsMethod(method.name())) {
String msg = "Risk of recursion, skipping bytecode generation of " + method.name();
Logger.log(LogTag.JFR_METHODTRACE, LogLevel.DEBUG, msg);
return false;
}
MethodTransform s = MethodTransform.ofStateful(
() -> MethodTransform.transformingCode(new Transform(method))
() -> MethodTransform.transformingCode(new Transform(classModel, code.get(), method))
);
classBuilder.transformMethod(m, s);
classBuilder.transformMethod(methodModel, s);
return true;
}
return false;
Expand Down
2 changes: 1 addition & 1 deletion src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/Method.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
/**
* Class that holds information about an instrumented method.
*/
record Method(long methodId, Modification modification, String name) {
record Method(long methodId, Modification modification, boolean constructor, String name) {
@Override
public String toString() {
return name + (modification.timing() ? " +timing" : " -timing") + (modification.tracing() ? " +tracing" : " -tracing") + " (Method ID: " + String.format("0x%08X)", methodId);
Expand Down
195 changes: 174 additions & 21 deletions src/jdk.jfr/share/classes/jdk/jfr/internal/tracing/Transform.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,19 @@
*/
package jdk.jfr.internal.tracing;

import java.lang.classfile.ClassModel;
import java.lang.classfile.CodeBuilder;
import java.lang.classfile.CodeElement;
import java.lang.classfile.CodeModel;
import java.lang.classfile.CodeTransform;
import java.lang.classfile.Label;
import java.lang.classfile.TypeKind;
import java.lang.classfile.instruction.InvokeInstruction;
import java.lang.classfile.instruction.ReturnInstruction;
import java.lang.classfile.instruction.ThrowInstruction;
import java.lang.constant.ClassDesc;
import java.util.ArrayList;
import java.util.List;

import jdk.jfr.internal.util.Bytecode;
import jdk.jfr.internal.util.Bytecode.MethodDesc;
Expand All @@ -43,59 +49,206 @@
* The method ID is determined by native code.
*/
final class Transform implements CodeTransform {
private static class TryBlock {
Label start;
Label end;
}
private static final ClassDesc METHOD_TRACER_CLASS = ClassDesc.of(MethodTracer.class.getName());
private static final MethodDesc TRACE_METHOD = MethodDesc.of("trace", "(JJ)V");
private static final MethodDesc TIMING_METHOD = MethodDesc.of("timing", "(JJ)V");
private static final MethodDesc TRACE_TIMING_METHOD = MethodDesc.of("traceTiming", "(JJ)V");
private static final MethodDesc TIMESTAMP_METHOD = MethodDesc.of("timestamp", "()J");

private final List<TryBlock> tryBlocks = new ArrayList<>();
private final boolean simplifiedInstrumentation;
private final ClassModel classModel;
private final Method method;
private int timestampSlot = -1;

Transform(Method method) {
Transform(ClassModel classModel, CodeModel model, Method method) {
this.method = method;
this.classModel = classModel;
// The JVMS (not the JLS) allows multiple mutually exclusive super/this.<init>
// invocations in a constructor body as long as only one lies on any given
// execution path. For example, this is valid bytecode:
//
// Foo(boolean value) {
// if (value) {
// staticMethodThatMayThrow();
// super();
// } else {
// try {
// if (value == 0) {
// throw new Exception("");
// }
// } catch (Throwable t) {
// throw t;
// }
// super();
// }
// }
//
// If such a method is found, instrumentation falls back to instrumenting only
// RET and ATHROW. This can cause exceptions to be missed or counted twice.
//
// An effect of this heuristic is that constructors like the one below
// will also trigger simplified instrumentation.
//
// class Bar {
// }
//
// class Foo extends Bar {
// Foo() {
// new Bar();
// }
// }
this.simplifiedInstrumentation = method.constructor() && constructorInvocations(model.elementList()) > 1;
}

private int constructorInvocations(List<CodeElement> elementList) {
int count = 0;
for (CodeElement e : elementList) {
if (isConstructorInvocation(e)) {
count++;
}
}
return count;
}

private boolean isConstructorInvocation(CodeElement element) {
if (element instanceof InvokeInstruction inv && inv.name().equalsString("<init>")) {
if (classModel.thisClass().equals(inv.owner())) {
return true;
}
if (classModel.superclass().isPresent()) {
return classModel.superclass().get().equals(inv.owner());
}
}
return false;
}

@Override
public final void accept(CodeBuilder builder, CodeElement element) {
public void accept(CodeBuilder builder, CodeElement element) {
if (simplifiedInstrumentation) {
acceptSimplifiedInstrumentation(builder, element);
return;
}
if (method.constructor()) {
acceptConstructor(builder, element, isConstructorInvocation(element));
} else {
acceptMethod(builder, element);
}
}

@Override
public void atEnd(CodeBuilder builder) {
endTryBlock(builder);
for (TryBlock block : tryBlocks) {
addCatchHandler(block, builder);
}
}

private void acceptConstructor(CodeBuilder builder, CodeElement element, boolean isConstructorInvocation) {
if (timestampSlot == -1) {
timestampSlot = invokeTimestamp(builder);
builder.lstore(timestampSlot);
if (!isConstructorInvocation) {
beginTryBlock(builder);
}
}
if (isConstructorInvocation) {
endTryBlock(builder);
builder.with(element);
beginTryBlock(builder);
return;
}
if (element instanceof ReturnInstruction) {
addTracing(builder);
}
builder.with(element);
}

private void endTryBlock(CodeBuilder builder) {
if (tryBlocks.isEmpty()) {
return;
}
TryBlock last = tryBlocks.getLast();
if (tryBlocks.getLast().end == null) {
last.end = builder.newBoundLabel();
}
}

private void beginTryBlock(CodeBuilder builder) {
TryBlock block = new TryBlock();
block.start = builder.newBoundLabel();
tryBlocks.add(block);
}

private void acceptSimplifiedInstrumentation(CodeBuilder builder, CodeElement element) {
if (timestampSlot == -1) {
timestampSlot = invokeTimestamp(builder);
builder.lstore(timestampSlot);
}
if (element instanceof ReturnInstruction || element instanceof ThrowInstruction) {
builder.lload(timestampSlot);
builder.ldc(method.methodId());
Modification modification = method.modification();
boolean objectInit = method.name().equals("java.lang.Object::<init>");
String suffix = objectInit ? "ObjectInit" : "";
if (modification.timing()) {
if (modification.tracing()) {
invokeTraceTiming(builder, suffix);
} else {
invokeTiming(builder, suffix);
}
addTracing(builder);
}
builder.with(element);
}

private void acceptMethod(CodeBuilder builder, CodeElement element) {
if (timestampSlot == -1) {
timestampSlot = invokeTimestamp(builder);
builder.lstore(timestampSlot);
beginTryBlock(builder);
}
if (element instanceof ReturnInstruction) {
addTracing(builder);
}
builder.with(element);
}

private void addCatchHandler(TryBlock block, CodeBuilder builder) {
Label catchHandler = builder.newBoundLabel();
int exceptionSlot = builder.allocateLocal(TypeKind.REFERENCE);
builder.astore(exceptionSlot);
addTracing(builder);
builder.aload(exceptionSlot);
builder.athrow();
builder.exceptionCatchAll(block.start, block.end, catchHandler);
}

private void addTracing(CodeBuilder builder) {
builder.lload(timestampSlot);
builder.ldc(method.methodId());
Modification modification = method.modification();
boolean objectInit = method.name().equals("java.lang.Object::<init>");
String suffix = objectInit ? "ObjectInit" : "";
if (modification.timing()) {
if (modification.tracing()) {
invokeTraceTiming(builder, suffix);
} else {
if (modification.tracing()) {
invokeTrace(builder, suffix);
}
invokeTiming(builder, suffix);
}
} else {
if (modification.tracing()) {
invokeTrace(builder, suffix);
}
}
builder.with(element);
}

public static void invokeTiming(CodeBuilder builder, String suffix) {
private static void invokeTiming(CodeBuilder builder, String suffix) {
builder.invokestatic(METHOD_TRACER_CLASS, TIMING_METHOD.name() + suffix, TIMING_METHOD.descriptor());
}

public static void invokeTrace(CodeBuilder builder, String suffix) {
private static void invokeTrace(CodeBuilder builder, String suffix) {
builder.invokestatic(METHOD_TRACER_CLASS, TRACE_METHOD.name() + suffix, TRACE_METHOD.descriptor());
}

public static void invokeTraceTiming(CodeBuilder builder, String suffix) {
private static void invokeTraceTiming(CodeBuilder builder, String suffix) {
builder.invokestatic(METHOD_TRACER_CLASS, TRACE_TIMING_METHOD.name() + suffix, TRACE_TIMING_METHOD.descriptor());
}

public static int invokeTimestamp(CodeBuilder builder) {
private static int invokeTimestamp(CodeBuilder builder) {
Bytecode.invokestatic(builder, METHOD_TRACER_CLASS, TIMESTAMP_METHOD);
return builder.allocateLocal(TypeKind.LONG);
}
Expand Down
Loading