JDK 9 RFR of JDK-8035279: Clean up internal deprecations in BigInteger

[Peter Levart]

On 02/20/2014 11:20 AM, David Holmes wrote:
> Hi Brian,
>
> On 20/02/2014 7:44 AM, 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.
>
> re: volatile
>
> The basic approach being taken with these primitive fields is lazy 
> initialization to a value that will always be the same if recomputed. 
> If the variables are not volatile then in the worst-case (this is 
> theoretical only) they will be recomputed by each thread that accesses 
> them. If they are volatile then they will only be recomputed by 
> threads that truly race on the initialization logic, but you will 
> thereafter pay the price of a volatile read on each access.
>
> Either way you will be functionally correct so it all comes down to 
> performance. Practically speaking I would expect you to be better off 
> overall without the volatile.
>
> As Paul notes the powerCache array does not need to be volatile as it 
> is initialized as part of class initialization.
>
> Cheers,
> David

Hi Brian,

I would also be concerned about correctness here. You changed fields to 
be volatile and assigned them with different that default (zero) values 
in field initializers. Take for example the field bitCount:

|  156     private volatile int bitCount = Integer.MIN_VALUE;|


In code you check this initial value and decide whether to lazily 
compute the real value or not based on this check:

3298     public int bitCount() {
3299         // This computation has a known, acceptable non-critical race condition.
3300         if (bitCount == Integer.MIN_VALUE) {  // bitCount not initialized yet


There has recently been a discussion on concurrency-dev mailing list 
about whether this is safe:

http://cs.oswego.edu/pipermail/concurrency-interest/2013-November/011951.html

...in cases where the instance of such constructed object escapes to 
non-constructing thread via data-race. I think the conclusion was that 
such semantics is desirable and might be guaranteed in a future updated 
JMM, but currently it is not, although most JVMs are implemented in such 
a way that this is safe. Citing Doug Lea:

"
Right. To summarize:

* Programmers do not expect that even though final fields are specifically
publication-safe, volatile fields are not always so.

* For various implementation reasons, JVMs arrange that
volatile fields are publication safe anyway, at least in
cases we know about.

* Actually updating the JMM/JLS to mandate this is not easy
(no small tweak that I know applies). But now is a good time
to be considering a full revision for JDK9.

* In the mean time, it would make sense to further test
and validate JVMs as meeting this likely future spec.

-Doug
"

Is there a special reason you changed the "uninitialized" value from 
default (zero) to non-zero?


Regards, Peter