Removing some unnecessary synchronization for cache objects

[Jacopo Cappellato-4]

Hi devs,

please review my commit below because I am working to apply a similar pattern to a lot of other classes in OFBiz. This effort is not a search-and-replace task but instead I am reviewing each individual usage of static UtilCache objects and refactoring (when necessary) the code to follow the pattern you see below.
Here are some points to notice (feedback is highly appreciated because I am planning to use a similar approach on several other classes):
1) if possible, make the static fields that hold the reference to UtilCache a private and final field; I know this is not required but in my opinion makes the code easier to read and more secure
2) when useful, use the new method UtilCache.putIfAbsentAndGet that always returns a value (the reference of the object passed to it or the reference of the object found in the cache); of course this is not mandatory because I could use putIfAbsent and then use the reference of the passed object (if the returnedvalue is null, which means that it was added to the cache) but I think that putIfAbsentAndGet often makes the code slightly more readable
3) removed the synchronized block in favour of two calls to the thread safe UtilCache object: "get" and "putIfAbsentAndGet" (or "putIfAbsent")

Kind regards,

Jacopo

Begin forwarded message:

> From: [hidden email]
> Subject: svn commit: r1343277 - /ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
> Date: May 28, 2012 5:02:39 PM GMT+02:00
> To: [hidden email]
> Reply-To: [hidden email]
>
> Author: jacopoc
> Date: Mon May 28 15:02:39 2012
> New Revision: 1343277
>
> URL: http://svn.apache.org/viewvc?rev=1343277&view=rev
> Log:
> Improved code that manages the cache:
> * removed unnecessary synchronization  
> * protected the UtilCache object (static field) by making it private and final
>
> Modified:
>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>
> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java?rev=1343277&r1=1343276&r2=1343277&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java (original)
> +++ ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java Mon May 28 15:02:39 2012
> @@ -38,7 +38,7 @@ public class RegionManager {
>
>     public static final String module = RegionManager.class.getName();
>
> -    protected static UtilCache<URL, Map<String, Region>> regionCache = UtilCache.createUtilCache("webapp.Regions.Config", 0, 0);
> +    private static final UtilCache<URL, Map<String, Region>> regionCache = UtilCache.createUtilCache("webapp.Regions.Config", 0, 0);
>
>     protected URL regionFile = null;
>
> @@ -54,14 +54,8 @@ public class RegionManager {
>     public Map<String, Region> getRegions() {
>         Map<String, Region> regions = regionCache.get(regionFile);
>         if (regions == null) {
> -            synchronized (this) {
> -                regions = regionCache.get(regionFile);
> -                if (regions == null) {
> -                    if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded for " + regionFile + ", loading now", module);
> -                    regions = readRegionXml(regionFile);
> -                    regionCache.put(regionFile, regions);
> -                }
> -            }
> +            if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded for " + regionFile + ", loading now", module);
> +            regions = regionCache.putIfAbsentAndGet(regionFile, readRegionXml(regionFile));
>         }
>         return regions;
>     }
>
>

[Jacopo Cappellato-4]

Please also review the following commit because it is another pattern I am going to apply to other code:


On May 28, 2012, at 5:25 PM, [hidden email] wrote:

> Author: jacopoc
> Date: Mon May 28 15:25:46 2012
> New Revision: 1343278
>
> URL: http://svn.apache.org/viewvc?rev=1343278&view=rev
> Log:
> Improved code that manages the cache:
> * protected the UtilCache object (static field) by making it private and final
> * instead of using the "put" method, use the "putIfAbsentAndGet" method to prevent the risk of adding the new value if another concurrent thread added it in the meantime
>
>
> Modified:
>    ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java
>
> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java?rev=1343278&r1=1343277&r2=1343278&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java (original)
> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java Mon May 28 15:25:46 2012
> @@ -56,7 +56,7 @@ import javax.xml.transform.stream.Stream
> public final class XslTransform {
>
>     public static final String module = XslTransform.class.getName();
> -    public static UtilCache<String, Templates> xslTemplatesCache = UtilCache.createUtilCache("XsltTemplates", 0, 0);
> +    private static final UtilCache<String, Templates> xslTemplatesCache = UtilCache.createUtilCache("XsltTemplates", 0, 0);
>
>     /**
>      * @param template the content or url of the xsl template
> @@ -116,7 +116,7 @@ public final class XslTransform {
>             Source templateSource = getSource(templateDocument, templateUrl, templateString);
>             translet = tFactory.newTemplates(templateSource);
>             if (UtilValidate.isNotEmpty(templateName)) {
> -                    xslTemplatesCache.put(templateName, translet);
> +                translet = xslTemplatesCache.putIfAbsentAndGet(templateName, translet);
>             }
>         }
>         if (translet != null) {
>
>

Jacopo

[Jacopo Cappellato-4]

Please also review this one because I have other similar upcoming.

Thanks,

Jacopo

On May 28, 2012, at 5:32 PM, [hidden email] wrote:

> Author: jacopoc
> Date: Mon May 28 15:32:28 2012
> New Revision: 1343287
>
> URL: http://svn.apache.org/viewvc?rev=1343287&view=rev
> Log:
> Improved code that manages the cache:
> * replaced putIfAbsent + get with putIfAbsentAndGet to avoid the risk of a NPE if the cache entry is removed (or expired) between the two calls
>
>
> Modified:
>    ofbiz/trunk/framework/entity/src/org/ofbiz/entity/model/ModelFieldTypeReader.java
>
> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/model/ModelFieldTypeReader.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/model/ModelFieldTypeReader.java?rev=1343287&r1=1343286&r2=1343287&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/model/ModelFieldTypeReader.java (original)
> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/model/ModelFieldTypeReader.java Mon May 28 15:32:28 2012
> @@ -89,10 +89,8 @@ public class ModelFieldTypeReader implem
>                 throw new IllegalStateException("Error loading field type file " + fieldTypeResourceHandler.getLocation());
>             }
>             Map<String, ModelFieldType> fieldTypeMap = createFieldTypeCache(document.getDocumentElement(), fieldTypeResourceHandler.getLocation());
> -            reader = new ModelFieldTypeReader(fieldTypeMap);
> -            readers.putIfAbsent(tempModelName, reader);
> +            reader = readers.putIfAbsentAndGet(tempModelName, new ModelFieldTypeReader(fieldTypeMap));
>             utilTimer.timerString("[ModelFieldTypeReader.getModelFieldTypeReader] Read " + fieldTypeMap.size() + " field types");
> -            reader = readers.get(tempModelName);
>         }
>         return reader;
>     }
>

[David E. Jones-2]

Looks like a good pattern.

-David


On May 28, 2012, at 9:27 AM, Jacopo Cappellato <[hidden email]> wrote:

> Please also review the following commit because it is another pattern I am going to apply to other code:
>
>
> On May 28, 2012, at 5:25 PM, [hidden email] wrote:
>
>> Author: jacopoc
>> Date: Mon May 28 15:25:46 2012
>> New Revision: 1343278
>>
>> URL: http://svn.apache.org/viewvc?rev=1343278&view=rev
>> Log:
>> Improved code that manages the cache:
>> * protected the UtilCache object (static field) by making it private and final
>> * instead of using the "put" method, use the "putIfAbsentAndGet" method to prevent the risk of adding the new value if another concurrent thread added it in the meantime
>>
>>
>> Modified:
>>   ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java
>>
>> Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java?rev=1343278&r1=1343277&r2=1343278&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java (original)
>> +++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/template/XslTransform.java Mon May 28 15:25:46 2012
>> @@ -56,7 +56,7 @@ import javax.xml.transform.stream.Stream
>> public final class XslTransform {
>>
>>    public static final String module = XslTransform.class.getName();
>> -    public static UtilCache<String, Templates> xslTemplatesCache = UtilCache.createUtilCache("XsltTemplates", 0, 0);
>> +    private static final UtilCache<String, Templates> xslTemplatesCache = UtilCache.createUtilCache("XsltTemplates", 0, 0);
>>
>>    /**
>>     * @param template the content or url of the xsl template
>> @@ -116,7 +116,7 @@ public final class XslTransform {
>>            Source templateSource = getSource(templateDocument, templateUrl, templateString);
>>            translet = tFactory.newTemplates(templateSource);
>>            if (UtilValidate.isNotEmpty(templateName)) {
>> -                    xslTemplatesCache.put(templateName, translet);
>> +                translet = xslTemplatesCache.putIfAbsentAndGet(templateName, translet);
>>            }
>>        }
>>        if (translet != null) {
>>
>>
>
> Jacopo
>

[Jacques Le Roux]

I'm currently reading "Java Concurrency in Practice" and I agree on these change. I saw you continued at http://svn.apache.org/viewvc?view=revision&revision=1343696, is that all?

Jacques

Jacopo Cappellato wrote:

> Hi devs,
>
> please review my commit below because I am working to apply a similar pattern to a lot of other classes in OFBiz. This effort is
> not a search-and-replace task but instead I am reviewing each individual usage of static UtilCache objects and refactoring (when
> necessary) the code to follow the pattern you see below.  
> Here are some points to notice (feedback is highly appreciated because I am planning to use a similar approach on several other
> classes): 1) if possible, make the static fields that hold the reference to UtilCache a private and final field; I know this is
> not required but in my opinion makes the code easier to read and more secure 2) when useful, use the new method
> UtilCache.putIfAbsentAndGet that always returns a value (the reference of the object passed to it or the reference of the object
> found in the cache); of course this is not mandatory because I could use putIfAbsent and then use the reference of the passed
> object (if the returnedvalue is null, which means that it was added to the cache) but I think that putIfAbsentAndGet often makes
> the code slightly more readable 3) removed the synchronized block in favour of two calls to the thread safe UtilCache object:
> "get" and "putIfAbsentAndGet" (or "putIfAbsent")    
>
> Kind regards,
>
> Jacopo
>
> Begin forwarded message:
>
>> From: [hidden email]
>> Subject: svn commit: r1343277 - /ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>> Date: May 28, 2012 5:02:39 PM GMT+02:00
>> To: [hidden email]
>> Reply-To: [hidden email]
>>
>> Author: jacopoc
>> Date: Mon May 28 15:02:39 2012
>> New Revision: 1343277
>>
>> URL: http://svn.apache.org/viewvc?rev=1343277&view=rev
>> Log:
>> Improved code that manages the cache:
>> * removed unnecessary synchronization
>> * protected the UtilCache object (static field) by making it private and final
>>
>> Modified:
>>    ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>>
>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java?rev=1343277&r1=1343276&r2=1343277&view=diff
>> ============================================================================== ---
>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java (original) +++
>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java Mon May 28 15:02:39 2012 @@ -38,7 +38,7 @@ public
>> class RegionManager {
>>
>>     public static final String module = RegionManager.class.getName();
>>
>> -    protected static UtilCache<URL, Map<String, Region>> regionCache = UtilCache.createUtilCache("webapp.Regions.Config", 0, 0);
>> +    private static final UtilCache<URL, Map<String, Region>> regionCache = UtilCache.createUtilCache("webapp.Regions.Config",
>> 0, 0);
>>
>>     protected URL regionFile = null;
>>
>> @@ -54,14 +54,8 @@ public class RegionManager {
>>     public Map<String, Region> getRegions() {
>>         Map<String, Region> regions = regionCache.get(regionFile);
>>         if (regions == null) {
>> -            synchronized (this) {
>> -                regions = regionCache.get(regionFile);
>> -                if (regions == null) {
>> -                    if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded for " + regionFile + ", loading now", module);
>> -                    regions = readRegionXml(regionFile);
>> -                    regionCache.put(regionFile, regions);
>> -                }
>> -            }
>> +            if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded for " + regionFile + ", loading now", module);
>> +            regions = regionCache.putIfAbsentAndGet(regionFile, readRegionXml(regionFile));
>>         }
>>         return regions;
>>     }

[Jacopo Cappellato-4]

Hi Jacques,

I did a first pass and completed most of them; but I know that there are still several areas with similar patterns and these could be enhanced as well with slightly different approaches.

Kind regards,

Jacopo

On Nov 10, 2012, at 10:30 AM, Jacques Le Roux wrote:

> I'm currently reading "Java Concurrency in Practice" and I agree on these change. I saw you continued at http://svn.apache.org/viewvc?view=revision&revision=1343696, is that all?
>
> Jacques
>
> Jacopo Cappellato wrote:
>> Hi devs,
>>
>> please review my commit below because I am working to apply a similar pattern to a lot of other classes in OFBiz. This effort is
>> not a search-and-replace task but instead I am reviewing each individual usage of static UtilCache objects and refactoring (when
>> necessary) the code to follow the pattern you see below.  
>> Here are some points to notice (feedback is highly appreciated because I am planning to use a similar approach on several other
>> classes): 1) if possible, make the static fields that hold the reference to UtilCache a private and final field; I know this is
>> not required but in my opinion makes the code easier to read and more secure 2) when useful, use the new method
>> UtilCache.putIfAbsentAndGet that always returns a value (the reference of the object passed to it or the reference of the object
>> found in the cache); of course this is not mandatory because I could use putIfAbsent and then use the reference of the passed
>> object (if the returnedvalue is null, which means that it was added to the cache) but I think that putIfAbsentAndGet often makes
>> the code slightly more readable 3) removed the synchronized block in favour of two calls to the thread safe UtilCache object:
>> "get" and "putIfAbsentAndGet" (or "putIfAbsent")    
>>
>> Kind regards,
>>
>> Jacopo
>>
>> Begin forwarded message:
>>
>>> From: [hidden email]
>>> Subject: svn commit: r1343277 - /ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>>> Date: May 28, 2012 5:02:39 PM GMT+02:00
>>> To: [hidden email]
>>> Reply-To: [hidden email]
>>>
>>> Author: jacopoc
>>> Date: Mon May 28 15:02:39 2012
>>> New Revision: 1343277
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1343277&view=rev
>>> Log:
>>> Improved code that manages the cache:
>>> * removed unnecessary synchronization
>>> * protected the UtilCache object (static field) by making it private and final
>>>
>>> Modified:
>>>   ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>>>
>>> Modified: ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java
>>> URL:
>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java?rev=1343277&r1=1343276&r2=1343277&view=diff
>>> ============================================================================== ---
>>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java (original) +++
>>> ofbiz/trunk/framework/webapp/src/org/ofbiz/webapp/region/RegionManager.java Mon May 28 15:02:39 2012 @@ -38,7 +38,7 @@ public
>>> class RegionManager {
>>>
>>>    public static final String module = RegionManager.class.getName();
>>>
>>> -    protected static UtilCache<URL, Map<String, Region>> regionCache = UtilCache.createUtilCache("webapp.Regions.Config", 0, 0);
>>> +    private static final UtilCache<URL, Map<String, Region>> regionCache = UtilCache.createUtilCache("webapp.Regions.Config",
>>> 0, 0);
>>>
>>>    protected URL regionFile = null;
>>>
>>> @@ -54,14 +54,8 @@ public class RegionManager {
>>>    public Map<String, Region> getRegions() {
>>>        Map<String, Region> regions = regionCache.get(regionFile);
>>>        if (regions == null) {
>>> -            synchronized (this) {
>>> -                regions = regionCache.get(regionFile);
>>> -                if (regions == null) {
>>> -                    if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded for " + regionFile + ", loading now", module);
>>> -                    regions = readRegionXml(regionFile);
>>> -                    regionCache.put(regionFile, regions);
>>> -                }
>>> -            }
>>> +            if (Debug.verboseOn()) Debug.logVerbose("Regions not loaded for " + regionFile + ", loading now", module);
>>> +            regions = regionCache.putIfAbsentAndGet(regionFile, readRegionXml(regionFile));
>>>        }
>>>        return regions;
>>>    }