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

[Stuart Marks]

On 2/20/14 9:55 AM, Peter Levart wrote:
> On 02/20/2014 11:20 AM, David Holmes wrote:
>> 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.
>
> 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.

Arrrgh. Yet another hairy JMM discussion .... :-)

Brian and I had a discussion about this internally, and I recommended volatile, 
but mainly because I couldn't convince myself that it was correct in the absence 
of volatile. But my understanding of the JMM could be wrong (I think I'm in good 
company here) so I appreciate the additional discussion.

In the context of the November concurrency-interest discussion, the phrase 
"escape ... via data race" implies to me that the object is *not* safely 
published. My assumption is that if the BigInteger is safely published, there is 
no issue. If it's not safely published, then other threads could see the default 
value (zero) of bitCount instead of the sentinel value MIN_VALUE, rely on zero 
improperly, and end up computing an incorrect result. That would be bad.

Making the field volatile would mitigate this issue, or so I thought.

> 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.

Yes -- this is a surprise to me.

Two more data points:

  - String.hash is not volatile but is potentially written by any thread. This 
is a similar data race. But it's not initialized at construction time, and the 
code uses the default value zero as a special case.

  - Joe Darcy updated the bug report with a pointer to some discussion that 
occurred several years ago on core-libs-dev, which was the code review of the 
changes that introduced the code Brian is proposing to change. I'll include the 
link here for convenience:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2009-February/001105.html

(I haven't read it yet.) It may shed some light on the current discussion.

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

Zero is a valid value for some (all?) of the fields, so zero can't be used 
directly as an "uninitialized" sentinel. The previous code offset the value 
space to allow zero as a sentinel, at a cost of sprinkling +1's, -1's, +2's, and 
-2's throughout the code.

s'marks