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
