You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by David Medinets <da...@gmail.com> on 2012/09/12 23:47:21 UTC
Double Checked Locking - broken? working?
I see the following code in the Property class:
public static boolean isValidTablePropertyKey(String key) {
if (validTableProperties == null) {
synchronized (Property.class) {
if (validTableProperties == null) {
HashSet<String> tmp = new HashSet<String>();
for (Property p : Property.values())
if (!p.getType().equals(PropertyType.PREFIX) &&
p.getKey().startsWith(Property.TABLE_PREFIX.getKey()))
tmp.add(p.getKey());
validTableProperties = tmp;
}
}
}
return validTableProperties.contains(key) ||
key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
|| key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) ||
key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey());
}
However, http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
says that Double Check Locking does not work. Is there any reason to
believe that the article is incorrect? At the end of the article, it
recommends using volatile in Java 1.5 and above. Is there any reason
the code should not be changed to use the volatile technique?
RE: EXTERNAL: Double Checked Locking - broken? working?
Posted by "Cardon, Tejay E" <te...@lmco.com>.
If it's truly an instance of lazy instantiation, then I'd suggest using the enum holder pattern described by Josh Bloch in Effective Java. Otherwise, volatile works, but it looks uglier.
Tejay
-----Original Message-----
From: David Medinets [mailto:david.medinets@gmail.com]
Sent: Wednesday, September 12, 2012 3:47 PM
To: accumulo-dev
Subject: EXTERNAL: Double Checked Locking - broken? working?
I see the following code in the Property class:
public static boolean isValidTablePropertyKey(String key) {
if (validTableProperties == null) {
synchronized (Property.class) {
if (validTableProperties == null) {
HashSet<String> tmp = new HashSet<String>();
for (Property p : Property.values())
if (!p.getType().equals(PropertyType.PREFIX) &&
p.getKey().startsWith(Property.TABLE_PREFIX.getKey()))
tmp.add(p.getKey());
validTableProperties = tmp;
}
}
}
return validTableProperties.contains(key) ||
key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
|| key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) || key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey());
}
However, http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
says that Double Check Locking does not work. Is there any reason to believe that the article is incorrect? At the end of the article, it recommends using volatile in Java 1.5 and above. Is there any reason the code should not be changed to use the volatile technique?
Re: Double Checked Locking - broken? working?
Posted by Christopher Tubbs <ct...@gmail.com>.
According to that same page, it says that the JDK5 memory model (and
presumably JDK6 as well? which we use...), this will work if the field
(validTableProperties) is declared volatile. That's the simplest
change, but Mike is right... it's not worth lazily loading because
it's a trivial operation... and it is certainly idempotent.
This code was the result of me being pedantic without properly
understanding locking... it was *not* done based on any actual
observed performance problem (not even a predicted one). If anything,
I only did it it that way as an exercise in learning locking. It
should be simplified as a static initializer instead and none of it
needs to be synchronized.
On Wed, Sep 12, 2012 at 11:09 PM, Mike Drob <md...@mdrob.com> wrote:
> It's a private static field that never changes once we do the initial load.
> The load looks idempotent, so I'm not sure it's worth the hassle of trying
> to synchronize on it, or even do it lazily. Maybe somebody with more
> history has seen that it is way more performance intensive than I give it
> credit for, but I think we should just throw that part in a static block
> and be done with it.
>
> Alternatively, in the middle of that article, he suggests that if you have
> a static singleton (which this appears to be) that you can throw it in
> another class and the memory model will work out.
>
> -Mike
>
> On Thu, Sep 13, 2012 at 12:25 AM, Eric Newton <er...@gmail.com> wrote:
>
>> The article is correct. Just go ahead and simplify the code.
>>
>> -Eric
>>
>> On Wed, Sep 12, 2012 at 5:47 PM, David Medinets <david.medinets@gmail.com
>> >wrote:
>>
>> > I see the following code in the Property class:
>> >
>> > public static boolean isValidTablePropertyKey(String key) {
>> > if (validTableProperties == null) {
>> > synchronized (Property.class) {
>> > if (validTableProperties == null) {
>> > HashSet<String> tmp = new HashSet<String>();
>> > for (Property p : Property.values())
>> > if (!p.getType().equals(PropertyType.PREFIX) &&
>> > p.getKey().startsWith(Property.TABLE_PREFIX.getKey()))
>> > tmp.add(p.getKey());
>> > validTableProperties = tmp;
>> > }
>> > }
>> > }
>> >
>> > return validTableProperties.contains(key) ||
>> > key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
>> > || key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) ||
>> > key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey());
>> > }
>> >
>> > However,
>> > http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
>> > says that Double Check Locking does not work. Is there any reason to
>> > believe that the article is incorrect? At the end of the article, it
>> > recommends using volatile in Java 1.5 and above. Is there any reason
>> > the code should not be changed to use the volatile technique?
>> >
>>
Re: Double Checked Locking - broken? working?
Posted by Mike Drob <md...@mdrob.com>.
It's a private static field that never changes once we do the initial load.
The load looks idempotent, so I'm not sure it's worth the hassle of trying
to synchronize on it, or even do it lazily. Maybe somebody with more
history has seen that it is way more performance intensive than I give it
credit for, but I think we should just throw that part in a static block
and be done with it.
Alternatively, in the middle of that article, he suggests that if you have
a static singleton (which this appears to be) that you can throw it in
another class and the memory model will work out.
-Mike
On Thu, Sep 13, 2012 at 12:25 AM, Eric Newton <er...@gmail.com> wrote:
> The article is correct. Just go ahead and simplify the code.
>
> -Eric
>
> On Wed, Sep 12, 2012 at 5:47 PM, David Medinets <david.medinets@gmail.com
> >wrote:
>
> > I see the following code in the Property class:
> >
> > public static boolean isValidTablePropertyKey(String key) {
> > if (validTableProperties == null) {
> > synchronized (Property.class) {
> > if (validTableProperties == null) {
> > HashSet<String> tmp = new HashSet<String>();
> > for (Property p : Property.values())
> > if (!p.getType().equals(PropertyType.PREFIX) &&
> > p.getKey().startsWith(Property.TABLE_PREFIX.getKey()))
> > tmp.add(p.getKey());
> > validTableProperties = tmp;
> > }
> > }
> > }
> >
> > return validTableProperties.contains(key) ||
> > key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
> > || key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) ||
> > key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey());
> > }
> >
> > However,
> > http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
> > says that Double Check Locking does not work. Is there any reason to
> > believe that the article is incorrect? At the end of the article, it
> > recommends using volatile in Java 1.5 and above. Is there any reason
> > the code should not be changed to use the volatile technique?
> >
>
Re: Double Checked Locking - broken? working?
Posted by Eric Newton <er...@gmail.com>.
The article is correct. Just go ahead and simplify the code.
-Eric
On Wed, Sep 12, 2012 at 5:47 PM, David Medinets <da...@gmail.com>wrote:
> I see the following code in the Property class:
>
> public static boolean isValidTablePropertyKey(String key) {
> if (validTableProperties == null) {
> synchronized (Property.class) {
> if (validTableProperties == null) {
> HashSet<String> tmp = new HashSet<String>();
> for (Property p : Property.values())
> if (!p.getType().equals(PropertyType.PREFIX) &&
> p.getKey().startsWith(Property.TABLE_PREFIX.getKey()))
> tmp.add(p.getKey());
> validTableProperties = tmp;
> }
> }
> }
>
> return validTableProperties.contains(key) ||
> key.startsWith(Property.TABLE_CONSTRAINT_PREFIX.getKey())
> || key.startsWith(Property.TABLE_ITERATOR_PREFIX.getKey()) ||
> key.startsWith(Property.TABLE_LOCALITY_GROUP_PREFIX.getKey());
> }
>
> However,
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
> says that Double Check Locking does not work. Is there any reason to
> believe that the article is incorrect? At the end of the article, it
> recommends using volatile in Java 1.5 and above. Is there any reason
> the code should not be changed to use the volatile technique?
>