Code Review 7051516: ThreadLocalRandom seed is never initialized so all instances generate the same sequence (original) (raw)

David Holmes David.Holmes at oracle.com
Fri Jun 17 01:40:20 UTC 2011

Chris Hegarty said the following on 06/17/11 02:37:

On 06/16/11 05:32 PM, Mike Duigou wrote:

Hi Chris;

The getClass() seems unnecessary. Why not : public Random(long seed) { this.seed = new AtomicLong(); setSeed(seed); } or private final AtomicLong seed = new AtomicLong(); public Random(long seed) { setSeed(seed); } Both of these would seem to have the same effect without needing to do the explicit class check. The change ( originally from Martin ) tries to save a couple of volatile writes in the common case. Yes, it's not a pretty as it could be.

But is the getClass + check + branch cheaper than those volatile writes?

I know the history is somewhat convoluted here and the real problem was caused by trying to define a subclass (ThreadLocalRandom) of Random that stripped away Random's thread-safety mechanisms. You not only end up with redundant state (two seeds!) but it breaks type substitutability. :(

But the milk has been spilt here and right now we just need to have TLR properly initialize its local seed. So its a thumbs up from me.

David

More information about the core-libs-dev mailing list