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: jaco...@apache.org > 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: comm...@ofbiz.apache.org > Reply-To: dev@ofbiz.apache.org > > 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; > } > >
