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

Reply via email to