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
