Re: svn commit: r959875 - /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ModelWidgetAction.java

[Adam Heath-2]

Jacques Le Roux wrote:
> Very good article indeed. What do we learn more in the book about DCL?
> If there is nothing more in the book, I suppose we prefer to use the
> static variable solution in OFBiz?

That is only good enough for certain situations.  Not for map-based
lookups.

The article could be better in explaining *why* the static variable is
better/different.  The book does.


> From: "Adrian Crum" <[hidden email]>
>> --- On Fri, 7/2/10, Scott Gray <[hidden email]> wrote:
>>> The book would have done well to use a more complicated
>>> example of why DCL shouldn't be used though. This
>>> article seems to do a better better job of it:
>>> http://www.ibm.com/developerworks/java/library/j-dcl.html
>>
>> That was the article I found a while back - before I got the book.
>

[Adrian Crum-2]

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.

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

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

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."

-Adrian

--- On Sat, 7/3/10, Jacques Le Roux <[hidden email]> wrote:

> From: Jacques Le Roux <[hidden email]>
> Subject: Re: svn commit: r959875 - /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ModelWidgetAction.java
> To: [hidden email]
> Date: Saturday, July 3, 2010, 4:41 AM
> Very good article indeed. What do we
> learn more in the book about DCL?
> If there is nothing more in the book, I suppose we prefer
> to use the static variable solution in OFBiz?
>
> Thanks
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
> > --- On Fri, 7/2/10, Scott Gray <[hidden email]>
> wrote:
> >> The book would have done well to use a more
> complicated
> >> example of why DCL shouldn't be used though. This
> >> article seems to do a better better job of it: http://www.ibm.com/developerworks/java/library/j-dcl.html
> >
> > That was the article I found a while back - before I
> got the book.
>
>

     

[Jacques Le Roux]

From: "Adam Heath" <[hidden email]>
> Jacques Le Roux wrote:
>> Very good article indeed. What do we learn more in the book about DCL?
>> If there is nothing more in the book, I suppose we prefer to use the
>> static variable solution in OFBiz?
>
> That is only good enough for certain situations.  Not for map-based
> lookups.

Ha ok!
 
> The article could be better in explaining *why* the static variable is
> better/different.  The book does.

Yes, I think I will buy the book, it's time to refresh my knowledge...

Jacques
 

>
>> From: "Adrian Crum" <[hidden email]>
>>> --- On Fri, 7/2/10, Scott Gray <[hidden email]> wrote:
>>>> The book would have done well to use a more complicated
>>>> example of why DCL shouldn't be used though. This
>>>> article seems to do a better better job of it:
>>>> http://www.ibm.com/developerworks/java/library/j-dcl.html
>>>
>>> That was the article I found a while back - before I got the book.
>>
>

[Jacques Le Roux]

Thanks Adrian,

Are they examples from the books or your own?

Jacques

From: "Adrian Crum" <[hidden email]>

> 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.
>
> 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;
>  }
> }
>
> 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;
> }
>
> 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."
>
> -Adrian
>
> --- On Sat, 7/3/10, Jacques Le Roux <[hidden email]> wrote:
>
>> From: Jacques Le Roux <[hidden email]>
>> Subject: Re: svn commit: r959875 - /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ModelWidgetAction.java
>> To: [hidden email]
>> Date: Saturday, July 3, 2010, 4:41 AM
>> Very good article indeed. What do we
>> learn more in the book about DCL?
>> If there is nothing more in the book, I suppose we prefer
>> to use the static variable solution in OFBiz?
>>
>> Thanks
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>> > --- On Fri, 7/2/10, Scott Gray <[hidden email]>
>> wrote:
>> >> The book would have done well to use a more
>> complicated
>> >> example of why DCL shouldn't be used though. This
>> >> article seems to do a better better job of it: http://www.ibm.com/developerworks/java/library/j-dcl.html
>> >
>> > That was the article I found a while back - before I
>> got the book.
>>
>>
>
>
>
>

[Adrian Crum-2]

The static field example is from the book.

-Adrian

--- On Sat, 7/3/10, Jacques Le Roux <[hidden email]> wrote:

> Thanks Adrian,
>
> Are they examples from the books or your own?
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
> > 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.
> >
> > 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;
> >  }
> > }
> >
> > 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;
> > }
> >
> > 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."
> >
> > -Adrian
> >
> > --- On Sat, 7/3/10, Jacques Le Roux <[hidden email]>
> wrote:
> >
> >> From: Jacques Le Roux <[hidden email]>
> >> Subject: Re: svn commit: r959875 -
> /ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ModelWidgetAction.java
> >> To: [hidden email]
> >> Date: Saturday, July 3, 2010, 4:41 AM
> >> Very good article indeed. What do we
> >> learn more in the book about DCL?
> >> If there is nothing more in the book, I suppose we
> prefer
> >> to use the static variable solution in OFBiz?
> >>
> >> Thanks
> >>
> >> Jacques
> >>
> >> From: "Adrian Crum" <[hidden email]>
> >> > --- On Fri, 7/2/10, Scott Gray <[hidden email]>
> >> wrote:
> >> >> The book would have done well to use a
> more
> >> complicated
> >> >> example of why DCL shouldn't be used
> though. This
> >> >> article seems to do a better better job
> of it: http://www.ibm.com/developerworks/java/library/j-dcl.html
> >> >
> >> > That was the article I found a while back -
> before I
> >> got the book.
> >>
> >>
> >
> >
> >
> >
>
>
>

[Adam Heath-2]

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).

>
> 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.

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.

[Adrian Crum-2]

--- On Sat, 7/3/10, Adam Heath <[hidden email]> 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.
>
>
>
>