JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses "double checked locking"

[Aleksey Shipilev]

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.