You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by rm...@apache.org on 2014/05/12 21:46:08 UTC
svn commit: r1594073 - in
/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Author: rmannibucau
Date: Mon May 12 19:46:08 2014
New Revision: 1594073
URL: http://svn.apache.org/r1594073
Log:
removing a bunch of synchronized thanks to ConcurrentHashMap - still removeAll to review since it can be not that good but using any synch would make it slower and not scalable
Modified:
commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.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/util/MemoryElementDescriptor.java
Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java?rev=1594073&r1=1594072&r2=1594073&view=diff
==============================================================================
--- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java (original)
+++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java Mon May 12 19:46:08 2014
@@ -81,7 +81,7 @@ public class RemoteCacheServerFactory
* @return Returns the remoteCacheServer.
*/
@SuppressWarnings("unchecked") // Need cast to specific RemoteCacheServer
- public static <K extends Serializable, V extends Serializable> RemoteCacheServer<K, V> getRemoteCacheServer()
+ public static <K, V> RemoteCacheServer<K, V> getRemoteCacheServer()
{
return (RemoteCacheServer<K, V>)remoteCacheServer;
}
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=1594073&r1=1594072&r2=1594073&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 Mon May 12 19:46:08 2014
@@ -86,8 +86,11 @@ public abstract class AbstractDoubleLink
/**
* This is called by super initialize.
+ *
+ * NOTE: should return a thread safe map
+ *
* <p>
- * @return new Hashtable()
+ * @return new ConcurrentHashMap()
*/
@Override
public Map<K, MemoryElementDescriptor<K, V>> createMap()
@@ -109,19 +112,15 @@ public abstract class AbstractDoubleLink
{
putCnt++;
- synchronized ( this )
- {
- // ABSTRACT
- MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
+ MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
- // this must be synchronized
- 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() ) )
- {
- list.remove( oldNode );
- }
+ // 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 );
}
// If we are over the max spool some
@@ -190,12 +189,14 @@ public abstract class AbstractDoubleLink
* @throws IOException
*/
@Override
- public final synchronized ICacheElement<K, V> get( K key )
+ public final ICacheElement<K, V> get( K key )
throws IOException
{
ICacheElement<K, V> ce = null;
- if ( log.isDebugEnabled() )
+ final boolean debugEnabled = log.isDebugEnabled();
+
+ if (debugEnabled)
{
log.debug( "getting item from cache " + cacheName + " for key " + key );
}
@@ -206,7 +207,7 @@ public abstract class AbstractDoubleLink
{
ce = me.ce;
hitCnt++;
- if ( log.isDebugEnabled() )
+ if (debugEnabled)
{
log.debug( cacheName + ": LRUMemoryCache hit for " + ce.getKey() );
}
@@ -217,7 +218,7 @@ public abstract class AbstractDoubleLink
else
{
missCnt++;
- if ( log.isDebugEnabled() )
+ if (debugEnabled)
{
log.debug( cacheName + ": LRUMemoryCache miss for " + key );
}
@@ -270,46 +271,38 @@ public abstract class AbstractDoubleLink
throws Error
{
ICacheElement<K, V> toSpool = null;
- synchronized ( this )
+ final MemoryElementDescriptor<K, V> last = list.getLast();
+ if ( last != null )
{
- if ( list.getLast() != null )
+ toSpool = last.ce;
+ if ( toSpool != null )
{
- toSpool = list.getLast().ce;
- if ( toSpool != null )
- {
- cache.spoolToDisk( list.getLast().ce );
- if ( !map.containsKey( list.getLast().ce.getKey() ) )
- {
- log.error( "update: map does not contain key: "
- + list.getLast().ce.getKey() );
- verifyCache();
- }
- if ( map.remove( list.getLast().ce.getKey() ) == null )
- {
- log.warn( "update: remove failed for key: "
- + list.getLast().ce.getKey() );
- verifyCache();
- }
- }
- else
+ cache.spoolToDisk( last.ce );
+ if ( map.remove( last.ce.getKey() ) == null )
{
- throw new Error( "update: last.ce is null!" );
+ log.warn( "update: remove failed for key: "
+ + last.ce.getKey() );
+ verifyCache();
}
- list.removeLast();
}
else
{
- verifyCache();
- throw new Error( "update: last is null!" );
+ throw new Error( "update: last.ce is null!" );
}
+ list.remove(last);
+ }
+ else
+ {
+ verifyCache();
+ throw new Error( "update: last is null!" );
+ }
- // If this is out of the sync block it can detect a mismatch
- // where there is none.
- if ( map.size() != dumpCacheSize() )
- {
- log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
- + dumpCacheSize() );
- }
+ // If this is out of the sync block it can detect a mismatch
+ // where there is none.
+ if ( map.size() != dumpCacheSize() )
+ {
+ log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
+ + dumpCacheSize() );
}
return toSpool;
}
@@ -324,7 +317,7 @@ public abstract class AbstractDoubleLink
* @throws IOException
*/
@Override
- public synchronized boolean remove( K key )
+ public boolean remove( K key )
throws IOException
{
if ( log.isDebugEnabled() )
@@ -338,39 +331,33 @@ public abstract class AbstractDoubleLink
if ( key instanceof String && ( (String) key ).endsWith( CacheConstants.NAME_COMPONENT_DELIMITER ) )
{
// remove all keys of the same name hierarchy.
- synchronized ( map )
+ for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
{
- for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
- {
- Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
- K k = entry.getKey();
+ Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
+ K k = entry.getKey();
- if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
- {
- list.remove( entry.getValue() );
- itr.remove();
- removed = true;
- }
+ if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
+ {
+ list.remove( entry.getValue() );
+ itr.remove();
+ removed = true;
}
}
}
else if ( key instanceof GroupAttrName )
{
// remove all keys of the same name hierarchy.
- synchronized ( map )
+ for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
{
- for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
- {
- Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
- K k = entry.getKey();
+ Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
+ K k = entry.getKey();
- if ( k instanceof GroupAttrName &&
- ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
- {
- list.remove( entry.getValue() );
- itr.remove();
- removed = true;
- }
+ if ( k instanceof GroupAttrName &&
+ ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
+ {
+ list.remove( entry.getValue() );
+ itr.remove();
+ removed = true;
}
}
}
@@ -396,7 +383,7 @@ public abstract class AbstractDoubleLink
* @throws IOException
*/
@Override
- public synchronized void removeAll()
+ public void removeAll()
throws IOException
{
map.clear();
@@ -410,7 +397,7 @@ public abstract class AbstractDoubleLink
* @param ce The feature to be added to the First
* @return MemoryElementDescriptor
*/
- protected synchronized MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
+ protected MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
{
MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
list.addFirst( me );
@@ -424,7 +411,7 @@ public abstract class AbstractDoubleLink
* @param ce The feature to be added to the First
* @return MemoryElementDescriptor
*/
- protected synchronized MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
+ protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
{
MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
list.addLast( me );
Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java?rev=1594073&r1=1594072&r2=1594073&view=diff
==============================================================================
--- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java (original)
+++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java Mon May 12 19:46:08 2014
@@ -32,7 +32,7 @@ public class MemoryElementDescriptor<K,
private static final long serialVersionUID = -1905161209035522460L;
/** The CacheElement wrapped by this descriptor */
- public ICacheElement<K, V> ce; // TODO privatise
+ public final ICacheElement<K, V> ce; // TODO privatise
/**
* Constructs a usable MemoryElementDescriptor.
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Hi
we should be as before now excepted we use ReentrantLock instead of
synchronized and method using a single structure (actually the map in
practice) which relies on ConcurrentHashMap thread safety.
Build passes locally (not sure why continuum is not happy but it was the
case before).
BTW I have a question, I wanted to compare performances between previous
jcs and trunk and I first used org.apache.jcs:jcs but it seems it is old
(performance were 4000 times slower. Did I do something wrong, what is the
last release I should compare to the current trunk?
Romain Manni-Bucau
Twitter: @rmannibucau
Blog: http://rmannibucau.wordpress.com/
LinkedIn: http://fr.linkedin.com/in/rmannibucau
Github: https://github.com/rmannibucau
2014-05-29 10:47 GMT+02:00 Thomas Vandahl <tv...@apache.org>:
> On 28.05.14 13:47, Romain Manni-Bucau wrote:
> > I'm convinced of it (why the comment was this way). I'll try to rework it
> > this week based on the LockFactory we spoke about in another thread.
>
> Please try to make sure that your conviction is proven by some unit test.
>
> Bye, Thomas.
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Thomas Vandahl <tv...@apache.org>.
On 28.05.14 13:47, Romain Manni-Bucau wrote:
> I'm convinced of it (why the comment was this way). I'll try to rework it
> this week based on the LockFactory we spoke about in another thread.
Please try to make sure that your conviction is proven by some unit test.
Bye, Thomas.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Phil Steitz <ph...@gmail.com>.
On 5/28/14, 4:47 AM, Romain Manni-Bucau wrote:
> I'm convinced of it (why the comment was this way). I'll try to rework it
> this week based on the LockFactory we spoke about in another thread.
Great. Would would be good to explain when implementing these
changes is:
0) what sequences of operations were performed atomically in sync
blocks before
1) why the atomicity is not necessary, if the sycn protection
(whether using synchronized blocks or explicit locks) is removed
2) what invariants JCS maintained before the changes and after
Then people can weigh in on whether the changes are OK, based on
known use cases and comfort level changing contracts (if
necessary). Ideal is to lay this out before committing changes so
you don't have to deal with -1's; but we are CTR so OK to commit
first and then explain after if that is easier. It is also better
if you can break large commits up if possible and break changes into
separate JIRAs where possible.
Phil
>
>
> Romain Manni-Bucau
> Twitter: @rmannibucau
> Blog: http://rmannibucau.wordpress.com/
> LinkedIn: http://fr.linkedin.com/in/rmannibucau
> Github: https://github.com/rmannibucau
>
>
> 2014-05-28 2:19 GMT+02:00 Phil Steitz <ph...@gmail.com>:
>
>> On 5/27/14, 11:17 AM, Benedikt Ritter wrote:
>>> Send from my mobile device
>>>
>>>> Am 13.05.2014 um 20:53 schrieb Phil Steitz <ph...@gmail.com>:
>>>>
>>>>> On 5/12/14, 12:46 PM, rmannibucau@apache.org wrote:
>>>>> Author: rmannibucau
>>>>> Date: Mon May 12 19:46:08 2014
>>>>> New Revision: 1594073
>>>>>
>>>>> URL: http://svn.apache.org/r1594073
>>>>> Log:
>>>>> removing a bunch of synchronized thanks to ConcurrentHashMap - still
>> removeAll to review since it can be not that good but using any synch would
>> make it slower and not scalable
>>>>> Modified:
>>>>>
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.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/util/MemoryElementDescriptor.java
>>>>> Modified:
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
>>>>> URL:
>> http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java?rev=1594073&r1=1594072&r2=1594073&view=diff
>> ==============================================================================
>>>>> ---
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
>> (original)
>>>>> +++
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
>> Mon May 12 19:46:08 2014
>>>>> @@ -81,7 +81,7 @@ public class RemoteCacheServerFactory
>>>>> * @return Returns the remoteCacheServer.
>>>>> */
>>>>> @SuppressWarnings("unchecked") // Need cast to specific
>> RemoteCacheServer
>>>>> - public static <K extends Serializable, V extends Serializable>
>> RemoteCacheServer<K, V> getRemoteCacheServer()
>>>>> + public static <K, V> RemoteCacheServer<K, V>
>> getRemoteCacheServer()
>>>>> {
>>>>> return (RemoteCacheServer<K, V>)remoteCacheServer;
>>>>> }
>>>>>
>>>>> 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=1594073&r1=1594072&r2=1594073&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
>> Mon May 12 19:46:08 2014
>>>>> @@ -86,8 +86,11 @@ public abstract class AbstractDoubleLink
>>>>>
>>>>> /**
>>>>> * This is called by super initialize.
>>>>> + *
>>>>> + * NOTE: should return a thread safe map
>>>>> + *
>>>>> * <p>
>>>>> - * @return new Hashtable()
>>>>> + * @return new ConcurrentHashMap()
>>>>> */
>>>>> @Override
>>>>> public Map<K, MemoryElementDescriptor<K, V>> createMap()
>>>>> @@ -109,19 +112,15 @@ public abstract class AbstractDoubleLink
>>>>> {
>>>>> putCnt++;
>>>>>
>>>>> - synchronized ( this )
>>>> I am not really familiar with this code, so this could be needless
>>>> concern; but removing this synch makes the adjustListForUpdate no
>>>> longer atomic with the put below. Is this OK?
>>> Sorry, I seem to have missed the answer to this question. Was it okay to
>> remove the synchronized?
>>
>> I don't know. That's why I asked if there were a locking / data
>> protection strategy implicit in the code and what data invariants
>> need to be maintained. This case should be straightforward. You
>> just need to convince yourself that the operations removed from the
>> sync block can safely be performed by multiple threads with no
>> atomicity guarantees.
>>
>> Phil
>>> Bene
>>>
>>>> Phil
>>>>> - {
>>>>> - // ABSTRACT
>>>>> - MemoryElementDescriptor<K, V> newNode =
>> adjustListForUpdate( ce );
>>>>> + MemoryElementDescriptor<K, V> newNode = adjustListForUpdate(
>> ce );
>>>>> - // this must be synchronized
>>>>> - 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() ) )
>>>>> - {
>>>>> - list.remove( oldNode );
>>>>> - }
>>>>> + // 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 );
>>>>> }
>>>>>
>>>>> // If we are over the max spool some
>>>>> @@ -190,12 +189,14 @@ public abstract class AbstractDoubleLink
>>>>> * @throws IOException
>>>>> */
>>>>> @Override
>>>>> - public final synchronized ICacheElement<K, V> get( K key )
>>>>> + public final ICacheElement<K, V> get( K key )
>>>>> throws IOException
>>>>> {
>>>>> ICacheElement<K, V> ce = null;
>>>>>
>>>>> - if ( log.isDebugEnabled() )
>>>>> + final boolean debugEnabled = log.isDebugEnabled();
>>>>> +
>>>>> + if (debugEnabled)
>>>>> {
>>>>> log.debug( "getting item from cache " + cacheName + " for
>> key " + key );
>>>>> }
>>>>> @@ -206,7 +207,7 @@ public abstract class AbstractDoubleLink
>>>>> {
>>>>> ce = me.ce;
>>>>> hitCnt++;
>>>>> - if ( log.isDebugEnabled() )
>>>>> + if (debugEnabled)
>>>>> {
>>>>> log.debug( cacheName + ": LRUMemoryCache hit for " +
>> ce.getKey() );
>>>>> }
>>>>> @@ -217,7 +218,7 @@ public abstract class AbstractDoubleLink
>>>>> else
>>>>> {
>>>>> missCnt++;
>>>>> - if ( log.isDebugEnabled() )
>>>>> + if (debugEnabled)
>>>>> {
>>>>> log.debug( cacheName + ": LRUMemoryCache miss for " +
>> key );
>>>>> }
>>>>> @@ -270,46 +271,38 @@ public abstract class AbstractDoubleLink
>>>>> throws Error
>>>>> {
>>>>> ICacheElement<K, V> toSpool = null;
>>>>> - synchronized ( this )
>>>>> + final MemoryElementDescriptor<K, V> last = list.getLast();
>>>>> + if ( last != null )
>>>>> {
>>>>> - if ( list.getLast() != null )
>>>>> + toSpool = last.ce;
>>>>> + if ( toSpool != null )
>>>>> {
>>>>> - toSpool = list.getLast().ce;
>>>>> - if ( toSpool != null )
>>>>> - {
>>>>> - cache.spoolToDisk( list.getLast().ce );
>>>>> - if ( !map.containsKey( list.getLast().ce.getKey()
>> ) )
>>>>> - {
>>>>> - log.error( "update: map does not contain key:
>> "
>>>>> - + list.getLast().ce.getKey() );
>>>>> - verifyCache();
>>>>> - }
>>>>> - if ( map.remove( list.getLast().ce.getKey() ) ==
>> null )
>>>>> - {
>>>>> - log.warn( "update: remove failed for key: "
>>>>> - + list.getLast().ce.getKey() );
>>>>> - verifyCache();
>>>>> - }
>>>>> - }
>>>>> - else
>>>>> + cache.spoolToDisk( last.ce );
>>>>> + if ( map.remove( last.ce.getKey() ) == null )
>>>>> {
>>>>> - throw new Error( "update: last.ce is null!" );
>>>>> + log.warn( "update: remove failed for key: "
>>>>> + + last.ce.getKey() );
>>>>> + verifyCache();
>>>>> }
>>>>> - list.removeLast();
>>>>> }
>>>>> else
>>>>> {
>>>>> - verifyCache();
>>>>> - throw new Error( "update: last is null!" );
>>>>> + throw new Error( "update: last.ce is null!" );
>>>>> }
>>>>> + list.remove(last);
>>>>> + }
>>>>> + else
>>>>> + {
>>>>> + verifyCache();
>>>>> + throw new Error( "update: last is null!" );
>>>>> + }
>>>>>
>>>>> - // If this is out of the sync block it can detect a
>> mismatch
>>>>> - // where there is none.
>>>>> - if ( map.size() != dumpCacheSize() )
>>>>> - {
>>>>> - log.warn( "update: After spool, size mismatch:
>> map.size() = " + map.size() + ", linked list size = "
>>>>> - + dumpCacheSize() );
>>>>> - }
>>>>> + // If this is out of the sync block it can detect a mismatch
>>>>> + // where there is none.
>>>>> + if ( map.size() != dumpCacheSize() )
>>>>> + {
>>>>> + log.warn( "update: After spool, size mismatch: map.size()
>> = " + map.size() + ", linked list size = "
>>>>> + + dumpCacheSize() );
>>>>> }
>>>>> return toSpool;
>>>>> }
>>>>> @@ -324,7 +317,7 @@ public abstract class AbstractDoubleLink
>>>>> * @throws IOException
>>>>> */
>>>>> @Override
>>>>> - public synchronized boolean remove( K key )
>>>>> + public boolean remove( K key )
>>>>> throws IOException
>>>>> {
>>>>> if ( log.isDebugEnabled() )
>>>>> @@ -338,39 +331,33 @@ public abstract class AbstractDoubleLink
>>>>> if ( key instanceof String && ( (String) key ).endsWith(
>> CacheConstants.NAME_COMPONENT_DELIMITER ) )
>>>>> {
>>>>> // remove all keys of the same name hierarchy.
>>>>> - synchronized ( map )
>>>>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
>> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>>>> {
>>>>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
>> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>>>> - {
>>>>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry
>> = itr.next();
>>>>> - K k = entry.getKey();
>>>>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry =
>> itr.next();
>>>>> + K k = entry.getKey();
>>>>>
>>>>> - if ( k instanceof String && ( (String) k
>> ).startsWith( key.toString() ) )
>>>>> - {
>>>>> - list.remove( entry.getValue() );
>>>>> - itr.remove();
>>>>> - removed = true;
>>>>> - }
>>>>> + if ( k instanceof String && ( (String) k
>> ).startsWith( key.toString() ) )
>>>>> + {
>>>>> + list.remove( entry.getValue() );
>>>>> + itr.remove();
>>>>> + removed = true;
>>>>> }
>>>>> }
>>>>> }
>>>>> else if ( key instanceof GroupAttrName )
>>>>> {
>>>>> // remove all keys of the same name hierarchy.
>>>>> - synchronized ( map )
>>>>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
>> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>>>> {
>>>>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
>> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>>>> - {
>>>>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry
>> = itr.next();
>>>>> - K k = entry.getKey();
>>>>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry =
>> itr.next();
>>>>> + K k = entry.getKey();
>>>>>
>>>>> - if ( k instanceof GroupAttrName &&
>>>>> -
>> ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
>>>>> - {
>>>>> - list.remove( entry.getValue() );
>>>>> - itr.remove();
>>>>> - removed = true;
>>>>> - }
>>>>> + if ( k instanceof GroupAttrName &&
>>>>> +
>> ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
>>>>> + {
>>>>> + list.remove( entry.getValue() );
>>>>> + itr.remove();
>>>>> + removed = true;
>>>>> }
>>>>> }
>>>>> }
>>>>> @@ -396,7 +383,7 @@ public abstract class AbstractDoubleLink
>>>>> * @throws IOException
>>>>> */
>>>>> @Override
>>>>> - public synchronized void removeAll()
>>>>> + public void removeAll()
>>>>> throws IOException
>>>>> {
>>>>> map.clear();
>>>>> @@ -410,7 +397,7 @@ public abstract class AbstractDoubleLink
>>>>> * @param ce The feature to be added to the First
>>>>> * @return MemoryElementDescriptor
>>>>> */
>>>>> - protected synchronized MemoryElementDescriptor<K, V> addFirst(
>> ICacheElement<K, V> ce )
>>>>> + protected MemoryElementDescriptor<K, V> addFirst(
>> ICacheElement<K, V> ce )
>>>>> {
>>>>> MemoryElementDescriptor<K, V> me = new
>> MemoryElementDescriptor<K, V>( ce );
>>>>> list.addFirst( me );
>>>>> @@ -424,7 +411,7 @@ public abstract class AbstractDoubleLink
>>>>> * @param ce The feature to be added to the First
>>>>> * @return MemoryElementDescriptor
>>>>> */
>>>>> - protected synchronized MemoryElementDescriptor<K, V> addLast(
>> ICacheElement<K, V> ce )
>>>>> + protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K,
>> V> ce )
>>>>> {
>>>>> MemoryElementDescriptor<K, V> me = new
>> MemoryElementDescriptor<K, V>( ce );
>>>>> list.addLast( me );
>>>>>
>>>>> Modified:
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
>>>>> URL:
>> http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java?rev=1594073&r1=1594072&r2=1594073&view=diff
>> ==============================================================================
>>>>> ---
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
>> (original)
>>>>> +++
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
>> Mon May 12 19:46:08 2014
>>>>> @@ -32,7 +32,7 @@ public class MemoryElementDescriptor<K,
>>>>> private static final long serialVersionUID = -1905161209035522460L;
>>>>>
>>>>> /** The CacheElement wrapped by this descriptor */
>>>>> - public ICacheElement<K, V> ce; // TODO privatise
>>>>> + public final ICacheElement<K, V> ce; // TODO privatise
>>>>>
>>>>> /**
>>>>> * Constructs a usable MemoryElementDescriptor.
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Romain Manni-Bucau <rm...@gmail.com>.
I'm convinced of it (why the comment was this way). I'll try to rework it
this week based on the LockFactory we spoke about in another thread.
Romain Manni-Bucau
Twitter: @rmannibucau
Blog: http://rmannibucau.wordpress.com/
LinkedIn: http://fr.linkedin.com/in/rmannibucau
Github: https://github.com/rmannibucau
2014-05-28 2:19 GMT+02:00 Phil Steitz <ph...@gmail.com>:
> On 5/27/14, 11:17 AM, Benedikt Ritter wrote:
> >
> > Send from my mobile device
> >
> >> Am 13.05.2014 um 20:53 schrieb Phil Steitz <ph...@gmail.com>:
> >>
> >>> On 5/12/14, 12:46 PM, rmannibucau@apache.org wrote:
> >>> Author: rmannibucau
> >>> Date: Mon May 12 19:46:08 2014
> >>> New Revision: 1594073
> >>>
> >>> URL: http://svn.apache.org/r1594073
> >>> Log:
> >>> removing a bunch of synchronized thanks to ConcurrentHashMap - still
> removeAll to review since it can be not that good but using any synch would
> make it slower and not scalable
> >>>
> >>> Modified:
> >>>
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.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/util/MemoryElementDescriptor.java
> >>>
> >>> Modified:
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
> >>> URL:
> http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java?rev=1594073&r1=1594072&r2=1594073&view=diff
> >>>
> ==============================================================================
> >>> ---
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
> (original)
> >>> +++
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
> Mon May 12 19:46:08 2014
> >>> @@ -81,7 +81,7 @@ public class RemoteCacheServerFactory
> >>> * @return Returns the remoteCacheServer.
> >>> */
> >>> @SuppressWarnings("unchecked") // Need cast to specific
> RemoteCacheServer
> >>> - public static <K extends Serializable, V extends Serializable>
> RemoteCacheServer<K, V> getRemoteCacheServer()
> >>> + public static <K, V> RemoteCacheServer<K, V>
> getRemoteCacheServer()
> >>> {
> >>> return (RemoteCacheServer<K, V>)remoteCacheServer;
> >>> }
> >>>
> >>> 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=1594073&r1=1594072&r2=1594073&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
> Mon May 12 19:46:08 2014
> >>> @@ -86,8 +86,11 @@ public abstract class AbstractDoubleLink
> >>>
> >>> /**
> >>> * This is called by super initialize.
> >>> + *
> >>> + * NOTE: should return a thread safe map
> >>> + *
> >>> * <p>
> >>> - * @return new Hashtable()
> >>> + * @return new ConcurrentHashMap()
> >>> */
> >>> @Override
> >>> public Map<K, MemoryElementDescriptor<K, V>> createMap()
> >>> @@ -109,19 +112,15 @@ public abstract class AbstractDoubleLink
> >>> {
> >>> putCnt++;
> >>>
> >>> - synchronized ( this )
> >> I am not really familiar with this code, so this could be needless
> >> concern; but removing this synch makes the adjustListForUpdate no
> >> longer atomic with the put below. Is this OK?
> > Sorry, I seem to have missed the answer to this question. Was it okay to
> remove the synchronized?
>
> I don't know. That's why I asked if there were a locking / data
> protection strategy implicit in the code and what data invariants
> need to be maintained. This case should be straightforward. You
> just need to convince yourself that the operations removed from the
> sync block can safely be performed by multiple threads with no
> atomicity guarantees.
>
> Phil
> >
> > Bene
> >
> >> Phil
> >>> - {
> >>> - // ABSTRACT
> >>> - MemoryElementDescriptor<K, V> newNode =
> adjustListForUpdate( ce );
> >>> + MemoryElementDescriptor<K, V> newNode = adjustListForUpdate(
> ce );
> >>>
> >>> - // this must be synchronized
> >>> - 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() ) )
> >>> - {
> >>> - list.remove( oldNode );
> >>> - }
> >>> + // 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 );
> >>> }
> >>>
> >>> // If we are over the max spool some
> >>> @@ -190,12 +189,14 @@ public abstract class AbstractDoubleLink
> >>> * @throws IOException
> >>> */
> >>> @Override
> >>> - public final synchronized ICacheElement<K, V> get( K key )
> >>> + public final ICacheElement<K, V> get( K key )
> >>> throws IOException
> >>> {
> >>> ICacheElement<K, V> ce = null;
> >>>
> >>> - if ( log.isDebugEnabled() )
> >>> + final boolean debugEnabled = log.isDebugEnabled();
> >>> +
> >>> + if (debugEnabled)
> >>> {
> >>> log.debug( "getting item from cache " + cacheName + " for
> key " + key );
> >>> }
> >>> @@ -206,7 +207,7 @@ public abstract class AbstractDoubleLink
> >>> {
> >>> ce = me.ce;
> >>> hitCnt++;
> >>> - if ( log.isDebugEnabled() )
> >>> + if (debugEnabled)
> >>> {
> >>> log.debug( cacheName + ": LRUMemoryCache hit for " +
> ce.getKey() );
> >>> }
> >>> @@ -217,7 +218,7 @@ public abstract class AbstractDoubleLink
> >>> else
> >>> {
> >>> missCnt++;
> >>> - if ( log.isDebugEnabled() )
> >>> + if (debugEnabled)
> >>> {
> >>> log.debug( cacheName + ": LRUMemoryCache miss for " +
> key );
> >>> }
> >>> @@ -270,46 +271,38 @@ public abstract class AbstractDoubleLink
> >>> throws Error
> >>> {
> >>> ICacheElement<K, V> toSpool = null;
> >>> - synchronized ( this )
> >>> + final MemoryElementDescriptor<K, V> last = list.getLast();
> >>> + if ( last != null )
> >>> {
> >>> - if ( list.getLast() != null )
> >>> + toSpool = last.ce;
> >>> + if ( toSpool != null )
> >>> {
> >>> - toSpool = list.getLast().ce;
> >>> - if ( toSpool != null )
> >>> - {
> >>> - cache.spoolToDisk( list.getLast().ce );
> >>> - if ( !map.containsKey( list.getLast().ce.getKey()
> ) )
> >>> - {
> >>> - log.error( "update: map does not contain key:
> "
> >>> - + list.getLast().ce.getKey() );
> >>> - verifyCache();
> >>> - }
> >>> - if ( map.remove( list.getLast().ce.getKey() ) ==
> null )
> >>> - {
> >>> - log.warn( "update: remove failed for key: "
> >>> - + list.getLast().ce.getKey() );
> >>> - verifyCache();
> >>> - }
> >>> - }
> >>> - else
> >>> + cache.spoolToDisk( last.ce );
> >>> + if ( map.remove( last.ce.getKey() ) == null )
> >>> {
> >>> - throw new Error( "update: last.ce is null!" );
> >>> + log.warn( "update: remove failed for key: "
> >>> + + last.ce.getKey() );
> >>> + verifyCache();
> >>> }
> >>> - list.removeLast();
> >>> }
> >>> else
> >>> {
> >>> - verifyCache();
> >>> - throw new Error( "update: last is null!" );
> >>> + throw new Error( "update: last.ce is null!" );
> >>> }
> >>> + list.remove(last);
> >>> + }
> >>> + else
> >>> + {
> >>> + verifyCache();
> >>> + throw new Error( "update: last is null!" );
> >>> + }
> >>>
> >>> - // If this is out of the sync block it can detect a
> mismatch
> >>> - // where there is none.
> >>> - if ( map.size() != dumpCacheSize() )
> >>> - {
> >>> - log.warn( "update: After spool, size mismatch:
> map.size() = " + map.size() + ", linked list size = "
> >>> - + dumpCacheSize() );
> >>> - }
> >>> + // If this is out of the sync block it can detect a mismatch
> >>> + // where there is none.
> >>> + if ( map.size() != dumpCacheSize() )
> >>> + {
> >>> + log.warn( "update: After spool, size mismatch: map.size()
> = " + map.size() + ", linked list size = "
> >>> + + dumpCacheSize() );
> >>> }
> >>> return toSpool;
> >>> }
> >>> @@ -324,7 +317,7 @@ public abstract class AbstractDoubleLink
> >>> * @throws IOException
> >>> */
> >>> @Override
> >>> - public synchronized boolean remove( K key )
> >>> + public boolean remove( K key )
> >>> throws IOException
> >>> {
> >>> if ( log.isDebugEnabled() )
> >>> @@ -338,39 +331,33 @@ public abstract class AbstractDoubleLink
> >>> if ( key instanceof String && ( (String) key ).endsWith(
> CacheConstants.NAME_COMPONENT_DELIMITER ) )
> >>> {
> >>> // remove all keys of the same name hierarchy.
> >>> - synchronized ( map )
> >>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> >>> {
> >>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> >>> - {
> >>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry
> = itr.next();
> >>> - K k = entry.getKey();
> >>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry =
> itr.next();
> >>> + K k = entry.getKey();
> >>>
> >>> - if ( k instanceof String && ( (String) k
> ).startsWith( key.toString() ) )
> >>> - {
> >>> - list.remove( entry.getValue() );
> >>> - itr.remove();
> >>> - removed = true;
> >>> - }
> >>> + if ( k instanceof String && ( (String) k
> ).startsWith( key.toString() ) )
> >>> + {
> >>> + list.remove( entry.getValue() );
> >>> + itr.remove();
> >>> + removed = true;
> >>> }
> >>> }
> >>> }
> >>> else if ( key instanceof GroupAttrName )
> >>> {
> >>> // remove all keys of the same name hierarchy.
> >>> - synchronized ( map )
> >>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> >>> {
> >>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K,
> V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> >>> - {
> >>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry
> = itr.next();
> >>> - K k = entry.getKey();
> >>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry =
> itr.next();
> >>> + K k = entry.getKey();
> >>>
> >>> - if ( k instanceof GroupAttrName &&
> >>> -
> ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
> >>> - {
> >>> - list.remove( entry.getValue() );
> >>> - itr.remove();
> >>> - removed = true;
> >>> - }
> >>> + if ( k instanceof GroupAttrName &&
> >>> +
> ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
> >>> + {
> >>> + list.remove( entry.getValue() );
> >>> + itr.remove();
> >>> + removed = true;
> >>> }
> >>> }
> >>> }
> >>> @@ -396,7 +383,7 @@ public abstract class AbstractDoubleLink
> >>> * @throws IOException
> >>> */
> >>> @Override
> >>> - public synchronized void removeAll()
> >>> + public void removeAll()
> >>> throws IOException
> >>> {
> >>> map.clear();
> >>> @@ -410,7 +397,7 @@ public abstract class AbstractDoubleLink
> >>> * @param ce The feature to be added to the First
> >>> * @return MemoryElementDescriptor
> >>> */
> >>> - protected synchronized MemoryElementDescriptor<K, V> addFirst(
> ICacheElement<K, V> ce )
> >>> + protected MemoryElementDescriptor<K, V> addFirst(
> ICacheElement<K, V> ce )
> >>> {
> >>> MemoryElementDescriptor<K, V> me = new
> MemoryElementDescriptor<K, V>( ce );
> >>> list.addFirst( me );
> >>> @@ -424,7 +411,7 @@ public abstract class AbstractDoubleLink
> >>> * @param ce The feature to be added to the First
> >>> * @return MemoryElementDescriptor
> >>> */
> >>> - protected synchronized MemoryElementDescriptor<K, V> addLast(
> ICacheElement<K, V> ce )
> >>> + protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K,
> V> ce )
> >>> {
> >>> MemoryElementDescriptor<K, V> me = new
> MemoryElementDescriptor<K, V>( ce );
> >>> list.addLast( me );
> >>>
> >>> Modified:
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
> >>> URL:
> http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java?rev=1594073&r1=1594072&r2=1594073&view=diff
> >>>
> ==============================================================================
> >>> ---
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
> (original)
> >>> +++
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
> Mon May 12 19:46:08 2014
> >>> @@ -32,7 +32,7 @@ public class MemoryElementDescriptor<K,
> >>> private static final long serialVersionUID = -1905161209035522460L;
> >>>
> >>> /** The CacheElement wrapped by this descriptor */
> >>> - public ICacheElement<K, V> ce; // TODO privatise
> >>> + public final ICacheElement<K, V> ce; // TODO privatise
> >>>
> >>> /**
> >>> * Constructs a usable MemoryElementDescriptor.
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Phil Steitz <ph...@gmail.com>.
On 5/27/14, 11:17 AM, Benedikt Ritter wrote:
>
> Send from my mobile device
>
>> Am 13.05.2014 um 20:53 schrieb Phil Steitz <ph...@gmail.com>:
>>
>>> On 5/12/14, 12:46 PM, rmannibucau@apache.org wrote:
>>> Author: rmannibucau
>>> Date: Mon May 12 19:46:08 2014
>>> New Revision: 1594073
>>>
>>> URL: http://svn.apache.org/r1594073
>>> Log:
>>> removing a bunch of synchronized thanks to ConcurrentHashMap - still removeAll to review since it can be not that good but using any synch would make it slower and not scalable
>>>
>>> Modified:
>>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.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/util/MemoryElementDescriptor.java
>>>
>>> Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
>>> URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java?rev=1594073&r1=1594072&r2=1594073&view=diff
>>> ==============================================================================
>>> --- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java (original)
>>> +++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java Mon May 12 19:46:08 2014
>>> @@ -81,7 +81,7 @@ public class RemoteCacheServerFactory
>>> * @return Returns the remoteCacheServer.
>>> */
>>> @SuppressWarnings("unchecked") // Need cast to specific RemoteCacheServer
>>> - public static <K extends Serializable, V extends Serializable> RemoteCacheServer<K, V> getRemoteCacheServer()
>>> + public static <K, V> RemoteCacheServer<K, V> getRemoteCacheServer()
>>> {
>>> return (RemoteCacheServer<K, V>)remoteCacheServer;
>>> }
>>>
>>> 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=1594073&r1=1594072&r2=1594073&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 Mon May 12 19:46:08 2014
>>> @@ -86,8 +86,11 @@ public abstract class AbstractDoubleLink
>>>
>>> /**
>>> * This is called by super initialize.
>>> + *
>>> + * NOTE: should return a thread safe map
>>> + *
>>> * <p>
>>> - * @return new Hashtable()
>>> + * @return new ConcurrentHashMap()
>>> */
>>> @Override
>>> public Map<K, MemoryElementDescriptor<K, V>> createMap()
>>> @@ -109,19 +112,15 @@ public abstract class AbstractDoubleLink
>>> {
>>> putCnt++;
>>>
>>> - synchronized ( this )
>> I am not really familiar with this code, so this could be needless
>> concern; but removing this synch makes the adjustListForUpdate no
>> longer atomic with the put below. Is this OK?
> Sorry, I seem to have missed the answer to this question. Was it okay to remove the synchronized?
I don't know. That's why I asked if there were a locking / data
protection strategy implicit in the code and what data invariants
need to be maintained. This case should be straightforward. You
just need to convince yourself that the operations removed from the
sync block can safely be performed by multiple threads with no
atomicity guarantees.
Phil
>
> Bene
>
>> Phil
>>> - {
>>> - // ABSTRACT
>>> - MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
>>> + MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
>>>
>>> - // this must be synchronized
>>> - 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() ) )
>>> - {
>>> - list.remove( oldNode );
>>> - }
>>> + // 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 );
>>> }
>>>
>>> // If we are over the max spool some
>>> @@ -190,12 +189,14 @@ public abstract class AbstractDoubleLink
>>> * @throws IOException
>>> */
>>> @Override
>>> - public final synchronized ICacheElement<K, V> get( K key )
>>> + public final ICacheElement<K, V> get( K key )
>>> throws IOException
>>> {
>>> ICacheElement<K, V> ce = null;
>>>
>>> - if ( log.isDebugEnabled() )
>>> + final boolean debugEnabled = log.isDebugEnabled();
>>> +
>>> + if (debugEnabled)
>>> {
>>> log.debug( "getting item from cache " + cacheName + " for key " + key );
>>> }
>>> @@ -206,7 +207,7 @@ public abstract class AbstractDoubleLink
>>> {
>>> ce = me.ce;
>>> hitCnt++;
>>> - if ( log.isDebugEnabled() )
>>> + if (debugEnabled)
>>> {
>>> log.debug( cacheName + ": LRUMemoryCache hit for " + ce.getKey() );
>>> }
>>> @@ -217,7 +218,7 @@ public abstract class AbstractDoubleLink
>>> else
>>> {
>>> missCnt++;
>>> - if ( log.isDebugEnabled() )
>>> + if (debugEnabled)
>>> {
>>> log.debug( cacheName + ": LRUMemoryCache miss for " + key );
>>> }
>>> @@ -270,46 +271,38 @@ public abstract class AbstractDoubleLink
>>> throws Error
>>> {
>>> ICacheElement<K, V> toSpool = null;
>>> - synchronized ( this )
>>> + final MemoryElementDescriptor<K, V> last = list.getLast();
>>> + if ( last != null )
>>> {
>>> - if ( list.getLast() != null )
>>> + toSpool = last.ce;
>>> + if ( toSpool != null )
>>> {
>>> - toSpool = list.getLast().ce;
>>> - if ( toSpool != null )
>>> - {
>>> - cache.spoolToDisk( list.getLast().ce );
>>> - if ( !map.containsKey( list.getLast().ce.getKey() ) )
>>> - {
>>> - log.error( "update: map does not contain key: "
>>> - + list.getLast().ce.getKey() );
>>> - verifyCache();
>>> - }
>>> - if ( map.remove( list.getLast().ce.getKey() ) == null )
>>> - {
>>> - log.warn( "update: remove failed for key: "
>>> - + list.getLast().ce.getKey() );
>>> - verifyCache();
>>> - }
>>> - }
>>> - else
>>> + cache.spoolToDisk( last.ce );
>>> + if ( map.remove( last.ce.getKey() ) == null )
>>> {
>>> - throw new Error( "update: last.ce is null!" );
>>> + log.warn( "update: remove failed for key: "
>>> + + last.ce.getKey() );
>>> + verifyCache();
>>> }
>>> - list.removeLast();
>>> }
>>> else
>>> {
>>> - verifyCache();
>>> - throw new Error( "update: last is null!" );
>>> + throw new Error( "update: last.ce is null!" );
>>> }
>>> + list.remove(last);
>>> + }
>>> + else
>>> + {
>>> + verifyCache();
>>> + throw new Error( "update: last is null!" );
>>> + }
>>>
>>> - // If this is out of the sync block it can detect a mismatch
>>> - // where there is none.
>>> - if ( map.size() != dumpCacheSize() )
>>> - {
>>> - log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
>>> - + dumpCacheSize() );
>>> - }
>>> + // If this is out of the sync block it can detect a mismatch
>>> + // where there is none.
>>> + if ( map.size() != dumpCacheSize() )
>>> + {
>>> + log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
>>> + + dumpCacheSize() );
>>> }
>>> return toSpool;
>>> }
>>> @@ -324,7 +317,7 @@ public abstract class AbstractDoubleLink
>>> * @throws IOException
>>> */
>>> @Override
>>> - public synchronized boolean remove( K key )
>>> + public boolean remove( K key )
>>> throws IOException
>>> {
>>> if ( log.isDebugEnabled() )
>>> @@ -338,39 +331,33 @@ public abstract class AbstractDoubleLink
>>> if ( key instanceof String && ( (String) key ).endsWith( CacheConstants.NAME_COMPONENT_DELIMITER ) )
>>> {
>>> // remove all keys of the same name hierarchy.
>>> - synchronized ( map )
>>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>> {
>>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>> - {
>>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>>> - K k = entry.getKey();
>>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>>> + K k = entry.getKey();
>>>
>>> - if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
>>> - {
>>> - list.remove( entry.getValue() );
>>> - itr.remove();
>>> - removed = true;
>>> - }
>>> + if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
>>> + {
>>> + list.remove( entry.getValue() );
>>> + itr.remove();
>>> + removed = true;
>>> }
>>> }
>>> }
>>> else if ( key instanceof GroupAttrName )
>>> {
>>> // remove all keys of the same name hierarchy.
>>> - synchronized ( map )
>>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>> {
>>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>>> - {
>>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>>> - K k = entry.getKey();
>>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>>> + K k = entry.getKey();
>>>
>>> - if ( k instanceof GroupAttrName &&
>>> - ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
>>> - {
>>> - list.remove( entry.getValue() );
>>> - itr.remove();
>>> - removed = true;
>>> - }
>>> + if ( k instanceof GroupAttrName &&
>>> + ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
>>> + {
>>> + list.remove( entry.getValue() );
>>> + itr.remove();
>>> + removed = true;
>>> }
>>> }
>>> }
>>> @@ -396,7 +383,7 @@ public abstract class AbstractDoubleLink
>>> * @throws IOException
>>> */
>>> @Override
>>> - public synchronized void removeAll()
>>> + public void removeAll()
>>> throws IOException
>>> {
>>> map.clear();
>>> @@ -410,7 +397,7 @@ public abstract class AbstractDoubleLink
>>> * @param ce The feature to be added to the First
>>> * @return MemoryElementDescriptor
>>> */
>>> - protected synchronized MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
>>> + protected MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
>>> {
>>> MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
>>> list.addFirst( me );
>>> @@ -424,7 +411,7 @@ public abstract class AbstractDoubleLink
>>> * @param ce The feature to be added to the First
>>> * @return MemoryElementDescriptor
>>> */
>>> - protected synchronized MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
>>> + protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
>>> {
>>> MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
>>> list.addLast( me );
>>>
>>> Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
>>> URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java?rev=1594073&r1=1594072&r2=1594073&view=diff
>>> ==============================================================================
>>> --- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java (original)
>>> +++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java Mon May 12 19:46:08 2014
>>> @@ -32,7 +32,7 @@ public class MemoryElementDescriptor<K,
>>> private static final long serialVersionUID = -1905161209035522460L;
>>>
>>> /** The CacheElement wrapped by this descriptor */
>>> - public ICacheElement<K, V> ce; // TODO privatise
>>> + public final ICacheElement<K, V> ce; // TODO privatise
>>>
>>> /**
>>> * Constructs a usable MemoryElementDescriptor.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs: auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Benedikt Ritter <be...@gmail.com>.
Send from my mobile device
> Am 13.05.2014 um 20:53 schrieb Phil Steitz <ph...@gmail.com>:
>
>> On 5/12/14, 12:46 PM, rmannibucau@apache.org wrote:
>> Author: rmannibucau
>> Date: Mon May 12 19:46:08 2014
>> New Revision: 1594073
>>
>> URL: http://svn.apache.org/r1594073
>> Log:
>> removing a bunch of synchronized thanks to ConcurrentHashMap - still removeAll to review since it can be not that good but using any synch would make it slower and not scalable
>>
>> Modified:
>> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.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/util/MemoryElementDescriptor.java
>>
>> Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
>> URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java?rev=1594073&r1=1594072&r2=1594073&view=diff
>> ==============================================================================
>> --- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java (original)
>> +++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java Mon May 12 19:46:08 2014
>> @@ -81,7 +81,7 @@ public class RemoteCacheServerFactory
>> * @return Returns the remoteCacheServer.
>> */
>> @SuppressWarnings("unchecked") // Need cast to specific RemoteCacheServer
>> - public static <K extends Serializable, V extends Serializable> RemoteCacheServer<K, V> getRemoteCacheServer()
>> + public static <K, V> RemoteCacheServer<K, V> getRemoteCacheServer()
>> {
>> return (RemoteCacheServer<K, V>)remoteCacheServer;
>> }
>>
>> 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=1594073&r1=1594072&r2=1594073&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 Mon May 12 19:46:08 2014
>> @@ -86,8 +86,11 @@ public abstract class AbstractDoubleLink
>>
>> /**
>> * This is called by super initialize.
>> + *
>> + * NOTE: should return a thread safe map
>> + *
>> * <p>
>> - * @return new Hashtable()
>> + * @return new ConcurrentHashMap()
>> */
>> @Override
>> public Map<K, MemoryElementDescriptor<K, V>> createMap()
>> @@ -109,19 +112,15 @@ public abstract class AbstractDoubleLink
>> {
>> putCnt++;
>>
>> - synchronized ( this )
>
> I am not really familiar with this code, so this could be needless
> concern; but removing this synch makes the adjustListForUpdate no
> longer atomic with the put below. Is this OK?
Sorry, I seem to have missed the answer to this question. Was it okay to remove the synchronized?
Bene
>
> Phil
>> - {
>> - // ABSTRACT
>> - MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
>> + MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
>>
>> - // this must be synchronized
>> - 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() ) )
>> - {
>> - list.remove( oldNode );
>> - }
>> + // 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 );
>> }
>>
>> // If we are over the max spool some
>> @@ -190,12 +189,14 @@ public abstract class AbstractDoubleLink
>> * @throws IOException
>> */
>> @Override
>> - public final synchronized ICacheElement<K, V> get( K key )
>> + public final ICacheElement<K, V> get( K key )
>> throws IOException
>> {
>> ICacheElement<K, V> ce = null;
>>
>> - if ( log.isDebugEnabled() )
>> + final boolean debugEnabled = log.isDebugEnabled();
>> +
>> + if (debugEnabled)
>> {
>> log.debug( "getting item from cache " + cacheName + " for key " + key );
>> }
>> @@ -206,7 +207,7 @@ public abstract class AbstractDoubleLink
>> {
>> ce = me.ce;
>> hitCnt++;
>> - if ( log.isDebugEnabled() )
>> + if (debugEnabled)
>> {
>> log.debug( cacheName + ": LRUMemoryCache hit for " + ce.getKey() );
>> }
>> @@ -217,7 +218,7 @@ public abstract class AbstractDoubleLink
>> else
>> {
>> missCnt++;
>> - if ( log.isDebugEnabled() )
>> + if (debugEnabled)
>> {
>> log.debug( cacheName + ": LRUMemoryCache miss for " + key );
>> }
>> @@ -270,46 +271,38 @@ public abstract class AbstractDoubleLink
>> throws Error
>> {
>> ICacheElement<K, V> toSpool = null;
>> - synchronized ( this )
>> + final MemoryElementDescriptor<K, V> last = list.getLast();
>> + if ( last != null )
>> {
>> - if ( list.getLast() != null )
>> + toSpool = last.ce;
>> + if ( toSpool != null )
>> {
>> - toSpool = list.getLast().ce;
>> - if ( toSpool != null )
>> - {
>> - cache.spoolToDisk( list.getLast().ce );
>> - if ( !map.containsKey( list.getLast().ce.getKey() ) )
>> - {
>> - log.error( "update: map does not contain key: "
>> - + list.getLast().ce.getKey() );
>> - verifyCache();
>> - }
>> - if ( map.remove( list.getLast().ce.getKey() ) == null )
>> - {
>> - log.warn( "update: remove failed for key: "
>> - + list.getLast().ce.getKey() );
>> - verifyCache();
>> - }
>> - }
>> - else
>> + cache.spoolToDisk( last.ce );
>> + if ( map.remove( last.ce.getKey() ) == null )
>> {
>> - throw new Error( "update: last.ce is null!" );
>> + log.warn( "update: remove failed for key: "
>> + + last.ce.getKey() );
>> + verifyCache();
>> }
>> - list.removeLast();
>> }
>> else
>> {
>> - verifyCache();
>> - throw new Error( "update: last is null!" );
>> + throw new Error( "update: last.ce is null!" );
>> }
>> + list.remove(last);
>> + }
>> + else
>> + {
>> + verifyCache();
>> + throw new Error( "update: last is null!" );
>> + }
>>
>> - // If this is out of the sync block it can detect a mismatch
>> - // where there is none.
>> - if ( map.size() != dumpCacheSize() )
>> - {
>> - log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
>> - + dumpCacheSize() );
>> - }
>> + // If this is out of the sync block it can detect a mismatch
>> + // where there is none.
>> + if ( map.size() != dumpCacheSize() )
>> + {
>> + log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
>> + + dumpCacheSize() );
>> }
>> return toSpool;
>> }
>> @@ -324,7 +317,7 @@ public abstract class AbstractDoubleLink
>> * @throws IOException
>> */
>> @Override
>> - public synchronized boolean remove( K key )
>> + public boolean remove( K key )
>> throws IOException
>> {
>> if ( log.isDebugEnabled() )
>> @@ -338,39 +331,33 @@ public abstract class AbstractDoubleLink
>> if ( key instanceof String && ( (String) key ).endsWith( CacheConstants.NAME_COMPONENT_DELIMITER ) )
>> {
>> // remove all keys of the same name hierarchy.
>> - synchronized ( map )
>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>> {
>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>> - {
>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>> - K k = entry.getKey();
>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>> + K k = entry.getKey();
>>
>> - if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
>> - {
>> - list.remove( entry.getValue() );
>> - itr.remove();
>> - removed = true;
>> - }
>> + if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
>> + {
>> + list.remove( entry.getValue() );
>> + itr.remove();
>> + removed = true;
>> }
>> }
>> }
>> else if ( key instanceof GroupAttrName )
>> {
>> // remove all keys of the same name hierarchy.
>> - synchronized ( map )
>> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>> {
>> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
>> - {
>> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>> - K k = entry.getKey();
>> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
>> + K k = entry.getKey();
>>
>> - if ( k instanceof GroupAttrName &&
>> - ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
>> - {
>> - list.remove( entry.getValue() );
>> - itr.remove();
>> - removed = true;
>> - }
>> + if ( k instanceof GroupAttrName &&
>> + ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
>> + {
>> + list.remove( entry.getValue() );
>> + itr.remove();
>> + removed = true;
>> }
>> }
>> }
>> @@ -396,7 +383,7 @@ public abstract class AbstractDoubleLink
>> * @throws IOException
>> */
>> @Override
>> - public synchronized void removeAll()
>> + public void removeAll()
>> throws IOException
>> {
>> map.clear();
>> @@ -410,7 +397,7 @@ public abstract class AbstractDoubleLink
>> * @param ce The feature to be added to the First
>> * @return MemoryElementDescriptor
>> */
>> - protected synchronized MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
>> + protected MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
>> {
>> MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
>> list.addFirst( me );
>> @@ -424,7 +411,7 @@ public abstract class AbstractDoubleLink
>> * @param ce The feature to be added to the First
>> * @return MemoryElementDescriptor
>> */
>> - protected synchronized MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
>> + protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
>> {
>> MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
>> list.addLast( me );
>>
>> Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
>> URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java?rev=1594073&r1=1594072&r2=1594073&view=diff
>> ==============================================================================
>> --- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java (original)
>> +++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java Mon May 12 19:46:08 2014
>> @@ -32,7 +32,7 @@ public class MemoryElementDescriptor<K,
>> private static final long serialVersionUID = -1905161209035522460L;
>>
>> /** The CacheElement wrapped by this descriptor */
>> - public ICacheElement<K, V> ce; // TODO privatise
>> + public final ICacheElement<K, V> ce; // TODO privatise
>>
>> /**
>> * Constructs a usable MemoryElementDescriptor.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1594073 - in /commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs:
auxiliary/remote/server/ engine/memory/ engine/memory/util/
Posted by Phil Steitz <ph...@gmail.com>.
On 5/12/14, 12:46 PM, rmannibucau@apache.org wrote:
> Author: rmannibucau
> Date: Mon May 12 19:46:08 2014
> New Revision: 1594073
>
> URL: http://svn.apache.org/r1594073
> Log:
> removing a bunch of synchronized thanks to ConcurrentHashMap - still removeAll to review since it can be not that good but using any synch would make it slower and not scalable
>
> Modified:
> commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.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/util/MemoryElementDescriptor.java
>
> Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java
> URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java?rev=1594073&r1=1594072&r2=1594073&view=diff
> ==============================================================================
> --- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java (original)
> +++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/auxiliary/remote/server/RemoteCacheServerFactory.java Mon May 12 19:46:08 2014
> @@ -81,7 +81,7 @@ public class RemoteCacheServerFactory
> * @return Returns the remoteCacheServer.
> */
> @SuppressWarnings("unchecked") // Need cast to specific RemoteCacheServer
> - public static <K extends Serializable, V extends Serializable> RemoteCacheServer<K, V> getRemoteCacheServer()
> + public static <K, V> RemoteCacheServer<K, V> getRemoteCacheServer()
> {
> return (RemoteCacheServer<K, V>)remoteCacheServer;
> }
>
> 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=1594073&r1=1594072&r2=1594073&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 Mon May 12 19:46:08 2014
> @@ -86,8 +86,11 @@ public abstract class AbstractDoubleLink
>
> /**
> * This is called by super initialize.
> + *
> + * NOTE: should return a thread safe map
> + *
> * <p>
> - * @return new Hashtable()
> + * @return new ConcurrentHashMap()
> */
> @Override
> public Map<K, MemoryElementDescriptor<K, V>> createMap()
> @@ -109,19 +112,15 @@ public abstract class AbstractDoubleLink
> {
> putCnt++;
>
> - synchronized ( this )
I am not really familiar with this code, so this could be needless
concern; but removing this synch makes the adjustListForUpdate no
longer atomic with the put below. Is this OK?
Phil
> - {
> - // ABSTRACT
> - MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
> + MemoryElementDescriptor<K, V> newNode = adjustListForUpdate( ce );
>
> - // this must be synchronized
> - 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() ) )
> - {
> - list.remove( oldNode );
> - }
> + // 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 );
> }
>
> // If we are over the max spool some
> @@ -190,12 +189,14 @@ public abstract class AbstractDoubleLink
> * @throws IOException
> */
> @Override
> - public final synchronized ICacheElement<K, V> get( K key )
> + public final ICacheElement<K, V> get( K key )
> throws IOException
> {
> ICacheElement<K, V> ce = null;
>
> - if ( log.isDebugEnabled() )
> + final boolean debugEnabled = log.isDebugEnabled();
> +
> + if (debugEnabled)
> {
> log.debug( "getting item from cache " + cacheName + " for key " + key );
> }
> @@ -206,7 +207,7 @@ public abstract class AbstractDoubleLink
> {
> ce = me.ce;
> hitCnt++;
> - if ( log.isDebugEnabled() )
> + if (debugEnabled)
> {
> log.debug( cacheName + ": LRUMemoryCache hit for " + ce.getKey() );
> }
> @@ -217,7 +218,7 @@ public abstract class AbstractDoubleLink
> else
> {
> missCnt++;
> - if ( log.isDebugEnabled() )
> + if (debugEnabled)
> {
> log.debug( cacheName + ": LRUMemoryCache miss for " + key );
> }
> @@ -270,46 +271,38 @@ public abstract class AbstractDoubleLink
> throws Error
> {
> ICacheElement<K, V> toSpool = null;
> - synchronized ( this )
> + final MemoryElementDescriptor<K, V> last = list.getLast();
> + if ( last != null )
> {
> - if ( list.getLast() != null )
> + toSpool = last.ce;
> + if ( toSpool != null )
> {
> - toSpool = list.getLast().ce;
> - if ( toSpool != null )
> - {
> - cache.spoolToDisk( list.getLast().ce );
> - if ( !map.containsKey( list.getLast().ce.getKey() ) )
> - {
> - log.error( "update: map does not contain key: "
> - + list.getLast().ce.getKey() );
> - verifyCache();
> - }
> - if ( map.remove( list.getLast().ce.getKey() ) == null )
> - {
> - log.warn( "update: remove failed for key: "
> - + list.getLast().ce.getKey() );
> - verifyCache();
> - }
> - }
> - else
> + cache.spoolToDisk( last.ce );
> + if ( map.remove( last.ce.getKey() ) == null )
> {
> - throw new Error( "update: last.ce is null!" );
> + log.warn( "update: remove failed for key: "
> + + last.ce.getKey() );
> + verifyCache();
> }
> - list.removeLast();
> }
> else
> {
> - verifyCache();
> - throw new Error( "update: last is null!" );
> + throw new Error( "update: last.ce is null!" );
> }
> + list.remove(last);
> + }
> + else
> + {
> + verifyCache();
> + throw new Error( "update: last is null!" );
> + }
>
> - // If this is out of the sync block it can detect a mismatch
> - // where there is none.
> - if ( map.size() != dumpCacheSize() )
> - {
> - log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
> - + dumpCacheSize() );
> - }
> + // If this is out of the sync block it can detect a mismatch
> + // where there is none.
> + if ( map.size() != dumpCacheSize() )
> + {
> + log.warn( "update: After spool, size mismatch: map.size() = " + map.size() + ", linked list size = "
> + + dumpCacheSize() );
> }
> return toSpool;
> }
> @@ -324,7 +317,7 @@ public abstract class AbstractDoubleLink
> * @throws IOException
> */
> @Override
> - public synchronized boolean remove( K key )
> + public boolean remove( K key )
> throws IOException
> {
> if ( log.isDebugEnabled() )
> @@ -338,39 +331,33 @@ public abstract class AbstractDoubleLink
> if ( key instanceof String && ( (String) key ).endsWith( CacheConstants.NAME_COMPONENT_DELIMITER ) )
> {
> // remove all keys of the same name hierarchy.
> - synchronized ( map )
> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> {
> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> - {
> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
> - K k = entry.getKey();
> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
> + K k = entry.getKey();
>
> - if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
> - {
> - list.remove( entry.getValue() );
> - itr.remove();
> - removed = true;
> - }
> + if ( k instanceof String && ( (String) k ).startsWith( key.toString() ) )
> + {
> + list.remove( entry.getValue() );
> + itr.remove();
> + removed = true;
> }
> }
> }
> else if ( key instanceof GroupAttrName )
> {
> // remove all keys of the same name hierarchy.
> - synchronized ( map )
> + for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> {
> - for (Iterator<Map.Entry<K, MemoryElementDescriptor<K, V>>> itr = map.entrySet().iterator(); itr.hasNext(); )
> - {
> - Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
> - K k = entry.getKey();
> + Map.Entry<K, MemoryElementDescriptor<K, V>> entry = itr.next();
> + K k = entry.getKey();
>
> - if ( k instanceof GroupAttrName &&
> - ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
> - {
> - list.remove( entry.getValue() );
> - itr.remove();
> - removed = true;
> - }
> + if ( k instanceof GroupAttrName &&
> + ((GroupAttrName<?>)k).groupId.equals(((GroupAttrName<?>)key).groupId))
> + {
> + list.remove( entry.getValue() );
> + itr.remove();
> + removed = true;
> }
> }
> }
> @@ -396,7 +383,7 @@ public abstract class AbstractDoubleLink
> * @throws IOException
> */
> @Override
> - public synchronized void removeAll()
> + public void removeAll()
> throws IOException
> {
> map.clear();
> @@ -410,7 +397,7 @@ public abstract class AbstractDoubleLink
> * @param ce The feature to be added to the First
> * @return MemoryElementDescriptor
> */
> - protected synchronized MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
> + protected MemoryElementDescriptor<K, V> addFirst( ICacheElement<K, V> ce )
> {
> MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
> list.addFirst( me );
> @@ -424,7 +411,7 @@ public abstract class AbstractDoubleLink
> * @param ce The feature to be added to the First
> * @return MemoryElementDescriptor
> */
> - protected synchronized MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
> + protected MemoryElementDescriptor<K, V> addLast( ICacheElement<K, V> ce )
> {
> MemoryElementDescriptor<K, V> me = new MemoryElementDescriptor<K, V>( ce );
> list.addLast( me );
>
> Modified: commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java
> URL: http://svn.apache.org/viewvc/commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java?rev=1594073&r1=1594072&r2=1594073&view=diff
> ==============================================================================
> --- commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java (original)
> +++ commons/proper/jcs/trunk/commons-jcs-core/src/main/java/org/apache/commons/jcs/engine/memory/util/MemoryElementDescriptor.java Mon May 12 19:46:08 2014
> @@ -32,7 +32,7 @@ public class MemoryElementDescriptor<K,
> private static final long serialVersionUID = -1905161209035522460L;
>
> /** The CacheElement wrapped by this descriptor */
> - public ICacheElement<K, V> ce; // TODO privatise
> + public final ICacheElement<K, V> ce; // TODO privatise
>
> /**
> * Constructs a usable MemoryElementDescriptor.
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org