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

[David Holmes]

Brian,

Peter is absolutely right - I realized my mistake lying in bed last night.

If the sentinel values were the default zero values there would be no 
issue but they are initialized to a different value. If the instances 
are then shared without a safe publication mechanism then it is possible 
that threads will see the zero default value and mistake that for an 
actual field value. So the fields have to be volatile to be guaranteed 
that their initialized value will be seen.

In practice, because there are also final fields in these instances 
implementations will most likely perform a storestore barrier after 
construction and prior to setting the reference to the created object. 
That would make non-volatile appear to work, but it would not be 
guaranteed to do so and the final-field "freeze" action only guarantees 
that unsafe publication will see the correct value of the final fields 
(and for final references anything reachable via that reference).

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

Note that this part of the discussion was shown to be false - it is 
certainly false in hotspot (except for the new AIX/PPC64 port).

Thanks,
David

On 21/02/2014 3:55 AM, Peter Levart wrote:
>
> 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
>
>
>
>
>
>
>