[concurrency-interest] ThreadLocalRandom.nextSecondarySeed() re-initializes TLR's seed

[Peter Levart]

Hi Doug,

On 06/19/2014 02:02 PM, Doug Lea wrote:
> On 06/19/2014 04:48 AM, Peter Levart wrote:
>> Or, even better, why not just using the next value from the "seeder" 
>> sequence
>> for the initial value of "secondary" seed and avoid interaction with 
>> TLR's main
>> seed/probe:
>
> Thanks! Or better, just use mix32:
>
>>
>> +            if ((r = (int)mix64(seeder.getAndAdd(SEEDER_INCREMENT))) 
>> == 0)
> =>
>      if ((r = mix32(seeder.getAndAdd(SEEDER_INCREMENT))) == 0)

That's right.

>
> I committed this to jsr166 cvs. As you noted, this only addresses
> an uncommon performance glitch. 

Not so performance as the "expected" behaviour. I'm assuming the aim of 
TLR.nextSecondarySeed() as a java.util.concurrent private thread-local 
source of random numbers is:
- enough quality for it's purpose
- fast
- does not "disturb" the sequence of the primary public TLR sequence.

I was concerned about the last point only.

> I don't have any further ideas
> since we discussed last year the tradeoffs between computing
> decent quality initial seeds versus class-loading.
> I still think we have the best practical compromise in place.

This pertains to the other thread (ThreadLocalRandom clinit troubles) 
started by Martin Buchholz, right? He's making a valid point. The 
"seeder" static field is still uninitialized during either 
NetworkInterface class initialization (as a result of 
NetworkInterface.getNetworkInterfaces() call) or during 
SecureRandom.getSeed() call. Either of which can execute user code in 
some configurations which might in turn use ThreadLocalRandom. If this 
happens, TLR.current() throws a NPE. I proposed a re-arrangement of 
class initialization that allows TLR to be fully functional even during 
it's initialization, albeit with a less randomized seed, and does not 
change the behaviour otherwise (since it triggers re-initialization at 
the end). See the proposed patch in the other thread.

Regards, Peter

>
> -Doug
>
>>           }
>>           UNSAFE.putInt(t, SECONDARY, r);
>>
>>
>> Regards, Peter
>>
>> On 06/19/2014 10:37 AM, Peter Levart wrote:
>>> Hi,
>>>
>>> I noticed an inconsistency in calling TLR.localInit() method. 
>>> Everywhere it's
>>> called conditionaly if thread-local "probe" is zero except in
>>> TLR.nextSecondarySeed() where it's called if "secondary" seed is 
>>> zero. This
>>> re-initializes the "probe" and "seed" even though they might have 
>>> already been
>>> initialized. It's not a big deal, because this happens at most once per
>>> thread, but it would be more consistent to call localInit() 
>>> conditionaly, I
>>> think:
>>>
>>>
>>> diff -r 5b45a5efe417
>>> src/share/classes/java/util/concurrent/ThreadLocalRandom.java
>>> --- a/src/share/classes/java/util/concurrent/ThreadLocalRandom.java 
>>> Tue May 20
>>> 10:11:23 2014 +0400
>>> +++ b/src/share/classes/java/util/concurrent/ThreadLocalRandom.java 
>>> Thu Jun 19
>>> 10:34:18 2014 +0200
>>> @@ -1034,7 +1034,8 @@
>>>              r ^= r << 5;
>>> }
>>>          else {
>>> - localInit();
>>> +            if (UNSAFE.getInt(t, PROBE) == 0)
>>> + localInit();
>>>              if ((r = (int)UNSAFE.getLong(t, SEED)) == 0)
>>>                  r = 1; // avoid zero
>>>          }
>>>
>>>
>>>
>>> Regards, Peter
>>>
>>
>> _______________________________________________
>> Concurrency-interest mailing list
>> Concurrency-interest at cs.oswego.edu
>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>
>