ThreadLocalRandom.nextSecondarySeed() re-initializes TLR's seed
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
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: 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:45:25 2014 +0200 @@ -1034,8 +1034,7 @@ r ^= r << 5; } else { - localInit(); - if ((r = (int)UNSAFE.getLong(t, SEED)) == 0) + if ((r = (int)mix64(seeder.getAndAdd(SEEDER_INCREMENT))) == 0) r = 1; // avoid zero } 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
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) I committed this to jsr166 cvs. As you noted, this only addresses an uncommon performance glitch. 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. -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@cs.oswego.edu http://cs.oswego.edu/mailman/listinfo/concurrency-interest
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@cs.oswego.edu http://cs.oswego.edu/mailman/listinfo/concurrency-interest
On Fri, Jun 20, 2014 at 1:42 AM, Peter Levart <peter.levart@gmail.com> wrote:
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.
I tried this same approach, and found that: yes, we see some previously failing tests that start passing when the TLR is functional even while executing its own clinit method, BUT there are other tests that still fail, and only start passing when the network init code is removed ... for unknown reasons. We could debug them, but the fundamental danger remains - TLR may be invoked very early during JDK startup, and it is simply too risky to load higher-level libraries so early, possibly invoking foreign code that might do anything. Which is why I keep trying to minimize TLR's external dependencies. Whatever we decide to do here, the same changes should be made to SplittableRandom. Maybe there's scope for some refactoring/sharing, both of code and of file system access at runtime.
participants (3)
-
Doug Lea
-
Martin Buchholz
-
Peter Levart