On 28 May 2014 18:06, <rmannibu...@apache.org> wrote: > Author: rmannibucau > Date: Wed May 28 17:06:12 2014 > New Revision: 1598071 > > URL: http://svn.apache.org/r1598071 > Log: > using reentrant locks instead of old synchronized
-1 This commit mixes two completely different changes: - re-entrant locks - addition of LogHelper I think the LogHelper class is a bad idea. It is also not thread-safe. If the cache is enabled, then it is not possible to change the logging level during a run. > Added: > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/logger/ > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/logger/LogHelper.java > Modified: > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/LRUMapJCS.java > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/control/CompositeCache.java > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractDoubleLinkedListMemoryCache.java > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractMemoryCache.java > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/LRUMap.java > > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/SingleLinkedList.java > > Modified: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/LRUMapJCS.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/LRUMapJCS.java?rev=1598071&r1=1598070&r2=1598071&view=diff > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/LRUMapJCS.java > (original) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/disk/LRUMapJCS.java > Wed May 28 17:06:12 2014 > @@ -19,6 +19,7 @@ package org.apache.commons.jcs.auxiliary > * under the License. > */ > > +import org.apache.commons.jcs.utils.logger.LogHelper; > import org.apache.commons.jcs.utils.struct.LRUMap; > import org.apache.commons.logging.Log; > import org.apache.commons.logging.LogFactory; > @@ -35,6 +36,7 @@ public class LRUMapJCS<K, V> > > /** The logger */ > private static final Log log = LogFactory.getLog( LRUMapJCS.class ); > + private static final LogHelper LOG_HELPER = new LogHelper(log); > > /** > * This creates an unbounded version. > @@ -69,7 +71,7 @@ public class LRUMapJCS<K, V> > @Override > protected void processRemovedLRU(K key, V value) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Removing key [" + key + "] from key store, value [" > + value + "]" ); > log.debug( "Key store size [" + this.size() + "]" ); > > Modified: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/control/CompositeCache.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/control/CompositeCache.java?rev=1598071&r1=1598070&r2=1598071&view=diff > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/control/CompositeCache.java > (original) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/control/CompositeCache.java > Wed May 28 17:06:12 2014 > @@ -46,6 +46,7 @@ import org.apache.commons.jcs.engine.sta > import org.apache.commons.jcs.engine.stats.behavior.ICacheStats; > import org.apache.commons.jcs.engine.stats.behavior.IStatElement; > import org.apache.commons.jcs.engine.stats.behavior.IStats; > +import org.apache.commons.jcs.utils.logger.LogHelper; > import org.apache.commons.logging.Log; > import org.apache.commons.logging.LogFactory; > > @@ -70,6 +71,7 @@ public class CompositeCache<K, V> > { > /** log instance */ > private static final Log log = LogFactory.getLog( CompositeCache.class ); > + private static final LogHelper LOG_HELPER = new LogHelper(log); > > /** > * EventQueue for handling element events. Lazy initialized. One for > each region. To be more efficient, the manager > @@ -228,7 +230,7 @@ public class CompositeCache<K, V> > throw new IllegalArgumentException( "key cannot be a GroupId " + > " for a put operation" ); > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Updating memory cache " + cacheElement.getKey() ); > } > @@ -271,7 +273,7 @@ public class CompositeCache<K, V> > // You could run a database cache as either a remote or a local disk. > // The types would describe the purpose. > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > if ( auxCaches.length > 0 ) > { > @@ -287,7 +289,7 @@ public class CompositeCache<K, V> > { > ICache<K, V> aux = auxCaches[i]; > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Auxilliary cache type: " + aux.getCacheType() ); > } > @@ -300,7 +302,7 @@ public class CompositeCache<K, V> > // SEND TO REMOTE STORE > if ( aux.getCacheType() == CacheType.REMOTE_CACHE ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "ce.getElementAttributes().getIsRemote() = " > + cacheElement.getElementAttributes().getIsRemote() > ); > @@ -313,7 +315,7 @@ public class CompositeCache<K, V> > // need to make sure the group cache understands that > // the key is a group attribute on update > aux.update( cacheElement ); > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Updated remote store for " + > cacheElement.getKey() + cacheElement ); > } > @@ -329,7 +331,7 @@ public class CompositeCache<K, V> > { > // lateral can't do the checking since it is dependent on the > // cache region restrictions > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "lateralcache in aux list: cattr " + > cacheAttr.isUseLateral() ); > } > @@ -339,7 +341,7 @@ public class CompositeCache<K, V> > // Currently always multicast even if the value is > // unchanged, to cause the cache item to move to the > front. > aux.update( cacheElement ); > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "updated lateral cache for " + > cacheElement.getKey() ); > } > @@ -348,7 +350,7 @@ public class CompositeCache<K, V> > // update disk if the usage pattern permits > else if ( aux.getCacheType() == CacheType.DISK_CACHE ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "diskcache in aux list: cattr " + > cacheAttr.isUseDisk() ); > } > @@ -357,7 +359,7 @@ public class CompositeCache<K, V> > && cacheElement.getElementAttributes().getIsSpool() ) > { > aux.update( cacheElement ); > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "updated disk cache for " + > cacheElement.getKey() ); > } > @@ -414,14 +416,14 @@ public class CompositeCache<K, V> > { > // swallow > } > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "spoolToDisk done for: " + ce.getKey() + > " on disk cache[" + i + "]" ); > } > } > else > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "DiskCache avaialbe, but JCS is not > configured to use the DiskCache as a swap." ); > } > @@ -483,7 +485,7 @@ public class CompositeCache<K, V> > > boolean found = false; > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "get: key = " + key + ", localOnly = " + localOnly ); > } > @@ -500,7 +502,7 @@ public class CompositeCache<K, V> > // Found in memory cache > if ( isExpired( element ) ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Memory > cache hit, but element expired" ); > } > @@ -513,7 +515,7 @@ public class CompositeCache<K, V> > } > else > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Memory > cache hit" ); > } > @@ -539,7 +541,7 @@ public class CompositeCache<K, V> > > if ( !localOnly || cacheType == > CacheType.DISK_CACHE ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Attempting to get from aux > [" + aux.getCacheName() + "] which is of type: " > + cacheType ); > @@ -555,7 +557,7 @@ public class CompositeCache<K, V> > } > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Got CacheElement: " + element ); > } > @@ -565,7 +567,7 @@ public class CompositeCache<K, V> > { > if ( isExpired( element ) ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() > + " - Aux cache[" + i + "] hit, but element expired." ); > } > @@ -582,7 +584,7 @@ public class CompositeCache<K, V> > } > else > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() > + " - Aux cache[" + i + "] hit" ); > } > @@ -610,7 +612,7 @@ public class CompositeCache<K, V> > { > missCountNotFound++; > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Miss" ); > } > @@ -666,7 +668,7 @@ public class CompositeCache<K, V> > { > Map<K, ICacheElement<K, V>> elements = new HashMap<K, > ICacheElement<K, V>>(); > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "get: key = " + keys + ", localOnly = " + localOnly ); > } > @@ -693,7 +695,7 @@ public class CompositeCache<K, V> > { > missCountNotFound += keys.size() - elements.size(); > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - " + ( keys.size() > - elements.size() ) + " Misses" ); > } > @@ -725,7 +727,7 @@ public class CompositeCache<K, V> > // Found in memory cache > if ( isExpired( element ) ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Memory > cache hit, but element expired" ); > } > @@ -737,7 +739,7 @@ public class CompositeCache<K, V> > } > else > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Memory > cache hit" ); > } > @@ -777,7 +779,7 @@ public class CompositeCache<K, V> > > if ( !localOnly || cacheType == CacheType.DISK_CACHE ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Attempting to get from aux [" + > aux.getCacheName() + "] which is of type: " > + cacheType ); > @@ -793,7 +795,7 @@ public class CompositeCache<K, V> > } > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Got CacheElements: " + elementsFromAuxiliary > ); > } > @@ -859,7 +861,7 @@ public class CompositeCache<K, V> > { > Map<K, ICacheElement<K, V>> elements = new HashMap<K, > ICacheElement<K, V>>(); > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "get: pattern [" + pattern + "], localOnly = " + > localOnly ); > } > @@ -932,7 +934,7 @@ public class CompositeCache<K, V> > > if ( !localOnly || cacheType == CacheType.DISK_CACHE ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Attempting to get from aux [" + > aux.getCacheName() + "] which is of type: " > + cacheType ); > @@ -947,7 +949,7 @@ public class CompositeCache<K, V> > log.error( "Error getting from aux", e ); > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Got CacheElements: " + > elementsFromAuxiliary ); > } > @@ -983,7 +985,7 @@ public class CompositeCache<K, V> > { > if ( isExpired( element ) ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Aux > cache[" + i + "] hit, but element expired." ); > } > @@ -999,7 +1001,7 @@ public class CompositeCache<K, V> > } > else > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheAttr.getCacheName() + " - Aux > cache[" + i + "] hit" ); > } > @@ -1028,7 +1030,7 @@ public class CompositeCache<K, V> > } > else > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Skipping memory update since no items are > allowed in memory" ); > } > @@ -1175,7 +1177,7 @@ public class CompositeCache<K, V> > } > try > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Removing " + key + " from cacheType" + > cacheType ); > } > @@ -1234,7 +1236,7 @@ public class CompositeCache<K, V> > { > memCache.removeAll(); > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Removed All keys from the memory cache." ); > } > @@ -1253,7 +1255,7 @@ public class CompositeCache<K, V> > { > try > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Removing All keys from cacheType" + > aux.getCacheType() ); > } > @@ -1416,7 +1418,7 @@ public class CompositeCache<K, V> > } > } > } > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Called save for [" + cacheAttr.getCacheName() + "]" > ); > } > @@ -1621,7 +1623,7 @@ public class CompositeCache<K, V> > > if ( maxLifeSeconds != -1 && ( timestamp - createTime ) > ( > maxLifeSeconds * timeFactorForMilliseconds) ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Exceeded maxLife: " + element.getKey() ); > } > @@ -1640,7 +1642,7 @@ public class CompositeCache<K, V> > > if ( ( idleTime != -1 ) && ( timestamp - lastAccessTime ) > > idleTime * timeFactorForMilliseconds ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.info( "Exceeded maxIdle: " + element.getKey() ); > } > @@ -1675,7 +1677,7 @@ public class CompositeCache<K, V> > ArrayList<IElementEventHandler> eventHandlers = > element.getElementAttributes().getElementEventHandlers(); > if ( eventHandlers != null ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Element Handlers are registered. Create event > type " + eventType ); > } > > Modified: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractDoubleLinkedListMemoryCache.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractDoubleLinkedListMemoryCache.java?rev=1598071&r1=1598070&r2=1598071&view=diff > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractDoubleLinkedListMemoryCache.java > (original) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractDoubleLinkedListMemoryCache.java > Wed May 28 17:06:12 2014 > @@ -28,6 +28,7 @@ import org.apache.commons.jcs.engine.sta > import org.apache.commons.jcs.engine.stats.Stats; > import org.apache.commons.jcs.engine.stats.behavior.IStatElement; > import org.apache.commons.jcs.engine.stats.behavior.IStats; > +import org.apache.commons.jcs.utils.logger.LogHelper; > import org.apache.commons.jcs.utils.struct.DoubleLinkedList; > import org.apache.commons.logging.Log; > import org.apache.commons.logging.LogFactory; > @@ -41,6 +42,8 @@ import java.util.Map; > import java.util.Map.Entry; > import java.util.Set; > import java.util.concurrent.ConcurrentHashMap; > +import java.util.concurrent.locks.Lock; > +import java.util.concurrent.locks.ReentrantLock; > > /** > * This class contains methods that are common to memory caches using the > double linked list, such > @@ -58,18 +61,19 @@ public abstract class AbstractDoubleLink > > /** The logger. */ > private static final Log log = LogFactory.getLog( > AbstractDoubleLinkedListMemoryCache.class ); > + private static final LogHelper LOG_HELPER = new LogHelper(log); > > /** thread-safe double linked list for lru */ > protected DoubleLinkedList<MemoryElementDescriptor<K, V>> list; // TODO > privatise > > /** number of hits */ > - private int hitCnt = 0; > + private volatile int hitCnt = 0; > > /** number of misses */ > - private int missCnt = 0; > + private volatile int missCnt = 0; > > /** number of puts */ > - private int putCnt = 0; > + private volatile int putCnt = 0; > > /** > * For post reflection creation initialization. > @@ -77,11 +81,19 @@ public abstract class AbstractDoubleLink > * @param hub > */ > @Override > - public synchronized void initialize( CompositeCache<K, V> hub ) > + public void initialize( CompositeCache<K, V> hub ) > { > - super.initialize( hub ); > - list = new DoubleLinkedList<MemoryElementDescriptor<K, V>>(); > - log.info( "initialized MemoryCache for " + cacheName ); > + lock.lock(); > + try > + { > + super.initialize(hub); > + list = new DoubleLinkedList<MemoryElementDescriptor<K, V>>(); > + log.info("initialized MemoryCache for " + cacheName); > + } > + finally > + { > + lock.unlock(); > + } > } > > /** > @@ -110,17 +122,24 @@ public abstract class AbstractDoubleLink > public final void update( ICacheElement<K, V> ce ) > throws IOException > { > - putCnt++; > + lock.lock(); > + try > + { > + putCnt++; > > - MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce ); > + MemoryElementDescriptor<K, V> newNode = adjustListForUpdate(ce); > > - // this should be synchronized if we were not using a > ConcurrentHashMap > - MemoryElementDescriptor<K, V> oldNode = map.put( > newNode.ce.getKey(), newNode ); > + // this should be synchronized if we were not using a > ConcurrentHashMap > + MemoryElementDescriptor<K, V> oldNode = > map.put(newNode.ce.getKey(), newNode); > > - // If the node was the same as an existing node, remove it. > - if ( oldNode != null && newNode.ce.getKey().equals( > oldNode.ce.getKey() ) ) > + // If the node was the same as an existing node, remove it. > + if (oldNode != null && > newNode.ce.getKey().equals(oldNode.ce.getKey())) { > + list.remove(oldNode); > + } > + } > + finally > { > - list.remove( oldNode ); > + lock.unlock(); > } > > // If we are over the max spool some > @@ -153,7 +172,7 @@ public abstract class AbstractDoubleLink > return; > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "In memory limit reached, spooling" ); > } > @@ -161,7 +180,7 @@ public abstract class AbstractDoubleLink > // Write the last 'chunkSize' items to disk. > int chunkSizeCorrected = Math.min( size, chunkSize ); > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "About to spool to disk cache, map size: " + size + > ", max objects: " > + this.cacheAttributes.getMaxObjects() + ", items to spool: > " + chunkSizeCorrected ); > @@ -175,7 +194,7 @@ public abstract class AbstractDoubleLink > spoolLastElement(); > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "update: After spool map size: " + map.size() + " > linked list size = " + dumpCacheSize() ); > } > @@ -194,7 +213,7 @@ public abstract class AbstractDoubleLink > { > ICacheElement<K, V> ce = null; > > - final boolean debugEnabled = log.isDebugEnabled(); > + final boolean debugEnabled = LOG_HELPER.isDebugEnabled();; > > if (debugEnabled) > { > @@ -205,19 +224,37 @@ public abstract class AbstractDoubleLink > > if ( me != null ) > { > - ce = me.ce; > - hitCnt++; > + lock.lock(); > + try > + { > + ce = me.ce; > + hitCnt++; > + > + // ABSTRACT > + adjustListForGet( me ); > + } > + finally > + { > + lock.unlock(); > + } > + > if (debugEnabled) > { > log.debug( cacheName + ": LRUMemoryCache hit for " + > ce.getKey() ); > } > - > - // ABSTRACT > - adjustListForGet( me ); > } > else > { > - missCnt++; > + lock.lock(); > + try > + { > + missCnt++; > + } > + finally > + { > + lock.unlock(); > + } > + > if (debugEnabled) > { > log.debug( cacheName + ": LRUMemoryCache miss for " + key ); > @@ -274,22 +311,28 @@ public abstract class AbstractDoubleLink > final MemoryElementDescriptor<K, V> last = list.getLast(); > if ( last != null ) > { > - toSpool = last.ce; > - if ( toSpool != null ) > + lock.lock(); > + try > { > - cache.spoolToDisk( last.ce ); > - if ( map.remove( last.ce.getKey() ) == null ) > + toSpool = last.ce; > + if (toSpool != null) { > + cache.spoolToDisk(last.ce); > + if (map.remove(last.ce.getKey()) == null) { > + log.warn("update: remove failed for key: " > + + last.ce.getKey()); > + verifyCache(); > + } > + } > + else > { > - log.warn( "update: remove failed for key: " > - + last.ce.getKey() ); > - verifyCache(); > + throw new Error("update: last.ce is null!"); > } > + list.remove(last); > } > - else > + finally > { > - throw new Error( "update: last.ce is null!" ); > + lock.unlock(); > } > - list.remove(last); > } > else > { > @@ -320,7 +363,7 @@ public abstract class AbstractDoubleLink > public boolean remove( K key ) > throws IOException > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "removing item for key: " + key ); > } > @@ -336,11 +379,19 @@ public abstract class AbstractDoubleLink > Map.Entry<K, MemoryElementDescriptor<K, V>> entry = > itr.next(); > K k = entry.getKey(); > > - if ( k instanceof String && ( (String) k ).startsWith( > key.toString() ) ) > + if (k instanceof String && ((String) > k).startsWith(key.toString())) > { > - list.remove( entry.getValue() ); > - itr.remove(); > - removed = true; > + lock.lock(); > + try > + { > + list.remove(entry.getValue()); > + itr.remove(); > + removed = true; > + } > + finally > + { > + lock.unlock(); > + } > } > } > } > @@ -355,21 +406,36 @@ public abstract class AbstractDoubleLink > if ( k instanceof GroupAttrName && > > ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId)) > { > - list.remove( entry.getValue() ); > - itr.remove(); > - removed = true; > + lock.lock(); > + try > + { > + list.remove(entry.getValue()); > + itr.remove(); > + removed = true; > + } > + finally > + { > + lock.unlock(); > + } > } > } > } > else > { > // remove single item. > - MemoryElementDescriptor<K, V> me = map.remove( key ); > - > - if ( me != null ) > + lock.lock(); > + try > { > - list.remove( me ); > - removed = true; > + MemoryElementDescriptor<K, V> me = map.remove(key); > + if (me != null) > + { > + list.remove(me); > + removed = true; > + } > + } > + finally > + { > + lock.unlock(); > } > } > > @@ -386,8 +452,16 @@ public abstract class AbstractDoubleLink > public void removeAll() > throws IOException > { > - list.removeAll(); > - map.clear(); > + lock.lock(); > + try > + { > + list.removeAll(); > + map.clear(); > + } > + finally > + { > + lock.unlock(); > + } > } > > // --------------------------- internal methods (linked list > implementation) > @@ -399,10 +473,18 @@ public abstract class AbstractDoubleLink > */ > protected MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce > ) > { > - MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, > V>( ce ); > - list.addFirst( me ); > - verifyCache( ce.getKey() ); > - return me; > + lock.lock(); > + try > + { > + MemoryElementDescriptor<K, V> me = new > MemoryElementDescriptor<K, V>(ce); > + list.addFirst(me); > + verifyCache(ce.getKey()); > + return me; > + } > + finally > + { > + lock.unlock(); > + } > } > > /** > @@ -413,10 +495,18 @@ public abstract class AbstractDoubleLink > */ > protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce ) > { > - MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, > V>( ce ); > - list.addLast( me ); > - verifyCache( ce.getKey() ); > - return me; > + lock.lock(); > + try > + { > + MemoryElementDescriptor<K, V> me = new > MemoryElementDescriptor<K, V>(ce); > + list.addLast(me); > + verifyCache(ce.getKey()); > + return me; > + } > + finally > + { > + lock.unlock(); > + } > } > > // ---------------------------------------------------------- debug > methods > @@ -451,7 +541,7 @@ public abstract class AbstractDoubleLink > @SuppressWarnings("unchecked") // No generics for public fields > protected void verifyCache() > { > - if ( !log.isDebugEnabled() ) > + if ( !LOG_HELPER.isDebugEnabled() ) > { > return; > } > @@ -534,7 +624,7 @@ public abstract class AbstractDoubleLink > @SuppressWarnings("unchecked") // No generics for public fields > private void verifyCache( K key ) > { > - if ( !log.isDebugEnabled() ) > + if ( !LOG_HELPER.isDebugEnabled() ) > { > return; > } > @@ -684,12 +774,7 @@ public abstract class AbstractDoubleLink > @Override > public Set<K> getKeySet() > { > - // need a better locking strategy here. > - synchronized ( this ) > - { > - // may need to lock to map here? > - return new LinkedHashSet<K>(map.keySet()); > - } > + return new LinkedHashSet<K>(map.keySet()); > } > > /** > @@ -699,18 +784,26 @@ public abstract class AbstractDoubleLink > * @see > org.apache.commons.jcs.engine.memory.behavior.IMemoryCache#getStatistics() > */ > @Override > - public synchronized IStats getStatistics() > + public IStats getStatistics() > { > IStats stats = new Stats(); > stats.setTypeName( /*add algorithm name*/"Memory Cache" ); > > ArrayList<IStatElement<?>> elems = new ArrayList<IStatElement<?>>(); > > - elems.add(new StatElement<Integer>( "List Size", > Integer.valueOf(list.size()) ) ); > - elems.add(new StatElement<Integer>( "Map Size", > Integer.valueOf(map.size()) ) ); > - elems.add(new StatElement<Integer>( "Put Count", > Integer.valueOf(putCnt) ) ); > - elems.add(new StatElement<Integer>( "Hit Count", > Integer.valueOf(hitCnt) ) ); > - elems.add(new StatElement<Integer>( "Miss Count", > Integer.valueOf(missCnt) ) ); > + lock.lock(); // not sure that's really relevant here but not that > important > + try > + { > + elems.add(new StatElement<Integer>("List Size", > Integer.valueOf(list.size()))); > + elems.add(new StatElement<Integer>("Map Size", > Integer.valueOf(map.size()))); > + elems.add(new StatElement<Integer>("Put Count", > Integer.valueOf(putCnt))); > + elems.add(new StatElement<Integer>("Hit Count", > Integer.valueOf(hitCnt))); > + elems.add(new StatElement<Integer>("Miss Count", > Integer.valueOf(missCnt))); > + } > + finally > + { > + lock.unlock(); > + } > > stats.setStatElements( elems ); > > > Modified: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractMemoryCache.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractMemoryCache.java?rev=1598071&r1=1598070&r2=1598071&view=diff > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractMemoryCache.java > (original) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/AbstractMemoryCache.java > Wed May 28 17:06:12 2014 > @@ -28,6 +28,7 @@ import org.apache.commons.jcs.engine.mem > import org.apache.commons.jcs.engine.memory.util.MemoryElementDescriptor; > import org.apache.commons.jcs.engine.stats.Stats; > import org.apache.commons.jcs.engine.stats.behavior.IStats; > +import org.apache.commons.jcs.utils.logger.LogHelper; > import org.apache.commons.logging.Log; > import org.apache.commons.logging.LogFactory; > > @@ -35,6 +36,8 @@ import java.io.IOException; > import java.util.HashMap; > import java.util.Map; > import java.util.Set; > +import java.util.concurrent.locks.Lock; > +import java.util.concurrent.locks.ReentrantLock; > > /** > * This base includes some common code for memory caches. > @@ -47,6 +50,7 @@ public abstract class AbstractMemoryCach > { > /** Log instance */ > private static final Log log = LogFactory.getLog( > AbstractMemoryCache.class ); > + private static final LogHelper LOG_HELPER = new LogHelper(log); > > /** The region name. This defines a namespace of sorts. */ > protected String cacheName; // TODO privatise (mainly seems to be used > externally for debugging) > @@ -69,21 +73,31 @@ public abstract class AbstractMemoryCach > /** How many to spool at a time. */ > protected int chunkSize; > > + protected final Lock lock = new ReentrantLock(); > + > /** > * For post reflection creation initialization > * <p> > * @param hub > */ > @Override > - public synchronized void initialize( CompositeCache<K, V> hub ) > + public void initialize( CompositeCache<K, V> hub ) > { > - this.cacheName = hub.getCacheName(); > - this.cacheAttributes = hub.getCacheAttributes(); > - this.cache = hub; > - map = createMap(); > + lock.lock(); > + try > + { > + this.cacheName = hub.getCacheName(); > + this.cacheAttributes = hub.getCacheAttributes(); > + this.cache = hub; > + map = createMap(); > > - chunkSize = cacheAttributes.getSpoolChunkSize(); > - status = CacheStatus.ALIVE; > + chunkSize = cacheAttributes.getSpoolChunkSize(); > + status = CacheStatus.ALIVE; > + } > + finally > + { > + lock.unlock(); > + } > } > > /** > @@ -163,14 +177,14 @@ public abstract class AbstractMemoryCach > MemoryElementDescriptor<K, V> me = map.get( key ); > if ( me != null ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheName + ": MemoryCache quiet hit for " + key > ); > } > > ce = me.ce; > } > - else if ( log.isDebugEnabled() ) > + else if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( cacheName + ": MemoryCache quiet miss for " + key ); > } > @@ -261,7 +275,7 @@ public abstract class AbstractMemoryCach > { > String attributeCacheName = this.cacheAttributes.getCacheName(); > if(attributeCacheName != null) > - return attributeCacheName; > + return attributeCacheName; > return cacheName; > } > > > Added: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/logger/LogHelper.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/logger/LogHelper.java?rev=1598071&view=auto > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/logger/LogHelper.java > (added) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/logger/LogHelper.java > Wed May 28 17:06:12 2014 > @@ -0,0 +1,42 @@ > +/* > + * Licensed to the Apache Software Foundation (ASF) under one > + * or more contributor license agreements. See the NOTICE file > + * distributed with this work for additional information > + * regarding copyright ownership. The ASF licenses this file > + * to you under the Apache License, Version 2.0 (the > + * "License"); you may not use this file except in compliance > + * with the License. You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, > + * software distributed under the License is distributed on an > + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY > + * KIND, either express or implied. See the License for the > + * specific language governing permissions and limitations > + * under the License. > + */ > +package org.apache.commons.jcs.utils.logger; > + > +import org.apache.commons.logging.Log; > + > +// when cached isDebugEnabled will save a lot of time > +public class LogHelper > +{ > + protected static final boolean cacheDebug = > Boolean.getBoolean("jcs.logger.cacheDebug"); // TODO: move it over cache > config if really used (=default not nice enough) > + > + private boolean debug; > + private Log log; > + > + public LogHelper(final Log log) > + { > + this.debug = log.isDebugEnabled(); > + this.log = log; > + } > + > + // faster than several log calls by default > + public boolean isDebugEnabled() > + { > + return cacheDebug ? debug : log.isDebugEnabled(); > + } > +} > > Modified: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/LRUMap.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/LRUMap.java?rev=1598071&r1=1598070&r2=1598071&view=diff > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/LRUMap.java > (original) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/LRUMap.java > Wed May 28 17:06:12 2014 > @@ -24,6 +24,7 @@ import org.apache.commons.jcs.engine.sta > import org.apache.commons.jcs.engine.stats.Stats; > import org.apache.commons.jcs.engine.stats.behavior.IStatElement; > import org.apache.commons.jcs.engine.stats.behavior.IStats; > +import org.apache.commons.jcs.utils.logger.LogHelper; > import org.apache.commons.logging.Log; > import org.apache.commons.logging.LogFactory; > > @@ -37,6 +38,8 @@ import java.util.Map; > import java.util.NoSuchElementException; > import java.util.Set; > import java.util.concurrent.ConcurrentHashMap; > +import java.util.concurrent.locks.Lock; > +import java.util.concurrent.locks.ReentrantLock; > > /** > * This is a simple LRUMap. It implements most of the map methods. It is not > recommended that you > @@ -57,6 +60,7 @@ public class LRUMap<K, V> > { > /** The logger */ > private static final Log log = LogFactory.getLog( LRUMap.class ); > + private static final LogHelper LOG_HELPER = new LogHelper(log); > > /** double linked list for lru */ > private final DoubleLinkedList<LRUElementDescriptor<K, V>> list; > @@ -79,6 +83,8 @@ public class LRUMap<K, V> > /** make configurable */ > private int chunkSize = 1; > > + private final Lock lock = new ReentrantLock(); > + > /** > * This creates an unbounded version. Setting the max objects will > result in spooling on > * subsequent puts. > @@ -123,8 +129,16 @@ public class LRUMap<K, V> > @Override > public void clear() > { > - map.clear(); > - list.removeAll(); > + lock.lock(); > + try > + { > + map.clear(); > + list.removeAll(); > + } > + finally > + { > + lock.unlock(); > + } > } > > /** > @@ -200,7 +214,7 @@ public class LRUMap<K, V> > { > V retVal = null; > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "getting item for key " + key ); > } > @@ -244,14 +258,14 @@ public class LRUMap<K, V> > LRUElementDescriptor<K, V> me = map.get( key ); > if ( me != null ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "LRUMap quiet hit for " + key ); > } > > ce = me.getPayload(); > } > - else if ( log.isDebugEnabled() ) > + else if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "LRUMap quiet miss for " + key ); > } > @@ -266,18 +280,26 @@ public class LRUMap<K, V> > @Override > public V remove( Object key ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "removing item for key: " + key ); > } > > // remove single item. > - LRUElementDescriptor<K, V> me = map.remove( key ); > + lock.lock(); > + try > + { > + LRUElementDescriptor<K, V> me = map.remove(key); > > - if ( me != null ) > + if (me != null) > + { > + list.remove(me); > + return me.getPayload(); > + } > + } > + finally > { > - list.remove( me ); > - return me.getPayload(); > + lock.unlock(); > } > > return null; > @@ -294,7 +316,8 @@ public class LRUMap<K, V> > putCnt++; > > LRUElementDescriptor<K, V> old = null; > - synchronized ( this ) > + lock.lock(); > + try > { > // TODO address double synchronization of addFirst, use write > lock > addFirst( key, value ); > @@ -308,13 +331,18 @@ public class LRUMap<K, V> > list.remove( old ); > } > } > + finally > + { > + lock.unlock(); > + } > > int size = map.size(); > // If the element limit is reached, we need to spool > > if ( this.maxObjects >= 0 && size > this.maxObjects ) > { > - if ( log.isDebugEnabled() ) > + final boolean debugEnabled = LOG_HELPER.isDebugEnabled(); > + if (debugEnabled) > { > log.debug( "In memory limit reached, removing least recently > used." ); > } > @@ -322,7 +350,7 @@ public class LRUMap<K, V> > // Write the last 'chunkSize' items to disk. > int chunkSizeCorrected = Math.min( size, getChunkSize() ); > > - if ( log.isDebugEnabled() ) > + if (debugEnabled) > { > log.debug( "About to remove the least recently used. map > size: " + size + ", max objects: " > + this.maxObjects + ", items to spool: " + > chunkSizeCorrected ); > @@ -334,33 +362,41 @@ public class LRUMap<K, V> > > for ( int i = 0; i < chunkSizeCorrected; i++ ) > { > - LRUElementDescriptor<K, V> last = list.getLast(); > - if ( last != null ) > + lock.lock(); > + try > { > - processRemovedLRU(last.getKey(), last.getPayload()); > - if ( map.remove( last.getKey() ) == null ) > + LRUElementDescriptor<K, V> last = list.getLast(); > + if (last != null) > + { > + processRemovedLRU(last.getKey(), last.getPayload()); > + if (map.remove(last.getKey()) == null) > + { > + log.warn("update: remove failed for key: " > + + last.getKey()); > + verifyCache(); > + } > + list.removeLast(); > + } > + else > { > - log.warn( "update: remove failed for key: " > - + last.getKey() ); > verifyCache(); > + throw new Error("update: last is null!"); > } > - list.remove(last); > } > - else > + finally > { > - verifyCache(); > - throw new Error( "update: last is null!" ); > + lock.unlock(); > } > } > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "update: After spool map size: " + map.size() ); > } > if ( map.size() != dumpCacheSize() ) > { > - log.error( "update: After spool, size mismatch: map.size() = > " + map.size() + ", linked list size = " > - + dumpCacheSize() ); > + log.error("update: After spool, size mismatch: map.size() = > " + map.size() + ", linked list size = " > + + dumpCacheSize()); > } > } > > @@ -379,8 +415,16 @@ public class LRUMap<K, V> > */ > private void addFirst(K key, V val) > { > - LRUElementDescriptor<K, V> me = new LRUElementDescriptor<K, V>(key, > val); > - list.addFirst( me ); > + lock.lock(); > + try > + { > + LRUElementDescriptor<K, V> me = new LRUElementDescriptor<K, > V>(key, val); > + list.addFirst( me ); > + } > + finally > + { > + lock.unlock(); > + } > } > > /** > @@ -402,7 +446,7 @@ public class LRUMap<K, V> > log.debug( "dumpingCacheEntries" ); > for ( LRUElementDescriptor<K, V> me = list.getFirst(); me != null; > me = (LRUElementDescriptor<K, V>) me.next ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "dumpCacheEntries> key=" + me.getKey() + ", val=" > + me.getPayload() ); > } > @@ -418,7 +462,7 @@ public class LRUMap<K, V> > for (Map.Entry<K, LRUElementDescriptor<K, V>> e : map.entrySet()) > { > LRUElementDescriptor<K, V> me = e.getValue(); > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "dumpMap> key=" + e.getKey() + ", val=" + > me.getPayload() ); > } > @@ -432,7 +476,7 @@ public class LRUMap<K, V> > @SuppressWarnings("unchecked") // No generics for public fields > protected void verifyCache() > { > - if ( !log.isDebugEnabled() ) > + if ( !LOG_HELPER.isDebugEnabled() ) > { > return; > } > @@ -524,7 +568,7 @@ public class LRUMap<K, V> > @SuppressWarnings("unchecked") // No generics for public fields > protected void verifyCache( Object key ) > { > - if ( !log.isDebugEnabled() ) > + if ( !LOG_HELPER.isDebugEnabled() ) > { > return; > } > @@ -556,7 +600,7 @@ public class LRUMap<K, V> > */ > protected void processRemovedLRU(K key, V value ) > { > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "Removing key: [" + key + "] from LRUMap store, value > = [" + value + "]" ); > log.debug( "LRUMap store size: '" + this.size() + "'." ); > @@ -615,18 +659,25 @@ public class LRUMap<K, V> > @Override > public Set<Map.Entry<K, V>> entrySet() > { > - // todo, we should return a defensive copy > - Set<Map.Entry<K, LRUElementDescriptor<K, V>>> entries = > map.entrySet(); > + lock.lock(); > + try > + { > + // todo, we should return a defensive copy > + Set<Map.Entry<K, LRUElementDescriptor<K, V>>> entries = > map.entrySet(); > > - Set<Map.Entry<K, V>> unWrapped = new HashSet<Map.Entry<K, V>>(); > + Set<Map.Entry<K, V>> unWrapped = new HashSet<Map.Entry<K, V>>(); > > - for (Map.Entry<K, LRUElementDescriptor<K, V>> pre : entries ) > + for (Map.Entry<K, LRUElementDescriptor<K, V>> pre : entries) { > + Map.Entry<K, V> post = new LRUMapEntry<K, V>(pre.getKey(), > pre.getValue().getPayload()); > + unWrapped.add(post); > + } > + > + return unWrapped; > + } > + finally > { > - Map.Entry<K, V> post = new LRUMapEntry<K, V>(pre.getKey(), > pre.getValue().getPayload()); > - unWrapped.add(post); > + lock.unlock(); > } > - > - return unWrapped; > } > > /** > > Modified: > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/SingleLinkedList.java > URL: > http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/SingleLinkedList.java?rev=1598071&r1=1598070&r2=1598071&view=diff > ============================================================================== > --- > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/SingleLinkedList.java > (original) > +++ > commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/utils/struct/SingleLinkedList.java > Wed May 28 17:06:12 2014 > @@ -19,6 +19,7 @@ package org.apache.commons.jcs.utils.str > * under the License. > */ > > +import org.apache.commons.jcs.utils.logger.LogHelper; > import org.apache.commons.logging.Log; > import org.apache.commons.logging.LogFactory; > > @@ -32,6 +33,7 @@ public class SingleLinkedList<T> > { > /** The logger */ > private static final Log log = LogFactory.getLog( SingleLinkedList.class > ); > + private static final LogHelper LOG_HELPER = new LogHelper(log); > > /** for sync */ > private final Object lock = new Object(); > @@ -64,7 +66,7 @@ public class SingleLinkedList<T> > > T value = node.payload; > > - if ( log.isDebugEnabled() ) > + if ( LOG_HELPER.isDebugEnabled() ) > { > log.debug( "head.payload = " + head.payload ); > log.debug( "node.payload = " + node.payload ); > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
