Re: JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
On 08/22/2013 03:37 AM, Brian Burkhalter wrote:
With respect to this issue
http://bugs.sun.com/view_bug.do?bug_id=6470700
the code of concern from java.lang.Math is
701private static Random randomNumberGenerator;
702
703private static synchronized Random initRNG() {
704Random rnd = randomNumberGenerator;
705return (rnd == null) ? (randomNumberGenerator = new Random()) :
rnd;
706 }
731public static double random() {
732Random rnd = randomNumberGenerator;
733if (rnd == null) rnd = initRNG();
734return rnd.nextDouble();
735}
where the class variable randomNumberGenerator is not used anywhere else in
the class. The complaint is that this is an instance of the broken
double-checked locking pattern. While at first glance this might appear to be
the case, it does not seem so to me. It looks more like an attempt to avoid
calling a synchronized method if randomNumberGenerator has already been
initialized.
A more typical pattern would be
private static Random randomNumberGenerator;
private static synchronized Random getRNG() {
if (randomNumberGenerator == null) {
randomNumberGenerator = new Random();
}
return randomNumberGenerator;
}
public static double random() {
return getRNG().nextDouble();
}
Comments, please.
(Sorry, I'm late to the party!)
I'm surprised people are still haunted by DCL :) In the correct form, it
has generally negligible synchronization overheads associated with
volatile read, and in return you have no class metadata waste. As in:
private static volatile Random rng;
private static Random getRNG() {
if (rng == null) {
synchronized(Math.class) {
rng = new Random();
}
}
return rng;
}
public static double random() {
return getRNG().nextDouble();
}
Also, random() will probably be the scalability bottleneck anyway due to
j.l.Random internal atomics. That means, we do not have to provide the
best possible performance for getRNG(), but rather eliminate the
scalability bottleneck. Holder idiom has a benefit of being sequentially
faster (because JITs elide the class init checks, and read the static
field), but in this case of heavy-weight mechanics in j.u.Random, it is
irrelevant. Hence, we can minimize the downside for class metadata
growth by going with DCL.
I would think the *major* problem the patch should fix is the visibility
of new Random() state in the other threads, because its instance is
being published via the data race -- both correct DCL and Holder idiom
solve that. But then again, our current implementation for Random is
using AtomicLong, which has the inherent thread-safeness, making the
only erroneous scenario impossible.
Bottom-line: commit either correct DCL just to secure ourselves from
the adversarial j.u.Random changes.
-Aleksey.
JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
With respect to this issue
http://bugs.sun.com/view_bug.do?bug_id=6470700
the code of concern from java.lang.Math is
701private static Random randomNumberGenerator;
702
703private static synchronized Random initRNG() {
704Random rnd = randomNumberGenerator;
705return (rnd == null) ? (randomNumberGenerator = new Random()) : rnd;
706 }
731public static double random() {
732Random rnd = randomNumberGenerator;
733if (rnd == null) rnd = initRNG();
734return rnd.nextDouble();
735}
where the class variable randomNumberGenerator is not used anywhere else in
the class. The complaint is that this is an instance of the broken
double-checked locking pattern. While at first glance this might appear to be
the case, it does not seem so to me. It looks more like an attempt to avoid
calling a synchronized method if randomNumberGenerator has already been
initialized.
A more typical pattern would be
private static Random randomNumberGenerator;
private static synchronized Random getRNG() {
if (randomNumberGenerator == null) {
randomNumberGenerator = new Random();
}
return randomNumberGenerator;
}
public static double random() {
return getRNG().nextDouble();
}
Comments, please.
Thanks,
Brian
Re: JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
On 8/21/13 5:37 PM, Brian Burkhalter wrote:
With respect to this issue
http://bugs.sun.com/view_bug.do?bug_id=6470700
the code of concern from java.lang.Math is
701private static Random randomNumberGenerator;
702
703private static synchronized Random initRNG() {
704Random rnd = randomNumberGenerator;
705return (rnd == null) ? (randomNumberGenerator = new Random()) : rnd;
706 }
731public static double random() {
732Random rnd = randomNumberGenerator;
733if (rnd == null) rnd = initRNG();
734return rnd.nextDouble();
735}
where the class variable randomNumberGenerator is not used anywhere else in the class.
The complaint is that this is an instance of the broken double-checked locking pattern. While at
first glance this might appear to be the case, it does not seem so to me. It looks more like an
attempt to avoid calling a synchronized method if randomNumberGenerator has already
been initialized.
A more typical pattern would be
private static Random randomNumberGenerator;
private static synchronized Random getRNG() {
if (randomNumberGenerator == null) {
randomNumberGenerator = new Random();
}
return randomNumberGenerator;
}
public static double random() {
return getRNG().nextDouble();
}
Comments, please.
I don't think you'd want to introduce the overhead of synchronization
here. It may be better in this case to use this kind of lazy init pattern:
static final class Holder {
static final Random RNG = new Random();
}
public static double random() {
return Holder.RNG.nextDouble();
}
--
- DML
Re: JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
On Aug 21, 2013, at 5:01 PM, David M. Lloyd wrote:
I don't think you'd want to introduce the overhead of synchronization here.
No, I don't. My example code was just that.
It may be better in this case to use this kind of lazy init pattern:
static final class Holder {
static final Random RNG = new Random();
}
public static double random() {
return Holder.RNG.nextDouble();
}
Thanks for the cleaner suggestion.
Brian
Re: JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
On Aug 21, 2013, at 4:37 PM, Brian Burkhalter brian.burkhal...@oracle.com
wrote:
With respect to this issue
http://bugs.sun.com/view_bug.do?bug_id=6470700
the code of concern from java.lang.Math is
701private static Random randomNumberGenerator;
702
703private static synchronized Random initRNG() {
704Random rnd = randomNumberGenerator;
705return (rnd == null) ? (randomNumberGenerator = new Random()) :
rnd;
706 }
731public static double random() {
732Random rnd = randomNumberGenerator;
733if (rnd == null) rnd = initRNG();
734return rnd.nextDouble();
735}
where the class variable randomNumberGenerator is not used anywhere else in
the class. The complaint is that this is an instance of the broken
double-checked locking pattern. While at first glance this might appear to be
the case, it does not seem so to me. It looks more like an attempt to avoid
calling a synchronized method if randomNumberGenerator has already been
initialized.
This looks very much like the Broken multithreaded version from
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
// Broken multithreaded version
// Double-Checked Locking idiom
class Foo {
private Helper helper = null;
public Helper getHelper() {
if (helper == null)
synchronized(this) {
if (helper == null)
helper = new Helper();
}
return helper;
}
// other functions and members...
}
Unfortunately, that code just does not work in the presence of either
optimizing compilers or shared memory multiprocessors.
Re: JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
There's a significant difference here: Random reads the field into a local
and then operates only on the local. Looking at the code, I only see one
possible (bizarre) circumstance where you can hit NPE. If code was
transformed to:
static double random() {
Random rnd = randomNumberGenerator; // assume null is read
if (randomNumberGenerator == null) ... // note the field is re-read
instead of local, and assume it's not null now
return rnd.nextDouble(); // NPE
}
I don't see a case where any sane compiler would do this but I think it's
legal transformation given that the field is non-volatile and single
threaded execution would not detect difference. Unless phantom reads
cannot be introduced even for plain fields, in which case I don't see how
NPE can occur.
Having said that, David's suggestion is cleaner anyway.
Sent from my phone
On Aug 21, 2013 8:36 PM, Steven Schlansker stevenschlans...@gmail.com
wrote:
On Aug 21, 2013, at 4:37 PM, Brian Burkhalter brian.burkhal...@oracle.com
wrote:
With respect to this issue
http://bugs.sun.com/view_bug.do?bug_id=6470700
the code of concern from java.lang.Math is
701private static Random randomNumberGenerator;
702
703private static synchronized Random initRNG() {
704Random rnd = randomNumberGenerator;
705return (rnd == null) ? (randomNumberGenerator = new Random())
: rnd;
706 }
731public static double random() {
732Random rnd = randomNumberGenerator;
733if (rnd == null) rnd = initRNG();
734return rnd.nextDouble();
735}
where the class variable randomNumberGenerator is not used anywhere
else in the class. The complaint is that this is an instance of the broken
double-checked locking pattern. While at first glance this might appear to
be the case, it does not seem so to me. It looks more like an attempt to
avoid calling a synchronized method if randomNumberGenerator has already
been initialized.
This looks very much like the Broken multithreaded version from
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
// Broken multithreaded version
// Double-Checked Locking idiom
class Foo {
private Helper helper = null;
public Helper getHelper() {
if (helper == null)
synchronized(this) {
if (helper == null)
helper = new Helper();
}
return helper;
}
// other functions and members...
}
Unfortunately, that code just does not work in the presence of either
optimizing compilers or shared memory multiprocessors.
Re: JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses double checked locking
On Aug 21 2013, at 17:01 , David M. Lloyd wrote:
On 8/21/13 5:37 PM, Brian Burkhalter wrote:
With respect to this issue
http://bugs.sun.com/view_bug.do?bug_id=6470700
the code of concern from java.lang.Math is
701private static Random randomNumberGenerator;
702
703private static synchronized Random initRNG() {
704Random rnd = randomNumberGenerator;
705return (rnd == null) ? (randomNumberGenerator = new Random()) :
rnd;
706 }
731public static double random() {
732Random rnd = randomNumberGenerator;
733if (rnd == null) rnd = initRNG();
734return rnd.nextDouble();
735}
where the class variable randomNumberGenerator is not used anywhere else
in the class. The complaint is that this is an instance of the broken
double-checked locking pattern. While at first glance this might appear to
be the case, it does not seem so to me. It looks more like an attempt to
avoid calling a synchronized method if randomNumberGenerator has already
been initialized.
A more typical pattern would be
private static Random randomNumberGenerator;
private static synchronized Random getRNG() {
if (randomNumberGenerator == null) {
randomNumberGenerator = new Random();
}
return randomNumberGenerator;
}
public static double random() {
return getRNG().nextDouble();
}
Comments, please.
I don't think you'd want to introduce the overhead of synchronization here.
It may be better in this case to use this kind of lazy init pattern:
static final class Holder {
static final Random RNG = new Random();
}
public static double random() {
return Holder.RNG.nextDouble();
}
--
- DML
David's suggestion seems very reasonable.
Mike
