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
>
> 701 private static Random randomNumberGenerator;
> 702
> 703 private static synchronized Random initRNG() {
> 704 Random rnd = randomNumberGenerator;
> 705 return (rnd == null) ? (randomNumberGenerator = new Random()) :
> rnd;
> 706 }
>
> 731 public static double random() {
> 732 Random rnd = randomNumberGenerator;
> 733 if (rnd == null) rnd = initRNG();
> 734 return 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.
