Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Do not recompute stack frames when instrumenting bytecode.

It turns out that we do not need to do that. See comment in
`ProfilerVisitor.java`. Also, since recomputing stack frame
map was the only reason we needed to implement
`getCommonSuperClass` we can now remove its implementation
that was causing problems on Java 7 due to a cyclic dependency
involving class loader because we would try to load a class
we are currently transforming and transformer is triggered just
before classloading.

//cc @namin who worked on this code with me.
  • Loading branch information...
commit a9bbfec8d58f68bd9105789754373f205d9981b1 1 parent b2776b4
@gkossakowski gkossakowski authored paulp committed
View
33 src/partest/scala/tools/partest/javaagent/ASMTransformer.java
@@ -26,33 +26,16 @@ private boolean shouldTransform(String className) {
className.startsWith("instrumented/"));
}
- public byte[] transform(final ClassLoader classLoader, String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) {
+ public byte[] transform(final ClassLoader classLoader, final String className, Class<?> classBeingRedefined, ProtectionDomain protectionDomain, byte[] classfileBuffer) {
if (shouldTransform(className)) {
- ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_FRAMES | ClassWriter.COMPUTE_MAXS) {
- // this is copied verbatim from the superclass,
- // except that we use the outer class loader
+ ClassWriter writer = new ClassWriter(ClassWriter.COMPUTE_MAXS) {
@Override protected String getCommonSuperClass(final String type1, final String type2) {
- Class<?> c, d;
- try {
- c = Class.forName(type1.replace('/', '.'), false, classLoader);
- d = Class.forName(type2.replace('/', '.'), false, classLoader);
- } catch (Exception e) {
- throw new RuntimeException(e.toString());
- }
- if (c.isAssignableFrom(d)) {
- return type1;
- }
- if (d.isAssignableFrom(c)) {
- return type2;
- }
- if (c.isInterface() || d.isInterface()) {
- return "java/lang/Object";
- } else {
- do {
- c = c.getSuperclass();
- } while (!c.isAssignableFrom(d));
- return c.getName().replace('.', '/');
- }
+ // Since we are not recomputing stack frame map, this should never be called we override this method because
+ // default implementation uses reflection for implementation and might try to load the class that we are
+ // currently processing. That leads to weird results like swallowed exceptions and classes being not
+ // transformed.
+ throw new RuntimeException("Unexpected call to getCommonSuperClass(" + type1 + ", " + type2 +
+ ") while transforming " + className);
}
};
ProfilerVisitor visitor = new ProfilerVisitor(writer);
View
13 src/partest/scala/tools/partest/javaagent/ProfilerVisitor.java
@@ -33,6 +33,19 @@ public MethodVisitor visitMethod(int access, String name, String desc, String si
// only instrument non-abstract methods
if((access & ACC_ABSTRACT) == 0) {
assert(className != null);
+ /* The following instructions do not modify compressed stack frame map so
+ * we don't need to worry about recalculating stack frame map. Specifically,
+ * let's quote "ASM 4.0, A Java bytecode engineering library" guide (p. 40):
+ *
+ * In order to save space, a compiled method does not contain one frame per
+ * instruction: in fact it contains only the frames for the instructions
+ * that correspond to jump targets or exception handlers, or that follow
+ * unconditional jump instructions. Indeed the other frames can be easily
+ * and quickly inferred from these ones.
+ *
+ * Instructions below are just loading constants and calling a method so according
+ * to definition above they do not contribute to compressed stack frame map.
+ */
mv.visitLdcInsn(className);
mv.visitLdcInsn(name);
mv.visitLdcInsn(desc);
Please sign in to comment.
Something went wrong with that request. Please try again.