User development, A new message was posted in the thread "ClassPool Refactoring":
http://community.jboss.org/message/527258#527258 Author : Flavia Rainone Profile : http://community.jboss.org/people/flavia.rain...@jboss.com Message: -------------------------------------------------------------- > mailto:flavia.rain...@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: http://community.jboss.org/message/527258#527258
_______________________________________________ jboss-user mailing list jboss-user@lists.jboss.org https://lists.jboss.org/mailman/listinfo/jboss-user
