Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Martin, Alan,
OK, I finally got back to this.
I agree that Martins changes should be fine for jdk8, but removing all
these fields may cause an issue with jdk7. Removing pads, fine. The rnd
and initialized fields are still applicable in jdk7 ( or 7 will need
changes to remove their dependency ).
For example, a serialized TLR from jdk8 with Martins changes,
deserialized with jdk7 would contain no field values, therefore the
defaults would apply. Calling setSeed on this object would not throw the
expected UOE.
I think minimally TLR in jdk8 needs to keep initialized, and possibly
write some reasonable value for rnd.
-Chris.
On 19/02/2013 17:48, Martin Buchholz wrote:
On Wed, Jan 16, 2013 at 7:01 AM, Chris Hegarty chris.hega...@oracle.com
mailto:chris.hega...@oracle.com wrote:
Thanks to all for the reviews and suggestions here. As you probably
seen, I pushed these changes to jdk8/tl earlier today (sorry, I
missed Alan as an official reviewer on the changeset.). Consider it
an initial version, pending any outcome from this, or other,
discussions.
Also, 8006409: ThreadLocalRandom should dropping padding fields
from its serialized form, has been filed to follow up on the
changes required to update the serial form.
For those watching, I was preferable to push on with these changes (
and I apologize if it appeared pushy ), as the Double/Long
Adder/Accumulator implementation, Striped64, now has a dependency on
this change. It has become unbearable to keep in sync with different
version of across different repositories.
Taking a look at this, I don't see any reason why we can't simply do
(while maintaining serialization compatibility):
diff --git
a/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
b/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
--- a/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
+++ b/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
@@ -368,50 +368,6 @@
private static final long serialVersionUID = -5851777807851030925L;
/**
- * @serialField rnd long
- * @serialField initialized boolean
- * @serialField pad0 long
- * @serialField pad1 long
- * @serialField pad2 long
- * @serialField pad3 long
- * @serialField pad4 long
- * @serialField pad5 long
- * @serialField pad6 long
- * @serialField pad7 long
- */
-private static final ObjectStreamField[] serialPersistentFields = {
-new ObjectStreamField(rnd, long.class),
-new ObjectStreamField(initialized, boolean.class),
-new ObjectStreamField(pad0, long.class),
-new ObjectStreamField(pad1, long.class),
-new ObjectStreamField(pad2, long.class),
-new ObjectStreamField(pad3, long.class),
-new ObjectStreamField(pad4, long.class),
-new ObjectStreamField(pad5, long.class),
-new ObjectStreamField(pad6, long.class),
-new ObjectStreamField(pad7, long.class) };
-
-/**
- * Saves the {@code ThreadLocalRandom} to a stream (that is,
serializes it).
- */
-private void writeObject(java.io.ObjectOutputStream out)
-throws java.io.IOException {
-
-java.io.ObjectOutputStream.PutField fields = out.putFields();
-fields.put(rnd, 0L);
-fields.put(initialized, true);
-fields.put(pad0, 0L);
-fields.put(pad1, 0L);
-fields.put(pad2, 0L);
-fields.put(pad3, 0L);
-fields.put(pad4, 0L);
-fields.put(pad5, 0L);
-fields.put(pad6, 0L);
-fields.put(pad7, 0L);
-out.writeFields();
-}
-
-/**
* Returns the {@link #current() current} thread's {@code
ThreadLocalRandom}.
*/
private Object readResolve() {
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On Wed, Feb 20, 2013 at 3:16 AM, Chris Hegarty chris.hega...@oracle.comwrote: Martin, Alan, OK, I finally got back to this. I agree that Martins changes should be fine for jdk8, but removing all these fields may cause an issue with jdk7. Removing pads, fine. The rnd and initialized fields are still applicable in jdk7 ( or 7 will need changes to remove their dependency ). For example, a serialized TLR from jdk8 with Martins changes, deserialized with jdk7 would contain no field values, therefore the defaults would apply. Calling setSeed on this object would not throw the expected UOE. My expectation was that with my changes, the default serialization would apply, and the serialized form would contain initiatialized=1. rnd would not be in the serialized form, but would deserialize to 0, which is the same as currently done. Completely untested. BTW, we still don't have a good story on cross-JDK serialization testing. I think minimally TLR in jdk8 needs to keep initialized, and possibly write some reasonable value for rnd. Whether rnd needs a reasonable value is a reasonable question.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 20/02/2013 12:16, Martin Buchholz wrote:
On Wed, Feb 20, 2013 at 3:16 AM, Chris Hegarty chris.hega...@oracle.com
mailto:chris.hega...@oracle.com wrote:
Martin, Alan,
OK, I finally got back to this.
I agree that Martins changes should be fine for jdk8, but removing
all these fields may cause an issue with jdk7. Removing pads, fine.
The rnd and initialized fields are still applicable in jdk7 ( or 7
will need changes to remove their dependency ).
For example, a serialized TLR from jdk8 with Martins changes,
deserialized with jdk7 would contain no field values, therefore the
defaults would apply. Calling setSeed on this object would not throw
the expected UOE.
My expectation was that with my changes, the default serialization would
apply, and the serialized form would contain initiatialized=1. rnd
would not be in the serialized form, but would deserialize to 0, which
is the same as currently done.
Yes, you are right. For some reason I was thinking that 'initialized'
field was also removed from this version. So this is fine.
Completely untested.
BTW, we still don't have a good story on cross-JDK serialization testing.
I think minimally TLR in jdk8 needs to keep initialized, and
possibly write some reasonable value for rnd.
Whether rnd needs a reasonable value is a reasonable question.
Unfortunately, if we agree that rnd needs a reasonable value then we are
stuck with serialPersistentFields, etc ( less the padding ).
How about (and additionally remove the pads):
--- a/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
Tue Feb 19 20:52:39 2013 -0800
+++ b/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
Wed Feb 20 14:08:28 2013 +
@@ -398,7 +398,7 @@ public class ThreadLocalRandom extends R
throws java.io.IOException {
java.io.ObjectOutputStream.PutField fields = out.putFields();
-fields.put(rnd, 0L);
+fields.put(rnd, UNSAFE.getLong(Thread.currentThread(), SEED));
fields.put(initialized, true);
fields.put(pad0, 0L);
fields.put(pad1, 0L);
-Chris.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On Wed, Jan 16, 2013 at 7:01 AM, Chris Hegarty chris.hega...@oracle.comwrote:
Thanks to all for the reviews and suggestions here. As you probably seen,
I pushed these changes to jdk8/tl earlier today (sorry, I missed Alan as an
official reviewer on the changeset.). Consider it an initial version,
pending any outcome from this, or other, discussions.
Also, 8006409: ThreadLocalRandom should dropping padding fields from its
serialized form, has been filed to follow up on the changes required to
update the serial form.
For those watching, I was preferable to push on with these changes ( and I
apologize if it appeared pushy ), as the Double/Long Adder/Accumulator
implementation, Striped64, now has a dependency on this change. It has
become unbearable to keep in sync with different version of across
different repositories.
Taking a look at this, I don't see any reason why we can't simply do
(while maintaining serialization compatibility):
diff --git a/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
b/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
--- a/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
+++ b/src/share/classes/java/util/concurrent/ThreadLocalRandom.java
@@ -368,50 +368,6 @@
private static final long serialVersionUID = -5851777807851030925L;
/**
- * @serialField rnd long
- * @serialField initialized boolean
- * @serialField pad0 long
- * @serialField pad1 long
- * @serialField pad2 long
- * @serialField pad3 long
- * @serialField pad4 long
- * @serialField pad5 long
- * @serialField pad6 long
- * @serialField pad7 long
- */
-private static final ObjectStreamField[] serialPersistentFields = {
-new ObjectStreamField(rnd, long.class),
-new ObjectStreamField(initialized, boolean.class),
-new ObjectStreamField(pad0, long.class),
-new ObjectStreamField(pad1, long.class),
-new ObjectStreamField(pad2, long.class),
-new ObjectStreamField(pad3, long.class),
-new ObjectStreamField(pad4, long.class),
-new ObjectStreamField(pad5, long.class),
-new ObjectStreamField(pad6, long.class),
-new ObjectStreamField(pad7, long.class) };
-
-/**
- * Saves the {@code ThreadLocalRandom} to a stream (that is,
serializes it).
- */
-private void writeObject(java.io.ObjectOutputStream out)
-throws java.io.IOException {
-
-java.io.ObjectOutputStream.PutField fields = out.putFields();
-fields.put(rnd, 0L);
-fields.put(initialized, true);
-fields.put(pad0, 0L);
-fields.put(pad1, 0L);
-fields.put(pad2, 0L);
-fields.put(pad3, 0L);
-fields.put(pad4, 0L);
-fields.put(pad5, 0L);
-fields.put(pad6, 0L);
-fields.put(pad7, 0L);
-out.writeFields();
-}
-
-/**
* Returns the {@link #current() current} thread's {@code
ThreadLocalRandom}.
*/
private Object readResolve() {
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 19/02/2013 17:48, Martin Buchholz wrote: : Taking a look at this, I don't see any reason why we can't simply do (while maintaining serialization compatibility): Right, it's not needed and I think we had Chris persuaded on this a few weeks ago. It does change the serialized form but that it's not an issue issue and it doesn't matter that they have the default value when deserialized on an older release. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 19 Feb 2013, at 18:35, Alan Bateman alan.bate...@oracle.com wrote: On 19/02/2013 17:48, Martin Buchholz wrote: : Taking a look at this, I don't see any reason why we can't simply do (while maintaining serialization compatibility): Right, it's not needed and I think we had Chris persuaded on this a few weeks ago. It does change the serialized form but that it's not an issue issue and it doesn't matter that they have the default value when deserialized on an older release. Agreed, but we do need to have this approved by the CCC. I'll follow up on this. -Chris -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Hello, I did an experiment with an alternative implementation of TLR: http://dl.dropbox.com/u/101777488/TLR/webrev.01/index.html This version is a modified proposed version. It has the following differences: - Instead of Thread.threadLocalRandomSeed field it has a Thread.threadLocalRandom reference field, pointing to an instance of TLR. - The ThreadLocalRandom is not a singleton, but an instance per thread (like JDK7's). - ThreadLocalRandom has a reference to constructing Thread, so it can check the validity of calling thread. I did some micro benchmarks and here are the results: http://dl.dropbox.com/u/101777488/TLR/TLR_benchmark_results.txt Results indicate that usage pattern: Thread.current().nextInt() is as fast as proposed variant while the nextInt() method itself is as fast as JDK7's, which is some 20% faster than proposed variant. So the alternative implementation seems to be faster and it has the following additional benefits: - Checks the calling thread and throws when called from invalid thread. - Could reinstate the padding fields (or @Contended long rnd) if needed (the tests were done without padding) It does have a memory overhead of one object per using thread. Not too much I think. Regards, Peter On 01/15/2013 05:33 PM, Chris Hegarty wrote: Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ * based on Aleksey initial webrev * applied serialPersistentFields, writeObject, readResolve ( as suggested by Heinz ) * did not apply readObject. It is unnecessary, defaultReadObject will read all the fields off the stream readResolve is necessary to give us good behavior ( as noted previously ). Once integrated, I will file a new bug to track the possible change of serialized form of TLR, and this can then remove serialPersistentFields and writeObject, if successful. -Chris. On 01/15/2013 01:57 PM, Alan Bateman wrote: On 15/01/2013 13:49, Chris Hegarty wrote: But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? Technically deleting fields is an incompatible change as the stream will not contain the fields. If someone is crazy enough to serialize with jdk8 TLR and send it to a jdk7 release then the fields will have their default value (as the values aren't in the stream). As the fields aren't used then I don't think this should make a difference. Going the other way shouldn't be an issue either as the pad fields will be ignored. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Hi Peter, This is an interesting experiment. On 01/16/2013 12:59 PM, Peter Levart wrote: I did some micro benchmarks and here are the results: http://dl.dropbox.com/u/101777488/TLR/TLR_benchmark_results.txt Results indicate that usage pattern: Thread.current().nextInt() is as fast as proposed variant while the nextInt() method itself is as fast as JDK7's, which is some 20% faster than proposed variant. I find this hard to believe, since the baseline experiment I did in the conceiving note in this thread actually tells otherwise: JDK8 (baseline, 4 threads) TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 16.1 +- 0.4 ns/op TL.get().nextInt(): 19.1 +- 0.6 ns/op JDK8 (patched, 4 threads) TLR.nextInt(): 6.5 +- 0.2 ns/op TLR.current().nextInt(): 6.4 +- 0.1 ns/op TL.get().nextInt(): 17.2 +- 2.0 ns/op That is, TLR.current().nextInt() is as fast as the already-acquired TLR.nextInt() even in the baselined case, which pretty much means we hit the lower bound for possible infrastructure overheads, and we actually compute. Note that the changes in next() did not degrade the performance either. Please check your microbenchmarks. So the alternative implementation seems to be faster and it has the following additional benefits: - Checks the calling thread and throws when called from invalid thread. This is the spec change; potentially breaks the code (instead of just degrading performance). I would not like to see that pushed into JDK. - Could reinstate the padding fields (or @Contended long rnd) if needed (the tests were done without padding) Hardly a benefit, since Thread can be padded as well, and padding there will yield better footprint. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Hi Aleksey, The test I used is simple: https://raw.github.com/plevart/lambda-hacks/master/jdk-test/src/org/openjdk/tests/java/util/concurrent/atomic/RandomTest.java I ran it on a i7-2600K (4 cores x 2 threads) PC with Linux OS and a JDK8 build from lambda repo using default JVM options (that means +UseCompressedOops enabled by default). Can you give the sources of your tests so I could try to run them on my machine? Regards, Peter On 01/16/2013 10:13 AM, Aleksey Shipilev wrote: Hi Peter, This is an interesting experiment. On 01/16/2013 12:59 PM, Peter Levart wrote: I did some micro benchmarks and here are the results: http://dl.dropbox.com/u/101777488/TLR/TLR_benchmark_results.txt Results indicate that usage pattern: Thread.current().nextInt() is as fast as proposed variant while the nextInt() method itself is as fast as JDK7's, which is some 20% faster than proposed variant. I find this hard to believe, since the baseline experiment I did in the conceiving note in this thread actually tells otherwise: JDK8 (baseline, 4 threads) TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 16.1 +- 0.4 ns/op TL.get().nextInt(): 19.1 +- 0.6 ns/op JDK8 (patched, 4 threads) TLR.nextInt(): 6.5 +- 0.2 ns/op TLR.current().nextInt(): 6.4 +- 0.1 ns/op TL.get().nextInt(): 17.2 +- 2.0 ns/op That is, TLR.current().nextInt() is as fast as the already-acquired TLR.nextInt() even in the baselined case, which pretty much means we hit the lower bound for possible infrastructure overheads, and we actually compute. Note that the changes in next() did not degrade the performance either. Please check your microbenchmarks. So the alternative implementation seems to be faster and it has the following additional benefits: - Checks the calling thread and throws when called from invalid thread. This is the spec change; potentially breaks the code (instead of just degrading performance). I would not like to see that pushed into JDK. - Could reinstate the padding fields (or @Contended long rnd) if needed (the tests were done without padding) Hardly a benefit, since Thread can be padded as well, and padding there will yield better footprint. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/16/2013 01:23 PM, Peter Levart wrote: The test I used is simple: https://raw.github.com/plevart/lambda-hacks/master/jdk-test/src/org/openjdk/tests/java/util/concurrent/atomic/RandomTest.java OK, you can't get serious with that test. The failures visible at the surface are: - dead-code elimination for parts of nextInt(): you should consume the results! - thread start/stops are not synchronized; there are significant chances threads are running solo for considerable amount of time. This should be much easier to write once our microbenchmark harness goes into OpenJDK; in the mean time, you will need to invest considerably more time into making this right. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/16/13 03:59, Peter Levart wrote: - Instead of Thread.threadLocalRandomSeed field it has a Thread.threadLocalRandom reference field, pointing to an instance of TLR. - The ThreadLocalRandom is not a singleton, but an instance per thread (like JDK7's). Yes, this is the Plan B I mentioned in mail last week. I had explored this. Both on performance and resource grounds, it is a little worse. Not hugely worse, but if we are going to improve TLR, then might as well take the best option. (Another reason to prefer current solution also serves as a reply to Tom: Yes we don't like using Unsafe just to bypass package-protection access control. But at least it is just accessing simple scalars, not references.) -Doug
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/16/2013 12:48 PM, Doug Lea wrote: On 01/16/13 03:59, Peter Levart wrote: - Instead of Thread.threadLocalRandomSeed field it has a Thread.threadLocalRandom reference field, pointing to an instance of TLR. - The ThreadLocalRandom is not a singleton, but an instance per thread (like JDK7's). Yes, this is the Plan B I mentioned in mail last week. I had explored this. Both on performance and resource grounds, it is a little worse. Not hugely worse, but if we are going to improve TLR, then might as well take the best option. (Another reason to prefer current solution also serves as a reply to Tom: Yes we don't like using Unsafe just to bypass package-protection access control. But at least it is just accessing simple scalars, not references.) Don't forget it is also writing them... :-( Regards, Peter -Doug
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Thanks to all for the reviews and suggestions here. As you probably seen, I pushed these changes to jdk8/tl earlier today (sorry, I missed Alan as an official reviewer on the changeset.). Consider it an initial version, pending any outcome from this, or other, discussions. Also, 8006409: ThreadLocalRandom should dropping padding fields from its serialized form, has been filed to follow up on the changes required to update the serial form. For those watching, I was preferable to push on with these changes ( and I apologize if it appeared pushy ), as the Double/Long Adder/Accumulator implementation, Striped64, now has a dependency on this change. It has become unbearable to keep in sync with different version of across different repositories. -Chris. On 16/01/2013 11:48, Doug Lea wrote: On 01/16/13 03:59, Peter Levart wrote: - Instead of Thread.threadLocalRandomSeed field it has a Thread.threadLocalRandom reference field, pointing to an instance of TLR. - The ThreadLocalRandom is not a singleton, but an instance per thread (like JDK7's). Yes, this is the Plan B I mentioned in mail last week. I had explored this. Both on performance and resource grounds, it is a little worse. Not hugely worse, but if we are going to improve TLR, then might as well take the best option. (Another reason to prefer current solution also serves as a reply to Tom: Yes we don't like using Unsafe just to bypass package-protection access control. But at least it is just accessing simple scalars, not references.) -Doug
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Doug Lea wrote:
On 01/14/13 17:58, Chris Hegarty wrote:
I'm not sure what we are saving here. Since new TLR is a singleton, we
save ~64 bytes per classloader. Worth messing with advanced
serialization mechanics? Probably not. I'll publish a revised webrev
soon.
serialPersistentFields is really quite simple, and will help avoid
any potential issues like this in the future. I can help implement
this on top of your patch if you like.
Thanks to Alan and Aleksey for noticing this and to Chris for
offering some serialPersistentFields incantations!
(The only way to serialize a TLR represents a strange abuse to
begin with. You'd need to save the result of
ThreadLocalRandom.current() in a field of a serialized object.
Which would be a terrible idea ...)
Right, which means we should also add a readResolve() method to return
ThreadLocalRandom.current(). All you need to do is add this to the
class and the format should be compatible with Java 1.7. This means
that if someone made the mistake of writing ThreadLocalRandom instances,
they would now still be able to read, but would instead get the correct
instance back for their thread:
/**
* We keep the same serial persistent format as in Java 1.7.
*/
private static final ObjectStreamField[] serialPersistentFields = {
new ObjectStreamField(rnd, long.class),
new ObjectStreamField(initialized, boolean.class),
new ObjectStreamField(pad0, long.class),
new ObjectStreamField(pad1, long.class),
new ObjectStreamField(pad2, long.class),
new ObjectStreamField(pad3, long.class),
new ObjectStreamField(pad4, long.class),
new ObjectStreamField(pad5, long.class),
new ObjectStreamField(pad6, long.class),
new ObjectStreamField(pad7, long.class),
};
/**
* Writes the ThreadLocalRandom object to the ObjectOutputStream
using the
* same format as in the past.
*/
private void writeObject(ObjectOutputStream out) throws IOException {
ObjectOutputStream.PutField fields = out.putFields();
fields.put(rnd, rnd);
fields.put(initialized, true);
fields.put(pad0, 0L);
fields.put(pad1, 0L);
fields.put(pad2, 0L);
fields.put(pad3, 0L);
fields.put(pad4, 0L);
fields.put(pad5, 0L);
fields.put(pad6, 0L);
fields.put(pad7, 0L);
out.writeFields();
}
/**
* Reads the ThreadLocalRandom object from the stream in order to keep
* the format compatible, but does not use any of the read data.
*/
private void readObject(ObjectInputStream in)
throws ClassNotFoundException, IOException {
in.readFields();
// we can ignore the values, since we will replace them in the
// readResolve() method anyway
}
/**
* Once the ThreadLocalRandom object has been read from the stream, we
* throw it away and instead return the correct instance for our thread.
*/
private Object readResolve() {
return current();
}
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 14/01/2013 23:55, Doug Lea wrote: Thanks to Alan and Aleksey for noticing this and to Chris for offering some serialPersistentFields incantations! (The only way to serialize a TLR represents a strange abuse to begin with. You'd need to save the result of ThreadLocalRandom.current() in a field of a serialized object. Which would be a terrible idea ...) It does seem nonsensical. Given that the padding isn't used then the simplest thing might be to do nothing, meaning treat this update as an API change that changes the serialized form. I don't see any compatibility issues as deserialization on an older release will just leave the padding fields with their default values (and as they are unused then it shouldn't matter). I think this would be simplest than adding serialPersistentFields and a writeObject to write these unused fields. -Alan
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 09:55 AM, Alan Bateman wrote: On 14/01/2013 23:55, Doug Lea wrote: Thanks to Alan and Aleksey for noticing this and to Chris for offering some serialPersistentFields incantations! (The only way to serialize a TLR represents a strange abuse to begin with. You'd need to save the result of ThreadLocalRandom.current() in a field of a serialized object. Which would be a terrible idea ...) It does seem nonsensical. Given that the padding isn't used then the simplest thing might be to do nothing, meaning treat this update as an API change that changes the serialized form. I don't see any compatibility issues as deserialization on an older release will just leave the padding fields with their default values (and as they are unused then it shouldn't matter). I think this would be simplest than adding serialPersistentFields and a writeObject to write these unused fields. -Alan If this is an API change, then it might as well be a change in the serializability of the ThreadLocalRandom instance. Since TLR extends Random which is already Serializable, this could be done by throwing appropriate exception in writeObject(). I don't think anyone would really notice. Serializability of a java.util.Random is meant to transfer the state of the object over the wire (including current seed). This can't be done for ThreadLocalRandom since it's a singleton with thread-bound state. So serializability of TLR is dubious. Regards, Peter
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 01:40 PM, Peter Levart wrote: If this is an API change, then it might as well be a change in the serializability of the ThreadLocalRandom instance. Since TLR extends Random which is already Serializable, this could be done by throwing appropriate exception in writeObject(). I don't think anyone would really notice. Serializability of a java.util.Random is meant to transfer the state of the object over the wire (including current seed). This can't be done for ThreadLocalRandom since it's a singleton with thread-bound state. So serializability of TLR is dubious. That's an interesting thought indeed. However, I would say already receiving TLR from the stream and throwing the don't want it exception would be surprising. We are at the curse of having these fields exposed in serialized form; the only thing we can do is to minimize further leakage. Chris, do you need help preparing the patch? -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 09:55 AM, Alan Bateman wrote: On 14/01/2013 23:55, Doug Lea wrote: Thanks to Alan and Aleksey for noticing this and to Chris for offering some serialPersistentFields incantations! (The only way to serialize a TLR represents a strange abuse to begin with. You'd need to save the result of ThreadLocalRandom.current() in a field of a serialized object. Which would be a terrible idea ...) It does seem nonsensical. Given that the padding isn't used then the simplest thing might be to do nothing, meaning treat this update as an API change that changes the serialized form. I don't see any compatibility issues as deserialization on an older release will just leave the padding fields with their default values (and as they are unused then it shouldn't matter). I think this would be simplest than adding serialPersistentFields and a writeObject to write these unused fields. And one more thing. With moving of fields off the TLR object, the semantics of de-serialization change. JDK7 TLR deserializes into an instance which is not thread-bound with the copy of the seed of original instance. So serializing/deserializing acts as a form of cloning the instance and detaching the copy from the thread. Proposed TLR can't keep this semantics (unless it is modeled as a private subclass of TLR for example which overrides existing logic and provides a writeReplace() method that replaces it with plain TLR instance before serializing). Regards, Peter
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 15/01/2013 09:40, Peter Levart wrote: If this is an API change, then it might as well be a change in the serializability of the ThreadLocalRandom instance. Since TLR extends Random which is already Serializable, this could be done by throwing appropriate exception in writeObject(). I don't think anyone would really notice. Dubious as it might be, I'm sure it would break somebody so I think it would be safer not to do this. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 09:57 AM, Aleksey Shipilev wrote:
On 01/15/2013 01:40 PM, Peter Levart wrote:
If this is an API change, then it might as well be a change in the
serializability of the ThreadLocalRandom instance. Since TLR extends
Random which is already Serializable, this could be done by throwing
appropriate exception in writeObject().
I don't think anyone would really notice.
Serializability of a java.util.Random is meant to transfer the state of
the object over the wire (including current seed). This can't be done
for ThreadLocalRandom since it's a singleton with thread-bound state. So
serializability of TLR is dubious.
That's an interesting thought indeed. However, I would say already
receiving TLR from the stream and throwing the don't want it exception
would be surprising. We are at the curse of having these fields exposed
in serialized form; the only thing we can do is to minimize further leakage.
Chris, do you need help preparing the patch?
I'm still not sure that we have agreement here.
My preference, for what it's worth, is to forge ahead with Peter's
suggestion, and update the TLR specification to throw in the case of
writeObject. I believe this would be the best solution.
Add similar text to the class description:
* Note, that although ThreadLocalRandom inherits Serializable
* interface from Random, it is not intended to be Serializable.
* Appropriate serialization methods are implemented to throw
* NotSerializableException.
/**
* Throws NotSerializableException, since TLR
* objects are not intended to be serializable.
*/
private void writeObject(java.io.ObjectOutputStream out)
throws NotSerializableException
{
throw new NotSerializableException(Not serializable.);
}
private void readObject(java.io.ObjectInputStream in)
throws NotSerializableException
{
// swallow all field data.
}
This of course, is subject to spec change approval.
-Chris.
-Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 02:12 PM, Chris Hegarty wrote:
On 01/15/2013 09:57 AM, Aleksey Shipilev wrote:
Chris, do you need help preparing the patch?
I'm still not sure that we have agreement here.
Sorry, I should not have replied in this thread.
My preference, for what it's worth, is to forge ahead with Peter's
suggestion, and update the TLR specification to throw in the case of
writeObject. I believe this would be the best solution.
Add similar text to the class description:
* Note, that although ThreadLocalRandom inherits Serializable
* interface from Random, it is not intended to be Serializable.
* Appropriate serialization methods are implemented to throw
* NotSerializableException.
/**
* Throws NotSerializableException, since TLR
* objects are not intended to be serializable.
*/
private void writeObject(java.io.ObjectOutputStream out)
throws NotSerializableException
{
throw new NotSerializableException(Not serializable.);
}
private void readObject(java.io.ObjectInputStream in)
throws NotSerializableException
{
// swallow all field data.
}
I'm technically OK with this, plus:
+ readResolve returning the actual TLR
+ SVUID fixup
This of course, is subject to spec change approval.
...but this leads me back to thinking if we should still persist the
leaked serialization form (serialPersistentFields code put together by
Heinz is fine) to make this functionality in JDK8 for Doug to rely on,
and then back off to suggestions for collapsing serialization forms.
-Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
(The only way to serialize a TLR represents a strange abuse to
begin with. You'd need to save the result of
ThreadLocalRandom.current() in a field of a serialized object.
Which would be a terrible idea ...)
And so, anything sensible that we do here will have the
(too nice?) effect of fixing the errors of anyone
crazy enough to do this -- rather than deserializing into
a non-thread-local ThreadLocalRandom, it will attach to
the common one.
Given all this, I think Heinz's suggestion below looks suitable.
Chris, do you agree about the serialization incantations?
-Doug
On 01/15/13 03:06, Dr Heinz M. Kabutz wrote:
Right, which means we should also add a readResolve() method to return
ThreadLocalRandom.current(). All you need to do is add this to the class and
the format should be compatible with Java 1.7. This means that if someone made
the mistake of writing ThreadLocalRandom instances, they would now still be able
to read, but would instead get the correct instance back for their thread:
/**
* We keep the same serial persistent format as in Java 1.7.
*/
private static final ObjectStreamField[] serialPersistentFields = {
new ObjectStreamField(rnd, long.class),
new ObjectStreamField(initialized, boolean.class),
new ObjectStreamField(pad0, long.class),
new ObjectStreamField(pad1, long.class),
new ObjectStreamField(pad2, long.class),
new ObjectStreamField(pad3, long.class),
new ObjectStreamField(pad4, long.class),
new ObjectStreamField(pad5, long.class),
new ObjectStreamField(pad6, long.class),
new ObjectStreamField(pad7, long.class),
};
/**
* Writes the ThreadLocalRandom object to the ObjectOutputStream using the
* same format as in the past.
*/
private void writeObject(ObjectOutputStream out) throws IOException {
ObjectOutputStream.PutField fields = out.putFields();
fields.put(rnd, rnd);
fields.put(initialized, true);
fields.put(pad0, 0L);
fields.put(pad1, 0L);
fields.put(pad2, 0L);
fields.put(pad3, 0L);
fields.put(pad4, 0L);
fields.put(pad5, 0L);
fields.put(pad6, 0L);
fields.put(pad7, 0L);
out.writeFields();
}
/**
* Reads the ThreadLocalRandom object from the stream in order to keep
* the format compatible, but does not use any of the read data.
*/
private void readObject(ObjectInputStream in)
throws ClassNotFoundException, IOException {
in.readFields();
// we can ignore the values, since we will replace them in the
// readResolve() method anyway
}
/**
* Once the ThreadLocalRandom object has been read from the stream, we
* throw it away and instead return the correct instance for our thread.
*/
private Object readResolve() {
return current();
}
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 15/01/2013 13:25, Doug Lea wrote: : And so, anything sensible that we do here will have the (too nice?) effect of fixing the errors of anyone crazy enough to do this -- rather than deserializing into a non-thread-local ThreadLocalRandom, it will attach to the common one. Given all this, I think Heinz's suggestion below looks suitable. Chris, do you agree about the serialization incantations? Heinz's proposal is fine but I think you also have the option of just dropping the pad fields from the serialization form (meaning no need for serialPersistentFields or writeObject). Technically it would be API change but one that shouldn't have any impact as the fields were never used and hence deserializing to their default value is okay. -Alan
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 01:25 PM, Doug Lea wrote:
(The only way to serialize a TLR represents a strange abuse to
begin with. You'd need to save the result of
ThreadLocalRandom.current() in a field of a serialized object.
Which would be a terrible idea ...)
And so, anything sensible that we do here will have the
(too nice?) effect of fixing the errors of anyone
crazy enough to do this -- rather than deserializing into
a non-thread-local ThreadLocalRandom, it will attach to
the common one.
Given all this, I think Heinz's suggestion below looks suitable.
Chris, do you agree about the serialization incantations?
Yes, I am starting to come to this conclusion too (Heinz's suggestion).
There are a few @serialField tags to be added so we continue to generate
correct javadoc. Otherwise, I think we are good. I'll follow up on this
and send out a complete patch for review.
If the TLR spec is ever to be updated to specify that it is not ( any
more ) serializable (Peters suggestion), writeObect throw, we can do
this at a later stage.
-Chris.
-Doug
On 01/15/13 03:06, Dr Heinz M. Kabutz wrote:
Right, which means we should also add a readResolve() method to return
ThreadLocalRandom.current(). All you need to do is add this to the
class and
the format should be compatible with Java 1.7. This means that if
someone made
the mistake of writing ThreadLocalRandom instances, they would now
still be able
to read, but would instead get the correct instance back for their
thread:
/**
* We keep the same serial persistent format as in Java 1.7.
*/
private static final ObjectStreamField[] serialPersistentFields = {
new ObjectStreamField(rnd, long.class),
new ObjectStreamField(initialized, boolean.class),
new ObjectStreamField(pad0, long.class),
new ObjectStreamField(pad1, long.class),
new ObjectStreamField(pad2, long.class),
new ObjectStreamField(pad3, long.class),
new ObjectStreamField(pad4, long.class),
new ObjectStreamField(pad5, long.class),
new ObjectStreamField(pad6, long.class),
new ObjectStreamField(pad7, long.class),
};
/**
* Writes the ThreadLocalRandom object to the ObjectOutputStream
using the
* same format as in the past.
*/
private void writeObject(ObjectOutputStream out) throws IOException {
ObjectOutputStream.PutField fields = out.putFields();
fields.put(rnd, rnd);
fields.put(initialized, true);
fields.put(pad0, 0L);
fields.put(pad1, 0L);
fields.put(pad2, 0L);
fields.put(pad3, 0L);
fields.put(pad4, 0L);
fields.put(pad5, 0L);
fields.put(pad6, 0L);
fields.put(pad7, 0L);
out.writeFields();
}
/**
* Reads the ThreadLocalRandom object from the stream in order to
keep
* the format compatible, but does not use any of the read data.
*/
private void readObject(ObjectInputStream in)
throws ClassNotFoundException, IOException {
in.readFields();
// we can ignore the values, since we will replace them in the
// readResolve() method anyway
}
/**
* Once the ThreadLocalRandom object has been read from the
stream, we
* throw it away and instead return the correct instance for our
thread.
*/
private Object readResolve() {
return current();
}
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 01:37 PM, Alan Bateman wrote: On 15/01/2013 13:25, Doug Lea wrote: : And so, anything sensible that we do here will have the (too nice?) effect of fixing the errors of anyone crazy enough to do this -- rather than deserializing into a non-thread-local ThreadLocalRandom, it will attach to the common one. Given all this, I think Heinz's suggestion below looks suitable. Chris, do you agree about the serialization incantations? Heinz's proposal is fine but I think you also have the option of just dropping the pad fields from the serialization form (meaning no need for serialPersistentFields or writeObject). Technically it would be API change but one that shouldn't have any impact as the fields were never used and hence deserializing to their default value is okay. But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? -Chris. -Alan
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 15/01/2013 13:49, Chris Hegarty wrote: But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? Technically deleting fields is an incompatible change as the stream will not contain the fields. If someone is crazy enough to serialize with jdk8 TLR and send it to a jdk7 release then the fields will have their default value (as the values aren't in the stream). As the fields aren't used then I don't think this should make a difference. Going the other way shouldn't be an issue either as the pad fields will be ignored. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 02:57 PM, Alan Bateman wrote: On 15/01/2013 13:49, Chris Hegarty wrote: But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? Technically deleting fields is an incompatible change as the stream will not contain the fields. If someone is crazy enough to serialize with jdk8 TLR and send it to a jdk7 release then the fields will have their default value (as the values aren't in the stream). As the fields aren't used then I don't think this should make a difference. Going the other way shouldn't be an issue either as the pad fields will be ignored. -Alan. Should there also be a change/patch to JDK7 TLR that would fix the strange behaviour (by adding readResolve)? This would align the semantics no matter which direction (7/7, 7/8, 8/7, 8/8) of serialization/deserialization is used... Regards, Peter
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
One more thing, not related to serialization: If a TLR reference is somehow passed from the thread that obtained it via TLR.current() to some other thread that did never call TLR.current() and this other thread calls methods on such instance (nextInt(), ...), it will start the random sequence from the zero seed, bypassing localInit() call... Is this ok? Regards, Peter
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 06:33 PM, Peter Levart wrote: One more thing, not related to serialization: If a TLR reference is somehow passed from the thread that obtained it via TLR.current() to some other thread that did never call TLR.current() and this other thread calls methods on such instance (nextInt(), ...), it will start the random sequence from the zero seed, bypassing localInit() call... Is this ok? I think this counts as accidental sharing, and Javadoc recommends to always do TLR.current().* to take the appropriate TLR; hence I think this is the adopted failure scenario. The upside for *not* fixing this is skipping the initialized checks from each of next*() methods, which are needed in corner cases only, thus saving the hot-path performance. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 03:38 PM, Aleksey Shipilev wrote: On 01/15/2013 06:33 PM, Peter Levart wrote: One more thing, not related to serialization: If a TLR reference is somehow passed from the thread that obtained it via TLR.current() to some other thread that did never call TLR.current() and this other thread calls methods on such instance (nextInt(), ...), it will start the random sequence from the zero seed, bypassing localInit() call... Is this ok? I think this counts as accidental sharing, and Javadoc recommends to always do TLR.current().* to take the appropriate TLR; hence I think this is the adopted failure scenario. The upside for *not* fixing this is skipping the initialized checks from each of next*() methods, which are needed in corner cases only, thus saving the hot-path performance. If javadoc recommends to always do TLR.current() then this method is also hot-path. So why not move the initialization check from current() to next(int bits)? Well, the following usage pattern can save some checks in the original case: TLR r = TLR.current(); r.next*(); r.next*(); Regards, Peter -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ * based on Aleksey initial webrev * applied serialPersistentFields, writeObject, readResolve ( as suggested by Heinz ) * did not apply readObject. It is unnecessary, defaultReadObject will read all the fields off the stream readResolve is necessary to give us good behavior ( as noted previously ). Once integrated, I will file a new bug to track the possible change of serialized form of TLR, and this can then remove serialPersistentFields and writeObject, if successful. -Chris. On 01/15/2013 01:57 PM, Alan Bateman wrote: On 15/01/2013 13:49, Chris Hegarty wrote: But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? Technically deleting fields is an incompatible change as the stream will not contain the fields. If someone is crazy enough to serialize with jdk8 TLR and send it to a jdk7 release then the fields will have their default value (as the values aren't in the stream). As the fields aren't used then I don't think this should make a difference. Going the other way shouldn't be an issue either as the pad fields will be ignored. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 08:33 PM, Chris Hegarty wrote: Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ Looks good to me; thanks for picking this up! -Aleksey. P.S. How many (Java) (concurrency) experts it takes to make a change to ThreadLocalRandom?
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/13 11:33, Chris Hegarty wrote: Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ Looks great; thanks!! -Doug * based on Aleksey initial webrev * applied serialPersistentFields, writeObject, readResolve ( as suggested by Heinz ) * did not apply readObject. It is unnecessary, defaultReadObject will read all the fields off the stream readResolve is necessary to give us good behavior ( as noted previously ). Once integrated, I will file a new bug to track the possible change of serialized form of TLR, and this can then remove serialPersistentFields and writeObject, if successful. -Chris. On 01/15/2013 01:57 PM, Alan Bateman wrote: On 15/01/2013 13:49, Chris Hegarty wrote: But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? Technically deleting fields is an incompatible change as the stream will not contain the fields. If someone is crazy enough to serialize with jdk8 TLR and send it to a jdk7 release then the fields will have their default value (as the values aren't in the stream). As the fields aren't used then I don't think this should make a difference. Going the other way shouldn't be an issue either as the pad fields will be ignored. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
From user's point of view, there is nothing thread local about it - it is more like one global random generator, which is unseedable and unrepeatable. So why not call it a different name, like concurrent random? On Mon, Jan 14, 2013 at 4:17 PM, Alan Bateman alan.bate...@oracle.com wrote: On 14/01/2013 18:20, Chris Hegarty wrote: Ooh this change goes beyond my review capability!!! I thought we were just eliminating the indirection of ThreadLocal, anyway... As a basic review I don't see anything obviously wrong, and I don't have a problem with adding the fields to j.l.Thread, but I'm not in a position to review in detail the algorithm used. Alan, did mention that he may be in a better position to do a detailed review. I can also sponsor this change. I looked at the changes and they look good to me. The additional fields of java.lang.Thread shouldn't be an issue. The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 15/01/2013 16:33, Chris Hegarty wrote: Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ * based on Aleksey initial webrev * applied serialPersistentFields, writeObject, readResolve ( as suggested by Heinz ) * did not apply readObject. It is unnecessary, defaultReadObject will read all the fields off the stream readResolve is necessary to give us good behavior ( as noted previously ). Once integrated, I will file a new bug to track the possible change of serialized form of TLR, and this can then remove serialPersistentFields and writeObject, if successful. The plan and the changes looks good to me too. -Alan
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 15/01/2013 16:33, Chris Hegarty wrote: Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ Any chance of using SharedSecrets instead of UNSAFE? You know, with Unsafe being like unsafe and everything. Tom
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Just a minor note: In writeObject(), the code writes constant 0L for field rnd. If we're trying to emulate the stream of JDK7 TLR then writeObject() could do better by writing the value of Thread.currentThread().threadLocalRandomSeed field... Regards, Peter On 01/15/2013 05:33 PM, Chris Hegarty wrote: Updated webrev: http://cr.openjdk.java.net/~chegar/8005926/webrev.01/webrev/ * based on Aleksey initial webrev * applied serialPersistentFields, writeObject, readResolve ( as suggested by Heinz ) * did not apply readObject. It is unnecessary, defaultReadObject will read all the fields off the stream readResolve is necessary to give us good behavior ( as noted previously ). Once integrated, I will file a new bug to track the possible change of serialized form of TLR, and this can then remove serialPersistentFields and writeObject, if successful. -Chris. On 01/15/2013 01:57 PM, Alan Bateman wrote: On 15/01/2013 13:49, Chris Hegarty wrote: But wouldn't there be an issue with JDK7 TLR streams being deserialized by JDK8, pad fields would be left in the Object input steam? If so, then we're down the road of having to have a readObject, etc... and possibly back to serialPersistentFields? Technically deleting fields is an incompatible change as the stream will not contain the fields. If someone is crazy enough to serialize with jdk8 TLR and send it to a jdk7 release then the fields will have their default value (as the values aren't in the stream). As the fields aren't used then I don't think this should make a difference. Going the other way shouldn't be an issue either as the pad fields will be ignored. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
I understand everyone is busy with JDK8 milestone, but maybe some critical people have missed this note? It would be terrific if someone could review this (or even sponsor it!). Thanks, Aleksey. On 01/11/2013 02:31 AM, Aleksey Shipilev wrote: Hi, Submitting this on behalf of Doug Lea. The webrev is here: http://cr.openjdk.java.net/~shade/8005926/webrev.00/ Bottom-line: merge ThreadLocalRandom state into Thread, to optimize many use cases around j.u.c.* code. The simple performance tests on 2x2 i5, Linux x86_64, 4 threads, 5 forks, 3x3s warmup, 5x3s measurement: JDK8 (baseline) TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 16.1 +- 0.4 ns/op TL.get().nextInt(): 19.1 +- 0.6 ns/op JDK8 (patched) TLR.nextInt(): 6.5 +- 0.2 ns/op TLR.current().nextInt(): 6.4 +- 0.1 ns/op TL.get().nextInt(): 17.2 +- 2.0 ns/op First line shows the peak performance of TLR itself, everything over that is the ThreadLocal overhead. One can see the patched version bypasses ThreadLocal machinery completely, and the overhead is slim to none. N.B. It gets especially interesting when there are many ThreadLocals registered. Making 1M ThreadLocals and pre-touching them bloats the thread-local maps, and we get: JDK8 (baseline), contaminators = 1M: TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 21.7 +- 5.3 ns/op TL.get().nextInt(): 28.7 +- 1.1 ns/op JDK8 (patched), contaminators = 1M: TLR.nextInt(): 6.6 +- 0.2 ns/op TLR.current().nextInt(): 6.5 +- 0.1 ns/op TL.get().nextInt(): 29.4 +- 0.5 ns/op Note that patched version successfully dodges this pathological case. Testing: - Doug tested on his platforms - Tested Linux x86_64 to build and run successfully - JPRT builds are OK - JPRT tests are OK (modulo some weird lambda/default-methods test failures in jdk8/tl) Attribution: - dl: original patch - shade: testing, copyright headers, etc. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
For what it is worth, it is on my list for today. -Chris. On 14/01/2013 13:31, Aleksey Shipilev wrote: I understand everyone is busy with JDK8 milestone, but maybe some critical people have missed this note? It would be terrific if someone could review this (or even sponsor it!). Thanks, Aleksey. On 01/11/2013 02:31 AM, Aleksey Shipilev wrote: Hi, Submitting this on behalf of Doug Lea. The webrev is here: http://cr.openjdk.java.net/~shade/8005926/webrev.00/ Bottom-line: merge ThreadLocalRandom state into Thread, to optimize many use cases around j.u.c.* code. The simple performance tests on 2x2 i5, Linux x86_64, 4 threads, 5 forks, 3x3s warmup, 5x3s measurement: JDK8 (baseline) TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 16.1 +- 0.4 ns/op TL.get().nextInt(): 19.1 +- 0.6 ns/op JDK8 (patched) TLR.nextInt(): 6.5 +- 0.2 ns/op TLR.current().nextInt(): 6.4 +- 0.1 ns/op TL.get().nextInt(): 17.2 +- 2.0 ns/op First line shows the peak performance of TLR itself, everything over that is the ThreadLocal overhead. One can see the patched version bypasses ThreadLocal machinery completely, and the overhead is slim to none. N.B. It gets especially interesting when there are many ThreadLocals registered. Making 1M ThreadLocals and pre-touching them bloats the thread-local maps, and we get: JDK8 (baseline), contaminators = 1M: TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 21.7 +- 5.3 ns/op TL.get().nextInt(): 28.7 +- 1.1 ns/op JDK8 (patched), contaminators = 1M: TLR.nextInt(): 6.6 +- 0.2 ns/op TLR.current().nextInt(): 6.5 +- 0.1 ns/op TL.get().nextInt(): 29.4 +- 0.5 ns/op Note that patched version successfully dodges this pathological case. Testing: - Doug tested on his platforms - Tested Linux x86_64 to build and run successfully - JPRT builds are OK - JPRT tests are OK (modulo some weird lambda/default-methods test failures in jdk8/tl) Attribution: - dl: original patch - shade: testing, copyright headers, etc. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Ooh this change goes beyond my review capability!!! I thought we were just eliminating the indirection of ThreadLocal, anyway... As a basic review I don't see anything obviously wrong, and I don't have a problem with adding the fields to j.l.Thread, but I'm not in a position to review in detail the algorithm used. Alan, did mention that he may be in a better position to do a detailed review. I can also sponsor this change. -Chris. On 14/01/2013 13:52, Chris Hegarty wrote: For what it is worth, it is on my list for today. -Chris. On 14/01/2013 13:31, Aleksey Shipilev wrote: I understand everyone is busy with JDK8 milestone, but maybe some critical people have missed this note? It would be terrific if someone could review this (or even sponsor it!). Thanks, Aleksey. On 01/11/2013 02:31 AM, Aleksey Shipilev wrote: Hi, Submitting this on behalf of Doug Lea. The webrev is here: http://cr.openjdk.java.net/~shade/8005926/webrev.00/ Bottom-line: merge ThreadLocalRandom state into Thread, to optimize many use cases around j.u.c.* code. The simple performance tests on 2x2 i5, Linux x86_64, 4 threads, 5 forks, 3x3s warmup, 5x3s measurement: JDK8 (baseline) TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 16.1 +- 0.4 ns/op TL.get().nextInt(): 19.1 +- 0.6 ns/op JDK8 (patched) TLR.nextInt(): 6.5 +- 0.2 ns/op TLR.current().nextInt(): 6.4 +- 0.1 ns/op TL.get().nextInt(): 17.2 +- 2.0 ns/op First line shows the peak performance of TLR itself, everything over that is the ThreadLocal overhead. One can see the patched version bypasses ThreadLocal machinery completely, and the overhead is slim to none. N.B. It gets especially interesting when there are many ThreadLocals registered. Making 1M ThreadLocals and pre-touching them bloats the thread-local maps, and we get: JDK8 (baseline), contaminators = 1M: TLR.nextInt(): 6.4 +- 0.1 ns/op TLR.current().nextInt(): 21.7 +- 5.3 ns/op TL.get().nextInt(): 28.7 +- 1.1 ns/op JDK8 (patched), contaminators = 1M: TLR.nextInt(): 6.6 +- 0.2 ns/op TLR.current().nextInt(): 6.5 +- 0.1 ns/op TL.get().nextInt(): 29.4 +- 0.5 ns/op Note that patched version successfully dodges this pathological case. Testing: - Doug tested on his platforms - Tested Linux x86_64 to build and run successfully - JPRT builds are OK - JPRT tests are OK (modulo some weird lambda/default-methods test failures in jdk8/tl) Attribution: - dl: original patch - shade: testing, copyright headers, etc. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On Mon, Jan 14, 2013 at 10:20 AM, Chris Hegarty chris.hega...@oracle.com wrote: Ooh this change goes beyond my review capability!!! We don't really need another one, since we already have Aleksey and Doug, right? We should cc: Doug on threads like this (even though he is on the list).
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On Mon, Jan 14, 2013 at 12:15 PM, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: I'm not sure I'm listed as reviewer by OpenJDK census. If that does not matter, then yes, count me as the reviewer :) OpenJDK should give Aleksey some reviewer bits, but I don't know how that happens.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 14/01/2013 18:20, Chris Hegarty wrote: Ooh this change goes beyond my review capability!!! I thought we were just eliminating the indirection of ThreadLocal, anyway... As a basic review I don't see anything obviously wrong, and I don't have a problem with adding the fields to j.l.Thread, but I'm not in a position to review in detail the algorithm used. Alan, did mention that he may be in a better position to do a detailed review. I can also sponsor this change. I looked at the changes and they look good to me. The additional fields of java.lang.Thread shouldn't be an issue. The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. -Alan.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 02:17 AM, Alan Bateman wrote: The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. Damn. These *are* the part of TLR serialized form :( http://docs.oracle.com/javase/7/docs/api/serialized-form.html#java.util.concurrent.ThreadLocalRandom Also, the class changes probably screwed up the SUID. I think we need to get the padding back (which should not be the problem since TLR is now a singleton, and add the SUID with the previous value. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
The padded fields are interesting, as in Java 7 they would quite possibly be optimized away or at least rearranged, no? My own tests certainly suggested this. Probably a case for @Contended? Also, do you intend on moving the padding fields over to Thread too? Other than my questions about padding, for what it's worth, I think this is a splendid suggestion :-) On 15/01/2013, Alan Bateman alan.bate...@oracle.com wrote: On 14/01/2013 18:20, Chris Hegarty wrote: Ooh this change goes beyond my review capability!!! I thought we were just eliminating the indirection of ThreadLocal, anyway... As a basic review I don't see anything obviously wrong, and I don't have a problem with adding the fields to j.l.Thread, but I'm not in a position to review in detail the algorithm used. Alan, did mention that he may be in a better position to do a detailed review. I can also sponsor this change. I looked at the changes and they look good to me. The additional fields of java.lang.Thread shouldn't be an issue. The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. -Alan. -- Dr Heinz M. Kabutz (PhD CompSci) Author of The Java(tm) Specialists' Newsletter Sun Java Champion IEEE Certified Software Development Professional http://www.javaspecialists.eu Tel: +30 69 75 595 262 Skype: kabutz
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
We can probably fake the serialized form with some field descriptors and a writeObject and readObject method. Then we can keep the format exactly the same as previously. The serialized form should not stop us from optimizing this very important class. Just my 2c On 15/01/2013, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:17 AM, Alan Bateman wrote: The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. Damn. These *are* the part of TLR serialized form :( http://docs.oracle.com/javase/7/docs/api/serialized-form.html#java.util.concurrent.ThreadLocalRandom Also, the class changes probably screwed up the SUID. I think we need to get the padding back (which should not be the problem since TLR is now a singleton, and add the SUID with the previous value. -Aleksey. -- Dr Heinz M. Kabutz (PhD CompSci) Author of The Java(tm) Specialists' Newsletter Sun Java Champion IEEE Certified Software Development Professional http://www.javaspecialists.eu Tel: +30 69 75 595 262 Skype: kabutz
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Agreed. But I think just faking it with the plain fields is less error-prone. The memory overhead is somewhat 8x8 = 64 bytes per JVM instance. -Aleksey. On 01/15/2013 02:40 AM, Dr Heinz M. Kabutz wrote: We can probably fake the serialized form with some field descriptors and a writeObject and readObject method. Then we can keep the format exactly the same as previously. The serialized form should not stop us from optimizing this very important class. Just my 2c On 15/01/2013, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:17 AM, Alan Bateman wrote: The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. Damn. These *are* the part of TLR serialized form :( http://docs.oracle.com/javase/7/docs/api/serialized-form.html#java.util.concurrent.ThreadLocalRandom Also, the class changes probably screwed up the SUID. I think we need to get the padding back (which should not be the problem since TLR is now a singleton, and add the SUID with the previous value. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 14 Jan 2013, at 22:33, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:17 AM, Alan Bateman wrote: The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. Damn. These *are* the part of TLR serialized form :( http://docs.oracle.com/javase/7/docs/api/serialized-form.html#java.util.concurrent.ThreadLocalRandom Also, the class changes probably screwed up the SUID. I think we need to get the padding back (which should not be the problem since TLR is now a singleton, and add the SUID with the previous value. Or serialPersistentFields? I was thinking of this anyway, so we could get rid of rnd. -Chris -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 02:36 AM, Dr Heinz M. Kabutz wrote: The padded fields are interesting, as in Java 7 they would quite possibly be optimized away or at least rearranged, no? At my level of HotSpot knowledge I find this very unlikely. Once the class is loaded, it's layout is finalized. That pretty much means the padding is with you forever (this is a nice side-effect current implementation of @Contended is relying on). My own tests certainly suggested this. Probably a case for @Contended? Also, do you intend on moving the padding fields over to Thread too? Doug insists there is no reason to pad Threads. I suspect there is the reason. This is the tie needing the empirical study. Preliminary data (in this thread) suggests we don't need to pad. At this point we can just wait for dust to settle in, make a do-over for perf experiments and decide whether to stick @Contended on Thread or not. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 02:47 AM, Chris Hegarty wrote: On 14 Jan 2013, at 22:33, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:17 AM, Alan Bateman wrote: The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. Damn. These *are* the part of TLR serialized form :( http://docs.oracle.com/javase/7/docs/api/serialized-form.html#java.util.concurrent.ThreadLocalRandom Also, the class changes probably screwed up the SUID. I think we need to get the padding back (which should not be the problem since TLR is now a singleton, and add the SUID with the previous value. Or serialPersistentFields? I was thinking of this anyway, so we could get rid of rnd. I'm not sure what we are saving here. Since new TLR is a singleton, we save ~64 bytes per classloader. Worth messing with advanced serialization mechanics? Probably not. I'll publish a revised webrev soon. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 14 Jan 2013, at 22:53, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:47 AM, Chris Hegarty wrote: On 14 Jan 2013, at 22:33, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:17 AM, Alan Bateman wrote: The other thing I notice is that the serialized form of the original ThreadLocalRandom included the padding fields, I guess they should have been transient in the original implementation. Damn. These *are* the part of TLR serialized form :( http://docs.oracle.com/javase/7/docs/api/serialized-form.html#java.util.concurrent.ThreadLocalRandom Also, the class changes probably screwed up the SUID. I think we need to get the padding back (which should not be the problem since TLR is now a singleton, and add the SUID with the previous value. Or serialPersistentFields? I was thinking of this anyway, so we could get rid of rnd. I'm not sure what we are saving here. Since new TLR is a singleton, we save ~64 bytes per classloader. Worth messing with advanced serialization mechanics? Probably not. I'll publish a revised webrev soon. serialPersistentFields is really quite simple, and will help avoid any potential issues like this in the future. I can help implement this on top of your patch if you like. -Chris. -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/15/2013 02:58 AM, Chris Hegarty wrote: serialPersistentFields is really quite simple, and will help avoid any potential issues like this in the future. I can help implement this on top of your patch if you like. Oh, please go ahead. That's the learning opportunity for me (never used sPF before, ready to learn from masters). -Aleksey.
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
Yeah, that's what I meant in my earlier post. It keeps the serial format compatible with little impact on performance in this case. On 15/01/2013, Aleksey Shipilev aleksey.shipi...@oracle.com wrote: On 01/15/2013 02:58 AM, Chris Hegarty wrote: serialPersistentFields is really quite simple, and will help avoid any potential issues like this in the future. I can help implement this on top of your patch if you like. Oh, please go ahead. That's the learning opportunity for me (never used sPF before, ready to learn from masters). -Aleksey. -- Dr Heinz M. Kabutz (PhD CompSci) Author of The Java(tm) Specialists' Newsletter Sun Java Champion IEEE Certified Software Development Professional http://www.javaspecialists.eu Tel: +30 69 75 595 262 Skype: kabutz
Re: RFR (S): CR 8005926: (thread) Merge ThreadLocalRandom state into java.lang.Thread
On 01/14/13 17:58, Chris Hegarty wrote: I'm not sure what we are saving here. Since new TLR is a singleton, we save ~64 bytes per classloader. Worth messing with advanced serialization mechanics? Probably not. I'll publish a revised webrev soon. serialPersistentFields is really quite simple, and will help avoid any potential issues like this in the future. I can help implement this on top of your patch if you like. Thanks to Alan and Aleksey for noticing this and to Chris for offering some serialPersistentFields incantations! (The only way to serialize a TLR represents a strange abuse to begin with. You'd need to save the result of ThreadLocalRandom.current() in a field of a serialized object. Which would be a terrible idea ...) -Doug
