On Feb 20, 2014, at 9:13 AM, Alan Bateman <alan.bate...@oracle.com> wrote:
> On 19/02/2014 21:44, Brian Burkhalter wrote: >> This patch concerns cleaning up some ugly internal deprecations. >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8035279 >> Webrev: http://cr.openjdk.java.net/~bpb/8035279/webrev.00/ >> >> All JTREG BigInteger tests pass, and the serialized form has been unaltered >> as verified by bidirectional read-write testing between Java 7 and this >> version. >> >> I would appreciate scrutiny of the arithmetic to make sure that I've made no >> silly errors. Also I would be interested in opinions as to whether the >> "volatile" keyword should in fact be used for the affected instance >> variables. >> > It's not clear (to me) that these should be volatile. I also wonder about > setting the initial value too as volatile could make this expensive in some > environments. > Yes, IIUC only atomic access is required. Here be dragons! Not sure the static powerCache field, in the original code, needs to be volatile either: 1137 private static volatile BigInteger[][] powerCache; If the recently added volatile on the fields are removed the setting of them for deserialization should be moved above the putMag call to preserve construction semantics (although see Alan's point on serialization compat): 4272 // Calculate mag field from magnitude and discard magnitude 4273 UnsafeHolder.putMag(this, mag); 4274 if (mag.length >= MAX_MAG_LENGTH) { 4275 try { 4276 checkRange(); 4277 } catch (ArithmeticException e) { 4278 throw new java.io.StreamCorruptedException("BigInteger: Out of the supported range"); 4279 } 4280 } 4281 4282 // Initialize cached fields to indicate not computed yet. 4283 bitCount = Integer.MIN_VALUE; 4284 bitLength = Integer.MIN_VALUE; 4285 lowestSetBit = Integer.MIN_VALUE; 4286 firstNonzeroIntNum = Integer.MIN_VALUE; In fact the UnsafeHolder.putSign impl could be changed to use putObject rather than putObjectVolatile, since we just need to stamp a fence in after all fields have been set in readObject (alternatively Unsafe.fullFence could be used as the last call before readObject returns). Paul. > Are you sure there isn't any serialization impact? Zeros has also meant these > are uninitialized so I just wonder about deserializing on an older JDK. > > -Alan. >
