You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Art Gramlich <ar...@gramlich-net.com> on 2005/02/21 05:39:32 UTC
[javaflow] Transforming classes that implement Continuable
While playing with javaflow, I found out the hard way that the
ContinuationClassLoader
is only transforming java that is in a certain package. I made this
patch to transform
classes that implement Continuable and leave the others alone. This
seems like the
correct way to do it, but I may be missing consequences of doing it
this way.
Art Gramlich
Index:
org/apache/commons/javaflow/bytecode/bcel/BcelClassTransformer.java
===================================================================
---
src/java/org/apache/commons/javaflow/bytecode/bcel/
BcelClassTransformer.java (revision 154619)
+++
src/java/org/apache/commons/javaflow/bytecode/bcel/
BcelClassTransformer.java (working copy)
@@ -100,6 +100,22 @@
private boolean currentMethodStatic = false;
private boolean debug = true;
+
+ private boolean implementsContinuable(JavaClass javaClazz) {
+ boolean continuable = false;
+ try {
+ JavaClass[] interfaces = javaClazz.getAllInterfaces();
+ for (int i=0; i<interfaces.length; i++) {
+ if
(CONTINUABLE_CLASS.equals(interfaces[i].getClassName())) {
+ continuable = true;
+ break;
+ }
+ }
+ } catch (ClassNotFoundException e) {
+ log.error(e.getMessage(),e);
+ }
+ return continuable;
+ }
public byte[] transform(final InputStream is) {
@@ -115,6 +131,10 @@
Repository.addClass(javaClazz);
+ if (!implementsContinuable(javaClazz)) {
+ return javaClazz.getBytes();
+ }
+
log.debug("transforming class " + javaClazz.getClassName());
Index: org/apache/commons/javaflow/ContinuationClassLoader.java
===================================================================
---
src/java/org/apache/commons/javaflow/ContinuationClassLoader.java
(revision 154619)
+++
src/java/org/apache/commons/javaflow/ContinuationClassLoader.java
(working copy)
@@ -15,10 +15,8 @@
*/
package org.apache.commons.javaflow;
-import java.io.IOException;
import java.io.InputStream;
-import org.apache.commons.io.IOUtils;
import org.apache.commons.javaflow.bytecode.ClassTransformer;
import org.apache.commons.javaflow.bytecode.bcel.BcelClassTransformer;
import org.apache.commons.logging.Log;
@@ -56,23 +54,8 @@
final InputStream is =
super.getResourceAsStream(name.replace('.','/') + ".class");
if (is != null) {
-
- if
(name.startsWith("org.apache.commons.javaflow.testcode")) {
- //log.debug("transforming " + name);
-
- final byte data[] = transformer.transform(is);
- return defineClass(name, data, 0, data.length);
- }
-
- try {
- final byte data[] = IOUtils.toByteArray(is);
-
- //log.debug("loading "+ name);
-
- return defineClass(name, data, 0, data.length);
- } catch (final IOException e) {
- throw new ClassNotFoundException("could not
read class", e);
- }
+ final byte data[] = transformer.transform(is);
+ return defineClass(name, data, 0, data.length);
} else {
log.debug("could not find " + name);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [javaflow] Transforming classes that implement Continuable
Posted by Art Gramlich <ar...@gramlich-net.com>.
On Wed, Feb 23, 2005 at 02:40:53PM -0700, Art Gramlich wrote:
> On Tue, Feb 22, 2005 at 08:08:01PM -0500, WHIRLYCOTT wrote:
> > Torsten Curdt wrote:
> >
> > >While I did some work with ASM I found it
> > >very nice to work with ...and so I wanted
> > >get rid of a direct BCEL dependency. Which
> >
> > Fyi, I'm working on porting the current BCEL ClassTransformer over to ASM.
> >
> > >...but for now I think rewriting on the
> > >package name is quite a useable alternative
> > >that probably will fit the real world
> > >requirements.
> >
> > You have mentioned this before, but I don't understand how you envision
> > this working. Does this mean that a class originally in package
> > com.foo.abc would be rewritten to rewritten.package.name by the
> > transformer? I'm fairly sure that I am not interpreting your idea
> > correctly because this will almost certainly cause problems for
> > instances of that class if they need to access package protected methods
> > and fields in other classes. If you could go into a bit of detail on
> > this, it would be super.
>
> Sorry if I butt in. What we were talking about is how to decide is a class
> should be transformed. Right now, the ContinuationClassLoader is checking if
> the package name starts with "org.apache.commons.javaflow.testcode" and transforms
> it. Just for giggles, I just did a little patch to make that configurable (and a set).
> Any opinions?
>
> Art Gramlich
>
>
Sorry, that last bit of code was wrong since it didn't check child packages...
It should be
Index: java/org/apache/commons/javaflow/ContinuationClassLoader.java
===================================================================
--- src/java/org/apache/commons/javaflow/ContinuationClassLoader.java (revision 155031)
+++ src/java/org/apache/commons/javaflow/ContinuationClassLoader.java (working copy)
@@ -17,6 +17,8 @@
import java.io.IOException;
import java.io.InputStream;
+import java.util.Set;
+import java.util.TreeSet;
import org.apache.commons.io.IOUtils;
import org.apache.commons.javaflow.bytecode.ClassTransformer;
@@ -33,66 +35,101 @@
private final static Log log = LogFactory.getLog(ContinuationCompilingClassLoader.class);
private ClassTransformer transformer = new BcelClassTransformer();
+ private Set packagesToTransform,packageRootsToTransform;
+ /*
public ContinuationClassLoader(ClassLoader parent) {
+ this(parent, new HashSet());
+ }
+ */
+
+ public ContinuationClassLoader(ClassLoader parent, Set packageRootsToTransform) {
super(parent);
//Repository.setRepository(new ClassLoaderRepository(parent));
+ this.packageRootsToTransform = packageRootsToTransform;
+ packagesToTransform = new TreeSet(packageRootsToTransform);
+ }
+
+ protected boolean inPackageListOrSubPackage(String name) {
+ int index = name.lastIndexOf('.');
+ String packageName;
+ if (index == -1) {
+ // If the class is in the default package, give the package name a single '.'
+ packageName = ".";
+ } else {
+ packageName = name.substring(0,index);
+ }
+ boolean shouldTransform = packagesToTransform.contains(packageName);
+ //log.debug("Checking if we should transform "+packageName+":"+shouldTransform);
+ if (shouldTransform || packageName.equals(".")) {
+ return shouldTransform;
+ } else {
+ return inPackageListOrSubPackage(packageName);
+ }
+ }
+
+ protected boolean shouldTransform(String name) {
+ boolean should = inPackageListOrSubPackage(name);
+ if (should || !packagesToTransform.contains(name)) {
+ packagesToTransform.add(name);
+ }
+ return should;
}
protected synchronized Class loadClass(String name, boolean resolve)
- throws ClassNotFoundException {
-
- Class c = findLoadedClass(name);
-
- if (c == null) {
- if (!name.startsWith("java.") &&
- !name.startsWith("javax.") &&
- !name.startsWith("sun.") &&
- !name.startsWith("org.xml.") &&
- !name.startsWith("org.w3c.") &&
- !name.startsWith("org.apache.commons.logging.")
- ) {
-
- final InputStream is = super.getResourceAsStream(name.replace('.','/') + ".class");
-
- if (is != null) {
-
- if (name.startsWith("org.apache.commons.javaflow.testcode")) {
- //log.debug("transforming " + name);
-
- final byte data[] = transformer.transform(is);
- return defineClass(name, data, 0, data.length);
- }
-
- try {
- final byte data[] = IOUtils.toByteArray(is);
-
- //log.debug("loading "+ name);
-
- return defineClass(name, data, 0, data.length);
- } catch (final IOException e) {
- throw new ClassNotFoundException("could not read class", e);
- }
- } else {
- log.debug("could not find " + name);
- }
- }
-
- final ClassLoader parent = getParent();
- if (parent != null) {
- //log.debug("parent loading " + name);
- c = parent.loadClass(name);
- } else {
- throw new ClassNotFoundException(name);
- }
-
- } else {
- log.debug("already loaded " + name);
- }
-
- if (resolve) {
- resolveClass(c);
- }
- return c;
+ throws ClassNotFoundException {
+
+ Class c = findLoadedClass(name);
+
+ if (c == null) {
+ if (!name.startsWith("java.") &&
+ !name.startsWith("javax.") &&
+ !name.startsWith("sun.") &&
+ !name.startsWith("org.xml.") &&
+ !name.startsWith("org.w3c.") &&
+ !name.startsWith("org.apache.commons.logging.")
+ ) {
+
+ final InputStream is = super.getResourceAsStream(name.replace('.','/') + ".class");
+
+ if (is != null) {
+
+ if (shouldTransform(name)) {
+ //log.debug("transforming " + name);
+
+ final byte data[] = transformer.transform(is);
+ return defineClass(name, data, 0, data.length);
+ }
+
+ try {
+ final byte data[] = IOUtils.toByteArray(is);
+
+ //log.debug("loading "+ name);
+
+ return defineClass(name, data, 0, data.length);
+ } catch (final IOException e) {
+ throw new ClassNotFoundException("could not read class", e);
+ }
+ } else {
+ log.debug("could not find " + name);
+ }
+ }
+
+ final ClassLoader parent = getParent();
+ if (parent != null) {
+ //log.debug("parent loading " + name);
+ c = parent.loadClass(name);
+ } else {
+ throw new ClassNotFoundException(name);
+ }
+
+ } else {
+ log.debug("already loaded " + name);
+ }
+
+ if (resolve) {
+ resolveClass(c);
+ }
+ return c;
+ }
}
-}
Index: test/org/apache/commons/javaflow/AbstractClassLoaderTestCase.java
===================================================================
--- src/test/org/apache/commons/javaflow/AbstractClassLoaderTestCase.java (revision 155031)
+++ src/test/org/apache/commons/javaflow/AbstractClassLoaderTestCase.java (working copy)
@@ -16,7 +16,9 @@
package org.apache.commons.javaflow;
import java.lang.reflect.Method;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import junit.framework.TestCase;
@@ -35,7 +37,9 @@
}
protected void call(final String methodName) throws Exception {
- final ClassLoader cl = new ContinuationClassLoader(getClass().getClassLoader());
+ Set packages = new HashSet();
+ packages.add("org.apache.commons.javaflow.testcode");
+ final ClassLoader cl = new ContinuationClassLoader(getClass().getClassLoader(), packages);
final Class clazz = cl.loadClass(clazzName);
final Object instance = clazz.newInstance();
assertNotNull(clazz);
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [javaflow] Transforming classes that implement Continuable
Posted by Art Gramlich <ar...@gramlich-net.com>.
On Tue, Feb 22, 2005 at 08:08:01PM -0500, WHIRLYCOTT wrote:
> Torsten Curdt wrote:
>
> >While I did some work with ASM I found it
> >very nice to work with ...and so I wanted
> >get rid of a direct BCEL dependency. Which
>
> Fyi, I'm working on porting the current BCEL ClassTransformer over to ASM.
>
> >...but for now I think rewriting on the
> >package name is quite a useable alternative
> >that probably will fit the real world
> >requirements.
>
> You have mentioned this before, but I don't understand how you envision
> this working. Does this mean that a class originally in package
> com.foo.abc would be rewritten to rewritten.package.name by the
> transformer? I'm fairly sure that I am not interpreting your idea
> correctly because this will almost certainly cause problems for
> instances of that class if they need to access package protected methods
> and fields in other classes. If you could go into a bit of detail on
> this, it would be super.
Sorry if I butt in. What we were talking about is how to decide is a class
should be transformed. Right now, the ContinuationClassLoader is checking if
the package name starts with "org.apache.commons.javaflow.testcode" and transforms
it. Just for giggles, I just did a little patch to make that configurable (and a set).
Any opinions?
Art Gramlich
Index: java/org/apache/commons/javaflow/ContinuationClassLoader.java
===================================================================
--- src/java/org/apache/commons/javaflow/ContinuationClassLoader.java (revision 155031)
+++ src/java/org/apache/commons/javaflow/ContinuationClassLoader.java (working copy)
@@ -17,6 +17,7 @@
import java.io.IOException;
import java.io.InputStream;
+import java.util.Set;
import org.apache.commons.io.IOUtils;
import org.apache.commons.javaflow.bytecode.ClassTransformer;
@@ -33,10 +34,31 @@
private final static Log log = LogFactory.getLog(ContinuationCompilingClassLoader.class);
private ClassTransformer transformer = new BcelClassTransformer();
+ private Set packagesToTransform;
+ /*
public ContinuationClassLoader(ClassLoader parent) {
+ this(parent, new HashSet());
+ }
+ */
+
+ public ContinuationClassLoader(ClassLoader parent, Set packageNamesToTransform) {
super(parent);
//Repository.setRepository(new ClassLoaderRepository(parent));
+ packagesToTransform = packageNamesToTransform;
+ }
+
+ protected boolean shouldTransform(String name) {
+ int index = name.lastIndexOf('.');
+ String packageName;
+ if (index == -1) {
+ // If the class is in the default package, give the package name a single '.'
+ packageName = ".";
+ } else {
+ packageName = name.substring(0,index);
+ }
+ //log.debug("Checking if we should transform "+packageName);
+ return packagesToTransform.contains(packageName);
}
protected synchronized Class loadClass(String name, boolean resolve)
@@ -57,7 +79,7 @@
if (is != null) {
- if (name.startsWith("org.apache.commons.javaflow.testcode")) {
+ if (shouldTransform(name)) {
//log.debug("transforming " + name);
final byte data[] = transformer.transform(is);
Index: test/org/apache/commons/javaflow/AbstractClassLoaderTestCase.java
===================================================================
--- src/test/org/apache/commons/javaflow/AbstractClassLoaderTestCase.java (revision 155031)
+++ src/test/org/apache/commons/javaflow/AbstractClassLoaderTestCase.java (working copy)
@@ -16,7 +16,9 @@
package org.apache.commons.javaflow;
import java.lang.reflect.Method;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import junit.framework.TestCase;
@@ -35,7 +37,9 @@
}
protected void call(final String methodName) throws Exception {
- final ClassLoader cl = new ContinuationClassLoader(getClass().getClassLoader());
+ Set packages = new HashSet();
+ packages.add("org.apache.commons.javaflow.testcode");
+ final ClassLoader cl = new ContinuationClassLoader(getClass().getClassLoader(), packages);
final Class clazz = cl.loadClass(clazzName);
final Object instance = clazz.newInstance();
assertNotNull(clazz);
>
> phil.
>
> --
> Whirlycott
> Philip Jacob
> phil@whirlycott.com
> http://www.whirlycott.com/phil/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [javaflow] Transforming classes that implement Continuable
Posted by Torsten Curdt <tc...@apache.org>.
>> While I did some work with ASM I found it
>> very nice to work with ...and so I wanted
>> get rid of a direct BCEL dependency. Which
>
>
> Fyi, I'm working on porting the current BCEL ClassTransformer over to ASM.
Awesome news! Thanks for the effort!
>> ...but for now I think rewriting on the
>> package name is quite a useable alternative
>> that probably will fit the real world
>> requirements.
>
>
> You have mentioned this before, but I don't understand how you envision
> this working. Does this mean that a class originally in package
> com.foo.abc would be rewritten to rewritten.package.name by the
> transformer? I'm fairly sure that I am not interpreting your idea
> correctly because this will almost certainly cause problems for
> instances of that class if they need to access package protected methods
> and fields in other classes. If you could go into a bit of detail on
> this, it would be super.
Sorry, for the late response...
...yes, you got the idea wrong :)
What I was talking is about defining the packages
that need to be rewritten. Like you define all classes
that start with "com.mystuff.flow." are meant to be
rewritten (as the most simple example).
So basically defining rewriting on the package level.
I think that's fine grained enough for a real world
scenario. ...and it keeps things simple.
cheers
--
Torsten
Re: [javaflow] Transforming classes that implement Continuable
Posted by WHIRLYCOTT <ph...@whirlycott.com>.
Torsten Curdt wrote:
> While I did some work with ASM I found it
> very nice to work with ...and so I wanted
> get rid of a direct BCEL dependency. Which
Fyi, I'm working on porting the current BCEL ClassTransformer over to ASM.
> ...but for now I think rewriting on the
> package name is quite a useable alternative
> that probably will fit the real world
> requirements.
You have mentioned this before, but I don't understand how you envision
this working. Does this mean that a class originally in package
com.foo.abc would be rewritten to rewritten.package.name by the
transformer? I'm fairly sure that I am not interpreting your idea
correctly because this will almost certainly cause problems for
instances of that class if they need to access package protected methods
and fields in other classes. If you could go into a bit of detail on
this, it would be super.
phil.
--
Whirlycott
Philip Jacob
phil@whirlycott.com
http://www.whirlycott.com/phil/
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [javaflow] Transforming classes that implement Continuable
Posted by Art Gramlich <ar...@gramlich-net.com>.
On Feb 21, 2005, at 1:09 AM, Torsten Curdt wrote:
>> While playing with javaflow, I found out the hard way that the
>> ContinuationClassLoader
>> is only transforming java that is in a certain package. I made this
>
> Sorry, about that ....of course that should be made coonfigurable.
>
>> patch to transform
>> classes that implement Continuable and leave the others alone. This
>> seems like the
>> correct way to do it, but I may be missing consequences of doing it
>> this way.
>
> Hehe ...have a look into an older revision
> Just recently I've removed exactly that :)
>
> I am not a big fan of (empty) marker interfaces.
> Plus this has a big impact on the behaviour of
> the BCEL processing.
>
> If you do a javaClazz.getAllInterfaces()
> you need to have a all the depending classes
> and interfaces already in the internal BCEL
> repository.
>
> While I did some work with ASM I found it
> very nice to work with ...and so I wanted
> get rid of a direct BCEL dependency. Which
> also includes accessing the repository from
> javaflow code other than the transformer.
>
> BCEL tries to find and parse unknown classes
> by itself ...which does not really work very
> well in our case. ...with some effort this
> could of course be fixed in BCEL.
>
> ...but for now I think rewriting on the
> package name is quite a useable alternative
> that probably will fit the real world
> requirements.
>
> As long it is configurable ;)
Looks like the path of least resistance. The patch I posted
earlier (the second fixed one) was a quick hack I threw together
to test the idea of passing in root packages to do transformation on.
While looking at it , I noticed that if the test case "tests" are moved
into a
child package, that the entire section where the non-transformed class
is
defined in the current classloader can be removed and these can be
delegated to
the parent loader instead. Again at this point I may be missing
something. Am I
wrong about this?
>
> On the other hand it might be good to do
> a full call flow analysis so we can rewrite
> only those methods that might need it because
> we are calling suspend.
>
> But this would required the full dependencies
> and is quite heavy weight on the load but more
> light weight on the execution. Haven't made my
> mind up on that yet.
>
> WDYT?
Obviously this would be the best implementation. It seems that it would
be very heavyweight though and require a full recompilation of any
dynamic
classes in order to find any calls. Also, couldn't someone get sneaky
and use reflection to call suspend (let's hope not) :-).
>
> cheers
> --
> Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [javaflow] Transforming classes that implement Continuable
Posted by Torsten Curdt <tc...@apache.org>.
> While playing with javaflow, I found out the hard way that the
> ContinuationClassLoader
> is only transforming java that is in a certain package. I made this
Sorry, about that ....of course that should be made coonfigurable.
> patch to transform
> classes that implement Continuable and leave the others alone. This
> seems like the
> correct way to do it, but I may be missing consequences of doing it
> this way.
Hehe ...have a look into an older revision
Just recently I've removed exactly that :)
I am not a big fan of (empty) marker interfaces.
Plus this has a big impact on the behaviour of
the BCEL processing.
If you do a javaClazz.getAllInterfaces()
you need to have a all the depending classes
and interfaces already in the internal BCEL
repository.
While I did some work with ASM I found it
very nice to work with ...and so I wanted
get rid of a direct BCEL dependency. Which
also includes accessing the repository from
javaflow code other than the transformer.
BCEL tries to find and parse unknown classes
by itself ...which does not really work very
well in our case. ...with some effort this
could of course be fixed in BCEL.
...but for now I think rewriting on the
package name is quite a useable alternative
that probably will fit the real world
requirements.
As long it is configurable ;)
On the other hand it might be good to do
a full call flow analysis so we can rewrite
only those methods that might need it because
we are calling suspend.
But this would required the full dependencies
and is quite heavy weight on the load but more
light weight on the execution. Haven't made my
mind up on that yet.
WDYT?
cheers
--
Torsten