ClassPool for JBoss Reflection

[flavia.rainone]

Kabir,

As was mentioned at the f2f meeting, I'm responsible for completing the Javassist implementation for JBoss Reflection.

Given all the work that Stale has done for https://jira.jboss.org/jira/browse/JBREFLECT-49, the main thing left is the ClassPool :).

As far as I understand, the ClassPools you created for JBAOP-666 are the only ones compatible with AS CL structure and should be needed in order for the Javassist to work correctly inside AS, right?

Can we move those ClassPools to jboss-reflect? Does this ClassPool structure does everything it is supposed to do or do I need to complement some functionality?

Ales, talking about ClassPools, I think JBREFLECT-3 is outdated? Now ScopedClassPool is not enough anymore.

[kabirkhan]

The classpools from asintegration-mc could be put into another jar, although I am not sure if jboss-reflect is the right place for them. Maybe they should be put into another project. Also, due to the workings of AOP, they extend AOPClassPool from the aop project, which in turn extends ScopedClassPool from javassist.

Ideally we should have one jar for the classpool from aop, one for the classpool from asintegration-core, one for the one from asintegration-jmx and one for asintegration-mc.

AOPClassPool and the ClassPoolFactories have some dependencies on the AspectManager, but it should be possible to abstract that out.

[alesj]

Ales, talking about ClassPools, I think JBREFLECT-3 is outdated? Now ScopedClassPool is not enough anymore.

True.

You can either change its description, as the main purpose of this issue was still to provide proper pool(s),
and then the scoped one looked like an obvious choice.

Or you can close it and create a new issue, up to you.
I would go with choice #1. :-)

[flavia.rainone]

The classpools from asintegration-mc could be put into another jar, although I am not sure if jboss-reflect is the right place for them. Maybe they should be put into another project.

This is a question whose answer I don't know. I thought it belonged to JBoss Reflect, as there is a task to create an appropriate class loader (JBREFLECT-3). Isn't those class pools all that is needed to make JBoss Reflect/Javassist to work on the AS environment?

Also, due to the workings of AOP, they extend AOPClassPool from the aop project, which in turn extends ScopedClassPool from javassist.

Ideally we should have one jar for the classpool from aop, one for the classpool from asintegration-core, one for the one from asintegration-jmx and one for asintegration-mc.

AOPClassPool and the ClassPoolFactories have some dependencies on the AspectManager, but it should be possible to abstract that out.

Yes, I can refactorate that, and extract the dependencies to a ClassPool wrapper, if we decide to go this way.

[flavia.rainone]

"alesj" wrote:
I would go with choice #1. :-)

Done:
https://jira.jboss.org/jira/browse/JBREFLECT-3

I still think that I'm going to update this jira description once again or add commentaries, once I have a better idea of what really needs to be done.

[flavia.rainone]

Kabir,

I have taken a very good look on all class pools, and I'm still having a hard time to figure out a few things.

Starting from javassist.ClassPool (http://www.docjar.com/html/api/javassist/ClassPool.java.html):
ClassPool allows you to retrieve a CtClass representing already existing classes through get and through getCtClass:
- getCtClass adds support to arrays and calls get
- get delegates to get0, plus increment some CtClass getCounter
- get0 uses childFirstLookup to determine whether it should try to retrieve the class from the parent domain before or after trying to retrieve itself. When trying to retrieve itself,it checks on whether the class is cached, if not, creates a new one by calling createCtClass, plus caches it.
Note that the caching of get0 is enabled or disabled according to the cache parameters. If cache is false, the ClassPool won't cache the result of createCtClass. Given get0 is an internal method, I've taken a look at all callers to see when is cache false and when is it true. It is false only when it is called by getAndRename, which IMO makes sense, given it is just a shortcut for creating a new class, so you don't want to cache the original one with a new name. So, we can assume that get0 will cache classes whenever needed.

Now, taking a look at ScopedClassPool:
http://www.docjar.com/html/api/javassist/scopedpool/ScopedClassPool.java.html
- it extends ClassPool
- adds a very good functionality: soften. This method allows that classes be moved from the hard reference classes cache to the softcache, that keeps soft references, allowing them to be gc'ed. This functionality is used by JBoss AOP to mark all classes that were not instrumented, thus preventing memory leak of non-instrumented CtClasses (the idea is that if they need to be created again, there won't be inconsistencies as no instrumentation has taken place). We could do the same for JBoss Reflection.
- cacheCtClass: complementing the soften functionality, this CP overwrites the cacheCtClass method, delegating to the superclass only if the class is being created dynamicly (i.e., it is a new class, that was not in the classpath), and adding it to softcache otherwise.
- adds a ScopedClassPoolRepository, that contains several pools.
- overwrites internal getCached method, so that whenever you need a cached class it will also look into the classes of the other class pools
- adds a getCachedLocally, which just looks for classes at the "classes" cache (defined at the super class ClassPool) and at the "softcache' as well.
- overwrites the toClass method, so that it uses the getClassLoader0 (there is a commentary justifying why it is that way), plus it calls lockInCache, which I didn't understand why. (1) ***
- added lockInCache, which is just a shortcut for calling super.cacheCtClass(c.getName(), c, false);
- added a close() method, that removes the cp from the repository
- added a getLocally method. This method basically removes a class from the softcache, creates it and caches it at the classes collection. I imagine that this method removes the class from the softcache for garanteeing consistency, so that there won't be two different versions of the same ctClass: one in the softcache and another one in the classes cache.

I figured that this ScopedClassPool and its repository were created to deal with UnifiedClassLoaders (let me know if I'm wrong). But now, the one million dollar question (2): why is it that getLocally was added to ScopedClassPool? Shouldn't ScopedClassPool had overwitten get0 instead? I see that this method is almost the same as the get0, except for two differences: it removes the class from the softcache (a much needed feature whenever a ct class is created) and it doesn't use childLookupFirst to delegate to the parent class loader. The result is that, IMO, by creating getLocally instead of overwriting get0, there are two ways of retrieving a ctclass, and the "official" way doesn't even handle the softcache stuff, creating an inconsistency. A way of solving but keeping getLocally would be overriding createCtClass like this:

softcache.remove(classname); super.createCtClass(...);

For the record, JBoss AOP never uses the official methods get and getCtClass... it uses the getLocally instead, thus avoiding the inconsistency mentioned. (3)

Now, we go to AOPClassPool:
- it extends ScopedClassPool
- getLocally: this method is overriden, but the strange thing is that the body of this method is a copy of the body of the super class, excepts for a few logging calls and except for teh fact that it replaces an invocation to ClassPool.cacheCtClass with a call to lockInCache, but they are equivalent. (4)
- getCached: the AOPClassPool overrides the ScopedClassPool.getCached method, thus overriding the single point of ScopedClassPool that uses the ScopedClassPoolRepository to delegate ctclass retrieval to other CPs of the same repository.
- there are several other details that I don't think are useful for my questions.

So, why does AOPClassPool extends ScopedClassPool if it doesn't uses the ScopedClassPoolRepository for retrieval of classes? (5)

Now, going to BaseClassPool (subclass of AOPClassPool), I see that it overrites createCtClass. It adds a call to lockInCache before calling super.createCtClass method. This generates another inconsistency for the get methods of ClassPool. Take a look at a part of get0 implementation:

491 clazz = createCtClass(classname, useCache);
 492 if (clazz != null) {
 493 // clazz.getName() != classname if classname is "[L<name>;".
 494 if (useCache)
 495 cacheCtClass(clazz.getName(), clazz, false);
 496
 497 return clazz;
 498 }

As you can see, get0 adds the class to the cache by calling cacheCtClass after calling createCtClass. As I mentioned previously, cacheCtClass is overriden by ScopedClassPool, and adds the class to classes cache only if it is not generated dynamic. By adding the lockInCache call to the createCtClass method at BaseClassPool, you are generating a (6):
- memory leak if the class was generated dynamically. The class will be added to both softcache (by Scoped.cacheCtClass) and to classes (by BaseClassPool.cacheCtClass), thus avoiding it from being gc'ed.
- or a redundancy if the class was not generated dynamically, as the Scoped.cacheCtClass will already add the class to the classes cache.


Now, taking a look at NonDelegatingClassPool (subclass of BaseClassPool), I see that it overrides createCtClass, recreating the behaviour we had originally in the ClassPool. So, why is it a subclass of ScopedClassPool in the first place? (7)

(8) Plus, taking a look at DelegatingClassPool (subclass of BaseClassPool), I see it overrides the get0 method, but this method is never invoked by JBoss AOP if it uses getLocally. So, are the ClassPool.get/ClassPool.getCtClass being used from inside AS? If they are not, I don't see how are the new functionalities of the new ClassPools being used inside AS. If they are, then I see a point in overriding get0, but there is an inconsistency given that JBoss AOP uses the getLocally method, defined at AOPClass superclass.

So, Kabir, I'm obviously missing several links that will help me understand the class pools. So, please, comment on the numbered remarks and questions, and let me know what is it I'm missing here :)

*** I mean, when you do toClass, the class is already in the cache by now. Plus, what if somebody is doing a toClass with a class that is in the softcache? The lockInCache call will force the class being added as a hard reference to the "classes" cache, thus ruining the soften feature.

[kabirkhan]

Historically, the ScopedClassPool is there to extract out the known behaviour at the time (early 2006 IIRC) of how the UnifiedClassLoaders (AS 4) worked. It used to all live in AOPClassPool, but since JBoss Retro also needed to do loadtime weaving stuff the common code was moved to ScopedClassPool.

1) The comment means that we should use the classloader associated with the pool rather than the default thread context classloader that would get passed in when somebody calls ClassPool.toClass().

2) get0() is an internal implementation detail in the pools, so getLocally() is there to expose the functionality. Also, it ensures that we just search the pool itself, since at weaving time we know the exact classloader (passed in by the agent).

3) If I do a search for who is calling ClassPool.get() I get loads of callers, so we are using that in AOP. Also, when you load up a CtClass, get a CtField or a CtMethod parameter type I think javassist will call get() internally to obtain that. So as mentioned in 2 when weaving a class we use the pool for the classloader used to load the CtClass since we know it must come from there, but later in the weaving process we end up in get0() when fetching in the superclass, fields, methods, parameters etc.

4) Probably just to get the extra logging. javassist ScopedClassPool could be fixed I guess, but it does not use log4j so there is not really any way to add logging there.

6) I don't think it is a memory leak, since the CtClasses need to be cached, and in AS we create/drop the pools when the classloaders are deployed/undeployed. It might be redundant as you say though.

5&7) I agree with you here, but a bunch of stuff in the API is using the ScopedClassPool(Factory/Repository) stuff. I was in two minds at the time whether to get rid of it, but was worried about breaking backwards compatibility. But yes, for the new delegating classpools a lot of the stuff (the searching across classpools) in ScopedClassPool is redundant. However, the repository is still used when registering and unregistering classloaders. Also, in AS 4 we do not use the new delegating classpools, but the old ones returned by org.jboss.aop.asintegration.jboss4.JBossClassPoolFactory.

8) ClassPool.get() is being used, see 2& 3.

[kabirkhan]

*** I mean, when you do toClass, the class is already in the cache by now. Plus, what if somebody is doing a toClass with a class that is in the softcache? The lockInCache call will force the class being added as a hard reference to the "classes" cache, thus ruining the soften feature.

I'm not really sure what you mean here, but toClass() is called when creating a new class so does that not need to be in the hard cache? If only in the softcache, when it is removed and somebody needs to obtain the CtClass again we would need to load it from its resource. Although in AS that would probably be fine since dynamically created classes have their bytes written to a temporary location (tempURL in AS 4, vfsmemory url in AS 5)

[flavia.rainone]

*** I mean, when you do toClass, the class is already in the cache by now. Plus, what if somebody is doing a toClass with a class that is in the softcache? The lockInCache call will force the class being added as a hard reference to the "classes" cache, thus ruining the soften feature.

I'm not really sure what you mean here, but toClass() is called when creating a new class so does that not need to be in the hard cache? If only in the softcache, when it is removed and somebody needs to obtain the CtClass again we would need to load it from its resource. Although in AS that would probably be fine since dynamically created classes have their bytes written to a temporary location (tempURL in AS 4, vfsmemory url in AS 5)

The lockInCache method forces the class to be added to the classes cache, which is hard reference based. But, if you add a class that was in the softcache to the classes cache, the softcache is useless for this class... it is in the softcache but it won't be gc'ed.

[flavia.rainone]

6) I don't think it is a memory leak, since the CtClasses need to be cached, and in AS we create/drop the pools when the classloaders are deployed/undeployed. It might be redundant as you say though.

What I meant is that it is pointless to add a ctclass to the softcache if you're also adding it to the hard reference cache at the same time... it won't be gc'ed.

[flavia.rainone]

1) The comment means that we should use the classloader associated with the pool rather than the default thread context classloader that would get passed in when somebody calls ClassPool.toClass().

Actually, my question was about the call to the lockInCache call... why would you put something in the cache if it is already there?

[flavia.rainone]

The classpools from asintegration-mc could be put into another jar, although I am not sure if jboss-reflect is the right place for them. Maybe they should be put into another project.

This is a question whose answer I don't know. I thought it belonged to JBoss Reflect, as there is a task to create an appropriate class loader (JBREFLECT-3). Isn't those class pools all that is needed to make JBoss Reflect/Javassist to work on the AS environment?

Ales, the main question here is where do these class pools belong to.

I'm gonna adapt the Class Pools Kabir wrote, removing AOP related stuff, and a few inconsistencies that I found throughout the CP hierarchy. The resulting CP is gonna fit JBoss AS CL structure, and that's what is necessary for using JBoss Reflect/Javassist with AS.

So, how tied is JBoss Reflect in to AS? If it is not tied in, these class pools belong elsewhere (where?). Otherwise, they belong to JBoss Reflect itself.

[kabirkhan]

JBoss Reflect is not really tied to AS, it is a tool for describing classes in any environment.

So just as the introspection implementation of JBoss Reflect is independent of the jboss-cl implementations (it just uses the right classloader in the environment it is running in) in a similar way the javassist implementation of JBoss Reflect should be independent of the actual classloader implementation.

I think it makes sense to move the classpool stuff out to a separate project which can be consumed by other projects.

[alesj]

Ales, the main question here is where do these class pools belong to.

So just as the introspection implementation of JBoss Reflect is independent of the jboss-cl implementations (it just uses the right classloader in the environment it is running in) in a similar way the javassist implementation of JBoss Reflect should be independent of the actual classloader implementation.

I think it makes sense to move the classpool stuff out to a separate project which can be consumed by other projects.

Yes, I agree with Kabir.

The Reflect should be CL impl agnostic.
You can simply create new CLPool project in jbossas/projects.
Which should also be properly structured - base spi, jboss-cl impl, ...
Where Reflect should then just consume minimal spi, everything else is impl detail depending on the env we're running in.

[kabirkhan]

javassist implementation of JBoss Reflect should be independent of the actual classloader implementation.

Should be classpool implementation