Transform lambda classes #4182
Transform lambda classes #4182
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| if (("spinInnerClass".equals(name) || "generateInnerClass".equals(name)) | ||
| && "()Ljava/lang/Class;".equals(descriptor)) { | ||
| mv = | ||
| new MethodVisitor(api, mv) { | ||
| @Override | ||
| public void visitMethodInsn( | ||
| int opcode, String owner, String name, String descriptor, boolean isInterface) { | ||
| super.visitMethodInsn(opcode, owner, name, descriptor, isInterface); | ||
| // look for a call to ASM ClassWriter.toByteArray() and insert transformation | ||
| // after it | ||
| if (opcode == Opcodes.INVOKEVIRTUAL | ||
| && "toByteArray".equals(name) | ||
| && "()[B".equals(descriptor)) { | ||
| mv.visitVarInsn(Opcodes.ALOAD, 0); | ||
| mv.visitFieldInsn( | ||
| Opcodes.GETFIELD, slashClassName, "lambdaClassName", "Ljava/lang/String;"); | ||
| mv.visitVarInsn(Opcodes.ALOAD, 0); | ||
| // targetClass is used to get the ClassLoader where lambda class will be defined | ||
| mv.visitFieldInsn( | ||
| Opcodes.GETFIELD, slashClassName, "targetClass", "Ljava/lang/Class;"); | ||
| mv.visitMethodInsn( | ||
| Opcodes.INVOKESTATIC, | ||
| Type.getInternalName(LambdaTransformer.class), | ||
| "transform", | ||
| "([BLjava/lang/String;Ljava/lang/Class;)[B", | ||
| false); | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
I ❤️ this.
At the same time I'm a little nervous about it.
I have a lot of confidence in our tests to catch problems, and I'm not worried if this doesn't match/apply in a new Java release, because we'll catch that and fix it similar to when new instrumented library is released that breaks instrumentation.
But I'm a little concerned about old Java versions that we don't test, say old Java 8 versions, or EOL'd Java 9, and this is replacing pretty important instrumentation (though I think it shouldn't break applications if it doesn't match/apply, so that part is good, plus those applications should definitely upgrade their Java versions 😄).
This concern is similar to old instrumented library versions that we don't test, though in those cases we have muzzle range detection to help confidence level.
Maybe a comment linking to the InnerClassLambdaMetafactory git history mentioning that we've manually verified that the conditions needed for this to match/apply are present across all Java 8+ versions(?)
Also it may (or may not) be worth adding Java 8u0 to our test matrix, similar to how we run tests against the oldest instrumented library version that we support.
Let's chat in the SIG meeting later today and see what others think. I can definitely be convinced, would love to not worry anymore about lambdas not getting instrumented (I don't know if we benefit from this anymore in our netty instrumentation given recent work, but it certainly has caused issues there in the past).
There was a problem hiding this comment.
and this is definitely a big concrete win:
[currently]
ContextStorein lambda class is backed by shared weak map, so care must be taken that value in map does not reference the key as otherwise there is a memory leak
There was a problem hiding this comment.
@laurit convinced me in SIG meeting that this is a safe (as these things go..) and proven approach
Also it may (or may not) be worth adding Java 8u0 to our test matrix, similar to how we run tests against the oldest instrumented library version that we support.
I'll open an issue to track/discuss if this is worth it or not.
| if (("spinInnerClass".equals(name) || "generateInnerClass".equals(name)) | ||
| && "()Ljava/lang/Class;".equals(descriptor)) { | ||
| mv = | ||
| new MethodVisitor(api, mv) { |
There was a problem hiding this comment.
Can you add a comment on why we can't use an Advice method (assuming we can't)?
There was a problem hiding this comment.
When I wrote this I didn't realize that actually advice could be used. For this we would need to instrument spinInnerClass to enter/exit a ThreadLocal and toByteArray of ClassWriter bundled in jdk to only transform when ThreadLocal is set. I do like the asm way a bit more because it is straightforward. Do you think it would be better to use advice for this?
There was a problem hiding this comment.
Advice is nice for making this instrumentation more consistent with our others. But if it adds significant complexity it's not worth it, a comment just describing that is fine.
|
💯 |
As lambda classes don't pass through
ClassFileTransformerthey aren't transformed as other classes and may require special handling (e.g.ContextStorein lambda class is backed by shared weak map, so care must be taken that value in map does not reference the key as otherwise there is a memory leak). This pr instruments jdk to capture bytes for lambda classes and passes them through our transformers so that lambda classes would behave as other classes.