Skip to content

Instantly share code, notes, and snippets.

@amaembo
Created July 21, 2021 17:08
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save amaembo/40c57d7dd423441e9673653f15e76c9d to your computer and use it in GitHub Desktop.
Save amaembo/40c57d7dd423441e9673653f15e76c9d to your computer and use it in GitHub Desktop.
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();
}
@amaembo
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link

Markvy commented Aug 15, 2021

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

@amaembo
Copy link
Author

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