commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Romain Manni-Bucau <rmannibu...@gmail.com>
Subject 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/
Date Wed, 28 May 2014 11:47:00 GMT
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 <phil.steitz@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 <phil.steitz@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
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message