--- On Sat, 7/3/10, Adam Heath <doo...@brainfood.com> wrote:
> Adrian Crum wrote:
> > As the article and the book (Java Concurrency In
> Practice) both point out - the motivation for using DCL (to
> overcome slow speed) is less of an issue (or a non-issue)
> with newer JVMs.
> >
> > I am not a Java expert. But enough Java experts have
> stressed the need to eliminate this anti-pattern that I am
> willing to take their word for it. I said that in
> preparation for repeating what Adam said earlier - there are
> enough potential problems with DCL that it should not be
> used. I agree that there might be some DCL code in OFBiz
> that hasn't caused any problems so far, and it might be
> tempting to think "if it isn't broke, don't fix it" - but
> personally, I prefer to remove the potential problem.
> >
> > I have seen DCL used in two circumstances in OFBiz: to
> lazy load a static field (singleton), and to get elements
> from a Map.
>
> I'd like to to state it now. Do not start removing
> these things. I
> am already working towards that goal. But to do it
> properly, requires
> sufficient multi-threaded testing, and that is going to
> take me a bit.
>
> I currently have 15 separate commits that remove
> synchronization(some
> of those are DCL).
Understood. I don't think anyone is suggesting that we go around with a DCL
hatchet and start chopping out code. From my perspective, we're just discussing
why it shouldn't be used.
>
> >
> > 1. Lazy load a static field
> > ---------------------------
> >
> > // This is bad, don't do this
> > public class MyClass {
> > private static Resource resource;
> >
> > public static Resource getInstance()
> {
> > if (resource == null {
> > synchronized
> (MyClass.class) {
> > if (resource ==
> null) {
> > resource
> = new Resource();
> > }
> > }
> > }
> > return resource;
> > }
> > }
> >
> > The easiest fix in this example is to make
> getInstance() synchronized:
> >
> > public synchronized static Resource getInstance() {
> > if (resource == null {
> > resource = new Resource();
> > }
> > return resource;
> > }
> >
> > The lazy load has been preserved, but at a cost of
> synchronizing every call to getInstance().
> >
> > It would be better to eliminate the lazy load
> altogether:
> >
> > public class MyClass {
> > private static Resource resource =
> new Resource();
> >
> > public static Resource getInstance()
> {
> > return resource;
> > }
>
> You don't need need the accessor here, if you make resource
> final.
>
> > }
>
> This is not the best approach. Now the item is always
> loaded whenever
> MyClass is referenced.
Keep in mind my reply was to Jacques - who wanted to know what the book had to
say on the subject. So, I gave him two examples of safe initialization taken
from the book (section 16.2.3).
>
> class MyClass {
> public static final Resource alwaysNeeded = new
> Resource();
> private static volatile
> AtomicReference<Resource> sometimesNeeded =
> new AtomicReference<Resource>();
> private static volatile
> AtomicReference<Resource> seldomNeeded = new
> AtomicReference<Resource>();
>
> public static Resource getSometimesNeeded() {
> Resource resource = sometimesNeeded.get();
> if (resource != null) return resource;
> resource = new Resource();
> sometimesNeeded.compareAndSet(null,
> resource);
> // second get needed incase 2 threads got
> past the null check
> return sometimesNeeded.get();
> }
> }
>
>
>
> > 2. Get elements from a Map
> > --------------------------
> >
> > // This is bad, don't do this
> > public class MyClass {
> > private static Map<String,
> Resource> resourceMap = FastMap.newInstance();
> >
> > public static Resource
> getResource(String resourceName) {
> > Resource resource =
> resourceMap.get(resourceName);
> > if (resource == null {
> > synchronized
> (MyClass.class) {
> > resource =
> resourceMap.get(resourceName);
> > if (resource ==
> null) {
> > resource
> = new Resource();
> >
> resourceMap.put(resourceName, resource);
> > }
> > }
> > }
> > return resource;
> > }
> > }
> >
> > Again, the easiest fix is to make the getResource
> method synchronized:
> >
> > public synchronized static Resource getResource(String
> resourceName) {
> > Resource resource =
> resourceMap.get(resourceName);
> > if (resource == null {
> > resource = new Resource();
> > resourceMap.put(resourceName,
> resource);
> > }
> > return resource;
> > }
> >
> > This fix comes at a cost of synchronizing every call
> to getResource. If the cost of creating a new instance of
> Resource is low, we can skip synchronization altogether:
> >
> > public static Resource getResource(String
> resourceName) {
> > Resource resource =
> resourceMap.get(resourceName);
> > if (resource == null {
> > resource = new Resource();
> > resourceMap.put(resourceName,
> resource);
> > resource =
> resourceMap.get(resourceName);
> > }
> > return resource;
> > }
>
> This is buggy. Multiple threads could try for the
> same resource,
> create multiple entries, and both call put. The map
> can then become
> corrupted. I've seen it happen. Do not do
> this.
>
> >
> > Using this method, there is a chance that more than
> one thread will create a Resource instance, but we are not
> concerned about that - we just do a second
> resourceMap.get(resourceName) method call to make sure we
> have the Resource instance that "won."
>
> ConcurrentMap map = new ConcurrentHashMap();
>
> map.putIfAbsent(resourceName, resource);
> resource = resourceMap.get(resourceName);
>
> These lines will fix the problem I mentioned.
>
>
>
>
