Mailing-List: contact dev-help@cayenne.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cayenne.apache.org
Received-SPF: pass (athena.apache.org: local policy)
Message-Id: <B82BB20B-4470-42F6-8713-7AB94C968899@objectstyle.org>
From: Andrus Adamchik <andrus@objectstyle.org>
To: dev@cayenne.apache.org
In-Reply-To: <20080916142412.91DCF2388A08@eris.apache.org>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v926)
Subject: Re: svn commit: r695898 -
 /cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/exp/Expression.java
Date: Tue, 16 Sep 2008 17:30:29 +0300
References: <20080916142412.91DCF2388A08@eris.apache.org>

-1. Unmanaged static caches are bad, especially in an embeddable  
framework like Cayenne. This is (a) a potential memory leak and (b) a  
potential synchronization nightmare (Expression is not immutable and  
can be modified by the caller).

Andrus


On Sep 16, 2008, at 5:24 PM, mgentry@apache.org wrote:

> Author: mgentry
> Date: Tue Sep 16 07:24:11 2008
> New Revision: 695898
>
> URL: http://svn.apache.org/viewvc?rev=695898&view=rev
> Log:
> Updated Expression.fromString to cache the parsed expressions  
> instead of re-parsing them every time.
>
> Modified:
>    cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/ 
> java/org/apache/cayenne/exp/Expression.java
>
> Modified: cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/ 
> src/main/java/org/apache/cayenne/exp/Expression.java
> URL: http://svn.apache.org/viewvc/cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/exp/Expression.java?rev=695898&r1=695897&r2=695898&view=diff
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/ 
> java/org/apache/cayenne/exp/Expression.java (original)
> +++ cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/ 
> java/org/apache/cayenne/exp/Expression.java Tue Sep 16 07:24:11 2008
> @@ -26,6 +26,7 @@
> import java.io.StringWriter;
> import java.util.Collection;
> import java.util.Collections;
> +import java.util.HashMap;
> import java.util.LinkedList;
> import java.util.List;
> import java.util.Map;
> @@ -119,21 +120,36 @@
>
>     protected int type;
>
> +    private static Map<String, Expression> expressionCache =
> +        Collections.synchronizedMap(new HashMap<String,  
> Expression>(32));
> +
>     /**
>      * Parses string, converting it to Expression. If string does  
> not represent a
>      * semantically correct expression, an ExpressionException is  
> thrown.
>      *
>      * @since 1.1
>      */
> -    // TODO: cache expression strings, since this operation is  
> pretty slow
>     public static Expression fromString(String expressionString) {
>         if (expressionString == null) {
>             throw new NullPointerException("Null expression string.");
>         }
>
> -        Reader reader = new StringReader(expressionString);
> +        // Retrieve, if possible, the expression from the cache.
> +        Expression expression =  
> expressionCache.get(expressionString);
> +
> +        // If the expression was cached, return it immediately  
> without parsing.
> +        if (expression != null)
> +            return expression;
> +
> +        // Couldn't find expression, parse, cache, and return it.
>         try {
> -            return new ExpressionParser(reader).expression();
> +            Reader reader = new StringReader(expressionString);
> +
> +            expression = new ExpressionParser(reader).expression();
> +
> +            expressionCache.put(expressionString, expression);
> +
> +            return expression;
>         }
>         catch (ParseException ex) {
>             throw new ExpressionException(ex.getMessage(), ex);
>