Hi! This opens a general discussion about caching on different levels. I strongly favour to have the Extensions only do the extension check once and then keep a boolean isActive flag if they need it more often. Any caching we do will use some memory and still has lots of expensive code to execute. I've measured this a few times already, and evaluating the current ClassLoader is more expensive than going through a list of 3 ClassDeactivators and do the evaluation.
If an Extension does need this information on any @Observes ProcessAnnotatedType, then it should definitely only evaluated it once and store the info locally. But this is really a good point to discuss our caching strategy at all. Do we like to define a rule of thumbs when we will apply caching internally and when not? Do we like to provide public SPIs to flush the caches, e.g. when dynamic class reload comes into place? LieGrue, strub ----- Original Message ----- > From: "[email protected]" <[email protected]> > To: [email protected] > Cc: > Sent: Sunday, January 22, 2012 5:35 PM > Subject: git commit: DELTASPIKE-24 fixed reworked implementation > > Updated Branches: > refs/heads/master e3f7e3e4e -> a54416242 > > > DELTASPIKE-24 fixed reworked implementation > > * added logging for the more complex scenarios > * improved performance of ClassDeactivation#isActivated > > > Project: http://git-wip-us.apache.org/repos/asf/incubator-deltaspike/repo > Commit: > http://git-wip-us.apache.org/repos/asf/incubator-deltaspike/commit/a5441624 > Tree: > http://git-wip-us.apache.org/repos/asf/incubator-deltaspike/tree/a5441624 > Diff: > http://git-wip-us.apache.org/repos/asf/incubator-deltaspike/diff/a5441624 > > Branch: refs/heads/master > Commit: a544162428258e66187464e43f6d98ad8f926667 > Parents: e3f7e3e > Author: gpetracek <[email protected]> > Authored: Sun Jan 22 17:26:51 2012 +0100 > Committer: gpetracek <[email protected]> > Committed: Sun Jan 22 17:26:51 2012 +0100 > > ---------------------------------------------------------------------- > .../core/api/activation/ClassDeactivation.java | 88 ++++++++++++--- > .../core/api/activation/ClassDeactivator.java | 2 + > 2 files changed, 76 insertions(+), 14 deletions(-) > ---------------------------------------------------------------------- > > > http://git-wip-us.apache.org/repos/asf/incubator-deltaspike/blob/a5441624/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivation.java > ---------------------------------------------------------------------- > diff --git > a/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivation.java > > b/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivation.java > index 6a31ad9..875ce6d 100644 > --- > a/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivation.java > +++ > b/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivation.java > @@ -45,6 +45,13 @@ public final class ClassDeactivation > private static Map<ClassLoader, List<ClassDeactivator>> > classDeactivatorMap > = new ConcurrentHashMap<ClassLoader, > List<ClassDeactivator>>(); > > + /** > + * Cache for the result. It won't contain many classes but it might be > accessed frequently. > + * Valid entries are only true or false. If an entry isn't available or > null, it gets calculated. > + */ > + private static Map<Class<? extends Deactivatable>, Boolean> > activationStatusCache > + = new ConcurrentHashMap<Class<? extends Deactivatable>, > Boolean>(); > + > private ClassDeactivation() > { > // private ct to prevent utility class from instantiation. > @@ -53,48 +60,100 @@ public final class ClassDeactivation > /** > * Evaluates if the given {@link Deactivatable} is active. > * > - * @param deactivatableClazz {@link Deactivatable} under test. > + * @param targetClass {@link Deactivatable} under test. > * @return <code>true</code> if it is active, > <code>false</code> otherwise > */ > - public static synchronized boolean isActivated(Class<? extends > Deactivatable> deactivatableClazz) > + public static boolean isActivated(Class<? extends Deactivatable> > targetClass) > + { > + Boolean activatedClassCacheEntry = > activationStatusCache.get(targetClass); > + > + if (activatedClassCacheEntry == null) > + { > + initDeactivatableCacheFor(targetClass); > + activatedClassCacheEntry = > activationStatusCache.get(targetClass); > + } > + return activatedClassCacheEntry; > + } > + > + private static synchronized void initDeactivatableCacheFor(Class<? > extends Deactivatable> targetClass) > { > + Boolean activatedClassCacheEntry = > activationStatusCache.get(targetClass); > + > + if (activatedClassCacheEntry != null) //double-check > + { > + return; > + } > + > List<ClassDeactivator> classDeactivators = > getClassDeactivators(); > > - Boolean isActivated = Boolean.TRUE; // by default a class is always > activated. > + Boolean isActivated = null; > + Boolean isLocallyActivated; > + Class<? extends ClassDeactivator> deactivationDetected = null; > + > + LOG.info("start evaluation if " + targetClass.getName() + > " is de-/activated"); > > for (ClassDeactivator classDeactivator : classDeactivators) > { > - Boolean isLocallyActivated = > classDeactivator.isActivated(deactivatableClazz); > + isLocallyActivated = classDeactivator.isActivated(targetClass); > + > if (isLocallyActivated != null) > { > isActivated = isLocallyActivated; > } > - } > - > - if (!isActivated) > - { > - LOG.info("Deactivating class " + deactivatableClazz); > + > + /* > + * Check and log the details across class-deactivators > + */ > + if (Boolean.FALSE.equals(isActivated)) > + { > + deactivationDetected = classDeactivator.getClass(); > + LOG.fine("Deactivating class " + targetClass); > + } > + else if (Boolean.TRUE.equals(isActivated) && > deactivationDetected != null) > + { > + LOG.fine("Reactivation of: " + targetClass.getName() > + " by " + classDeactivator.getClass().getName() + > + " - original deactivated by: " + > deactivationDetected.getName() + "."); > + > + LOG.fine("If that isn't the intended behaviour, you > have to use a higher ordinal for " + > + deactivationDetected.getName()); > + } > } > > - return isActivated; > + cacheResult(targetClass, isActivated); > } > > + private static void cacheResult(Class<? extends Deactivatable> > targetClass, Boolean activated) > + { > + if (Boolean.FALSE.equals(activated)) > + { > + activationStatusCache.put(targetClass, false); > + LOG.info("class: " + targetClass.getName() + " is > deactivated."); > + } > + else //in case of true or null (classes are activated by default) > + { > + activationStatusCache.put(targetClass, true); > + LOG.info("class: " + targetClass.getName() + " is > activated."); > + } > + } > > /** > * @return the List of configured @{link ClassDeactivator}s for the > current > context ClassLoader. > */ > private static List<ClassDeactivator> getClassDeactivators() > { > - List<ClassDeactivator> classDeactivators = > classDeactivatorMap.get(ClassUtils.getClassLoader(null)); > + ClassLoader classLoader = ClassUtils.getClassLoader(null); > + List<ClassDeactivator> classDeactivators = > classDeactivatorMap.get(classLoader); > + > if (classDeactivators == null) > { > - classDeactivators = getConfiguredClassDeactivator(); > + return initConfiguredClassDeactivators(classLoader); > } > > return classDeactivators; > } > > - private static List<ClassDeactivator> getConfiguredClassDeactivator() > + //synchronized isn't needed - #initDeactivatableCacheFor is already > synchronized > + private static List<ClassDeactivator> > initConfiguredClassDeactivators(ClassLoader classLoader) > { > List<String> classDeactivatorClassNames = > ConfigResolver.getAllPropertyValues(ClassDeactivator.class.getName()); > > @@ -113,10 +172,11 @@ public final class ClassDeactivation > catch (Exception e) > { > LOG.warning(classDeactivatorClassName + " can't be > instantiated"); > - throw new RuntimeException(e); > + throw new IllegalStateException(e); > } > } > > + classDeactivatorMap.put(classLoader, classDeactivators); > return classDeactivators; > } > } > > http://git-wip-us.apache.org/repos/asf/incubator-deltaspike/blob/a5441624/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivator.java > ---------------------------------------------------------------------- > diff --git > a/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivator.java > > b/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivator.java > index 89e4a6f..d0788e9 100644 > --- > a/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivator.java > +++ > b/deltaspike/core/api/src/main/java/org/apache/deltaspike/core/api/activation/ClassDeactivator.java > @@ -21,6 +21,8 @@ package org.apache.deltaspike.core.api.activation; > import java.io.Serializable; > > /** > + * <p>An implementation has to be stateless.</p> > + * > * <p>A class-deactivator allows to specify deactivated classes which > can't be deactivated via std. CDI mechanisms. > * This might be the case for CDI Extensions because CDI mechanisms are not > available at startup time.</p> > * >
