JBoss development,
A new message was posted in the thread "ClassPool Refactoring":
mailto:flavia.rainone@jboss.com wrote:
All tests are passing now in my system, which allowed me to validate two previous fixes
Kabir and I implemented for the problem of duplicate CtClasses.
Kabir's fix consists of
https://jira.jboss.org/jira/browse/JBAOP-766 :
*public* *class* DelegatingClassPool *extends* BaseClassPool
{
...
*public* CtClass getCached(String classname)
{
>>> //TODO Not 100 sure this is correct, but it seems to do the job where
repeated calls to get() on a pool in a hierarchy returns a different instance of the
class
>>> CtClass clazz = super.getCachedLocally(classname);
>>> *if* (clazz != *null*)
>>> *return* clazz;
*if* (isGeneratedClass(classname))
{
*return* *null*;
}
*return* domain.getCachedOrCreate(this, classname, *false*);
}
}
My fix has never been comitted and consists of adding a check on the classes cache to
BaseClassPool.get:
*public* *class* BaseClassPool *extends* AbstractClassPool
{
...
@Override
*public* *final* CtClass get(String classname) *throws* NotFoundException
{
*boolean* trace = logger.isTraceEnabled();
*if* (trace) logger.trace(*this* + " initiating get of " + classname);
*if* (this.getClassLoader() == *null*)
{
*throw* *new* IllegalStateException("Illegal call. " + " A class
cannot be retrieved from ClassPool " +
*this* + " because the corresponding
ClassLoader is garbage collected");
}
*try*
{
>>> *if* (this.classes.containsKey(classname))
>>> {
>>> *return* (CtClass) classes.get(classname);
>>> }
CtClass clazz = super.get(classname);
*if* (trace)
logger.trace(*this* + " completed get of " + classname + "
" + getClassPoolLogStringForClass(clazz));
*return* clazz;
}
*catch* (NotFoundException e)
{
*if* (trace)
logger.trace(classname + " could not be found from " + this, e);
*throw* e;
}
}
}
It turns out that, once all tests were fixed for issue
https://jira.jboss.org/jira/browse/JBREFLECT-94, those fixes weren't needed anymore,
which means that JBREFLECT-94 was the real issue.
Now I'm left with the dillema of whether should I commit those fixes. I have started
running some tests to try to figure out if any of those bring some improvement, and it
seems that looking up classes cache in BaseClassLoader.get does bring a few improvements
to the performance of aop tests, but I need to run those tests more times in order to do a
reasonable investigation.
While working with Kabir to fix the AOP tests for AS, he pointed out that there could be
an issue related to my fix. It seemed obviously right to me to look in the classes cache
(a cache declared in ScopedClassPool that contains all classes created by the current
class pool) before doing anything else. To me, if we have a hit in the cache, it means
that the current pool is the one that is supposed to load the class, and that there is no
need to look further by calling super.get method. But Kabir raised the following
possibility. What if the class pool scenario (a reflection of the class loader/domain
scenario) changes after the current class pool loaded the class, in a way that this class
pool is no longer the one responsible for loading that class? In that case, looking for
the class in the classes cache would be a serious mistake. Is this scenario possible?
Regarding Kabir's fix, I'm also looking for any opnion regarding whether it
should be committed. It seems, though, that adding both fixes brings some small
performance penalty. I'll continue running tests to be able to assert whether this is
really true, and what is the penalty we're talking about.
I ran the performance
tests, and concluded that there is not performance penalty in including both
"fixes". On the contrary, I have seen some small improvements with those
(AOPUnitTestCase runs 2.5 seconds faster with the fix on BaseClassPool; as starts 200 ms
faster in all mode with both fixes).
I'm comitting this.
--------------------------------------------------------------
To reply to this message visit the message page: