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: 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; >> }
