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

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