Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Patch for InnerClassLambdaMetafactory to generate line number debug info
diff --git a/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java b/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
--- a/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java (revision 57611b30219191160f7faccb811b41a31c25c0b8)
+++ b/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java (date 1626887157914)
@@ -315,6 +315,18 @@
return generateInnerClass();
}
+ private static StackTraceElement getCallerFrame() {
+ StackTraceElement[] trace = new Exception().getStackTrace();
+ for (int i = 0; i < trace.length - 1; i++) {
+ StackTraceElement ste = trace[i];
+ if (ste.getClassName().equals("java.lang.invoke.MethodHandleNatives") &&
+ ste.getMethodName().equals("linkCallSite")) {
+ return trace[i + 1];
+ }
+ }
+ return null;
+ }
+
/**
* Generate a class file which implements the functional
* interface, define and return the class.
@@ -344,6 +356,14 @@
cw.visit(CLASSFILE_VERSION, ACC_SUPER + ACC_FINAL + ACC_SYNTHETIC,
lambdaClassName, null,
JAVA_LANG_OBJECT, interfaceNames);
+ int lineNumber;
+ StackTraceElement ste = getCallerFrame();
+ if (ste != null) {
+ cw.visitSource(ste.getFileName(), null);
+ lineNumber = ste.getLineNumber();
+ } else {
+ lineNumber = -1;
+ }
// Generate final fields to be filled in by constructor
for (int i = 0; i < argDescs.length; i++) {
@@ -363,14 +383,14 @@
// Forward the SAM method
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, interfaceMethodName,
interfaceMethodType.toMethodDescriptorString(), null, null);
- new ForwardingMethodGenerator(mv).generate(interfaceMethodType);
+ new ForwardingMethodGenerator(mv, lineNumber).generate(interfaceMethodType);
// Forward the altMethods
if (altMethods != null) {
for (MethodType mt : altMethods) {
mv = cw.visitMethod(ACC_PUBLIC, interfaceMethodName,
mt.toMethodDescriptorString(), null, null);
- new ForwardingMethodGenerator(mv).generate(mt);
+ new ForwardingMethodGenerator(mv, lineNumber).generate(mt);
}
}
@@ -541,14 +561,19 @@
* method, converting arguments, as needed.
*/
private class ForwardingMethodGenerator extends TypeConvertingMethodAdapter {
+ private final int lineNumber;
- ForwardingMethodGenerator(MethodVisitor mv) {
+ ForwardingMethodGenerator(MethodVisitor mv, int lineNumber) {
super(mv);
+ this.lineNumber = lineNumber;
}
void generate(MethodType methodType) {
visitCode();
+ Label start = new Label();
+ visitLabel(start);
+
if (implKind == MethodHandleInfo.REF_newInvokeSpecial) {
visitTypeInsn(NEW, implMethodClassName);
visitInsn(DUP);
@@ -585,6 +610,9 @@
visitInsn(getReturnOpcode(samReturnClass));
// Maxs computed by ClassWriter.COMPUTE_MAXS,these arguments ignored
visitMaxs(-1, -1);
+ if (lineNumber >= 0) {
+ visitLineNumber(lineNumber, start);
+ }
visitEnd();
}
@Markvy

This comment has been minimized.

Copy link

@Markvy Markvy commented Aug 6, 2021

I read your article on habr where you said that this is so obviously crazy that you're not going to propose adding this to JDK.
But it's not obvious to me. (Maybe I didn't read carefully enough?) Can you explain why this is a bad idea?

@amaembo

This comment has been minimized.

Copy link
Owner Author

@amaembo amaembo commented Aug 6, 2021

I read your article on habr where you said that this is so obviously crazy that you're not going to propose adding this to JDK.
But it's not obvious to me. (Maybe I didn't read carefully enough?) Can you explain why this is a bad idea?

The patch in its current state is quite dirty (e.g., it relies on specific lines in the stack trace). Also, the whole feature limits possible reuse of method-reference runtime representations. There is a dormant proposal in amber project to deduplicate, e.g., repeating Objects::nonNull within the same class. For now, every method reference or lambda is expanded into a separate hidden class at runtime, so deduplication might save memory, code cache, JIT compilation time, etc. However, this direction is totally incompatible with my patch, as I rely on the fact that every method reference has its own runtime representation.

@Markvy

This comment has been minimized.

Copy link

@Markvy Markvy commented Aug 7, 2021

Thanks for the quick response! Re: current state dirty: surely that's fixable if people did decide that this is a feature worth having.
Re: deduplication: I admit that I can't see anyway to achieve deduplication and also get good proper line numbers in stack traces.
Those seem to be inherently in tension, because if you share literally everything, you're also sharing line numbers.

And yet I wonder: is this the right trade off?
For instance, the JVM doesn't do tail call elimination, which would likewise reduce memory usage, but it would make stack traces less useful in more or less the same way that this does.

I feel that the choice to use method references versus lambdas should be based purely on which one makes the code more readable, without having to also keep in mind "well, this way is more efficient, but that way gives better stack traces".
(I would change my mind if it turned out that the efficiency gains were sufficiently massive.)

Oh, and mostly unrelated (feel free to ignore this whole paragraph) but while we're on the topic of efficiency: a lambda can either capture "this" or not: the decision is made by javac examining the body of the lambda. Likewise, an inner class can be a static inner class, or not. But even though javac could make this determination as well, it doesn't, and instead it depends on syntactic details of how the class was declared. (E.g., it's static if you declare it static, or if it's a local class inside a static method.) I wish this could be fixed so that forgetting to add "static" in the right place stops causing pointless memory leaks.

@amaembo

This comment has been minimized.

Copy link
Owner Author

@amaembo amaembo commented Aug 7, 2021

And yet I wonder: is this the right trade off?

Probably not, but I don't feel I have enough energy to persuade JDK folks to accept this. In any case, turning this into a proper JDK patch will require too much time and energy. I'm not against if somebody else will do this :-)

Likewise, an inner class can be a static inner class, or not

This part is under active investigation now, see openjdk/jdk#4966 for details.

@Markvy

This comment has been minimized.

Copy link

@Markvy Markvy commented Aug 9, 2021

Re: static inner classes: this is amazing, I thought it would literally never get fixed. Thanks for the link!
Re: energy to persuade: maybe everyone will just love this idea and no persuasion will be needed?

@Markvy

This comment has been minimized.

Copy link

@Markvy Markvy commented Aug 15, 2021

Do you mind if I post your article to the JDK mailing list?

@amaembo

This comment has been minimized.

Copy link
Owner Author

@amaembo amaembo commented Aug 15, 2021

It's up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment