[pde-ui-dev] PDE API tools and ASM bytecode manipulation framework

[Eugene Kuleshov <eu@xxxxxxxx>]

Hi everyone,

 Chris Aniszczyk mentioned another day that PDE API tools are using ASM 
framework. Since I am one of the developers of ASM, I thought it would 
be useful to share my thoughts and comments about PDE usage of ASM as 
well as few questions that could help me to get better understanding of 
your implementation and use cases. My observations are more or less 
random and mostly based on a quick scanning trough ASM API calls and 
don't assume deep analysis of the PDE implementation. Please don't take 
those notes as criticism, those simply suggestions for better usage of 
the ASM API that could help to improve performance and memory usage for 
your application. I think it is really great application for ASM library 
and I am very excited about it.

 First of all I see that PDE API tools is using "asm" and "asm-tree" 
jars. However "asm-tree" jar is usually used for complex in-memory class 
transformations and method analysis, which doesn't seem the case for PDE 
API tools. I found two uses of the "tree" classes and they both can be 
removed: org.eclipse.pde.api.tools.internal.comparator.TypeDescriptor 
and org.eclipse.pde.api.tools.internal.search.ClassFileVisitor
 Basically those classes are making inefficient use of ClassNode, where 
it can be replaced either with org.objectweb.asm.commons.EmptyVisitor 
from "asm-commons" jar or your own dummy implementation of all ASM's 
visitor interfaces, like the 
org.eclipse.pde.api.tools.internal.util.ClassVisitorAdapter you have. 
That would improve processing performance and eliminate unnecessary 
memory allocation.

 If I understood TypeDescriptor.initialize() method correctly, it is 
not interested in the method code, so you could use 
classReader.accept(visitor, ClassReader.SKIP_CODE); to completely skip 
all methods code from visiting. Same applies to implementation of 
SearchEngine.getExtraction(..) and TagScanner.Visitor.getMethods(..) 
methods, where you also can add ClassReader.SKIP_CODE to avoid visiting 
method code.

 There is a feature in ASM that allows to skip unneeded methods and 
other class artifacts. So you can return a null from visitMethod() call, 
then method code will be also skipped (that also happens when you visit 
an abstract or native method). For example, in 
Converter.MyClassFileAdapter.visitMethod() you could rewrite the 
following code:

           MethodVisitor visitor = super.visitMethod(accessFlags, 
methodName, desc, signature, exceptions);
           if (visitor != null) {
               if (reportRefs) {
                   visitor = new MyMethodAdapter(visitor);
               } else {
                   visitor = new ClearCodeAttributeMethodAdapter(visitor);
               }
           }
           return visitor;

 like this :

           MethodVisitor visitor = super.visitMethod(accessFlags, 
methodName, desc, signature, exceptions);
           if (visitor != null) {
               if (reportRefs) {
                   visitor = new MyMethodAdapter(visitor);
               } else {
                   visitor.visitEnd();  // for safety
                   visitor = null;
               }
           }
           return visitor;

 The above essentially eliminating ClearCodeAttributeMethodAdapter, 
which should probably have been implementing MethodVisitor directly 
instead of extending MethodAdapter, or 
org.objectweb.asm.commons.EmptyVisitor should have been used instead of it.

 If none of your visitors actually cares about StackMap information, 
you may as well add ClassReader.SKIP_FRAMES flag to 
ClassReader.accept(..) call when reading or transforming classes.

 It is better to not extend ClassAdapter when visitor doesn't do any 
transformations and it is used only to collect data, like 
ClassFileDescriptorBuilder does. I would suggest to use something like 
EmptyVisitor for that. Though it is worth to mention that 
org.objectweb.asm.commons.EmptyVisitor is flattening the visiting 
events, i.e. it don't make difference between method and fields 
annotations, but you can return new instance of the EmptyVisitor 
subclass if you need to keep hierarchy.
 Also, code in ClassFileDescriptorBuilder.visitMethod() that extracts 
value for default annotation look fairly similar to the code in  
org.objectweb.asm.util.TraceAnnotationVisitor class from ASM's 
"asm-util" jar, but your version doesn't seem add separator between 
array entries.

 In ClassFileVisitor.visitMethod(..) method. The following code:
   switch(type.getSort()) {
     case Type.LONG :
     case Type.DOUBLE :
       argumentcount += 2;
    default:
       argumentcount++;
   }

 can be replaced with: argumentcount += type.getSize();

 The state machine used in Converter.MyMethodAdapter (passing 
stringLiteral between visitLdcInsn() and visitMethodInsn() methods) is 
probably going to work for classes compiled by javac or JDT, but there 
could be legal bytecode that won't fit into that pattern, but I am not 
sure what is the implication of that for PDE API tools.

 I think I understand what MyMethodAdapter does about reference types 
(which seem very neat idea), but I'd be interested to learn why you have 
special processing for ASTORE, IRETURN, RETURN, ATHROW and POP opcodes 
and not all INVOKE* opcodes?

 In Converter.MyClassFileAdapter.visit(..) it is better to use 
Opcodes.V1_5 instead of magic number "49". Though I wonder why you need 
to use that version instead of version from the original class?
 In the same method, it might be better to use annotation instead of 
custom attribute. Annotations are usually easier to access (in the 
bytecode as well as trough reflectio API) and you can still store them 
into classes with version <49. Though that is obviously matter of 
personal preferences.
 I also curious what Converter.MyMethodWriter is used for? I don't see 
any use of collected lastOffset value.

 Hope you would find these notes somehow useful. Please let me know if 
I could be at any help in regards to ASM framework and thanks again for 
using it.

 Eugene