RFR 8158039 VarHandle float/double field/array access should support CAS/set/add atomics

[Paul Sandoz]

> On 2 Jun 2016, at 09:13, David Holmes <david.holmes at oracle.com> wrote:
> 
> On 2/06/2016 1:24 AM, Paul Sandoz wrote:
>> (Please note this work is or will be covered with FC exception)
>> 
>> Hi,
>> 
>> Please review the VarHandles/Unsafe support for atomic ops on double/float fields/arrays.
> 
> I think this is misguided. If the user wants CAS for float/double they can do the conversion to int/long in a context where they know that conversion makes sense and where they can deal with NaNs appropriately.
> 

I would still like to support some clearly defined default behaviour if at all possible, since this is not easy to get right so we can add some value here. The user can use int/long if that behaviour is not suitable.


> At a minimum the fact this deals with raw bit patterns not semantic values needs to be exposed somehow. ie the handling of NaN needs to be explicit.
> 

Yes, it’s multiple NaN values that collapse to a single NaN value.

It would be good if our resident floating point expert Mr. Joe Darcy could chime in this respect.

On Float.intBitsToFloat:

* <p>Note that this method may not be able to return a
* {@code float} NaN with exactly same bit pattern as the
* {@code int} argument.  IEEE 754 distinguishes between two
* kinds of NaNs, quiet NaNs and <i>signaling NaNs</i>.  The
* differences between the two kinds of NaN are generally not
* visible in Java.  Arithmetic operations on signaling NaNs turn
* them into quiet NaNs with a different, but often similar, bit
* pattern.  However, on some processors merely copying a
* signaling NaN also performs that conversion.  In particular,
* copying a signaling NaN to return it to the calling method may
* perform this conversion.  So {@code intBitsToFloat} may
* not be able to return a {@code float} with a signaling NaN
* bit pattern.  Consequently, for some {@code int} values,
* {@code floatToRawIntBits(intBitsToFloat(start))} may
* <i>not</i> equal {@code start}.  Moreover, which
* particular bit patterns represent signaling NaNs is platform
* dependent; although all NaN bit patterns, quiet or signaling,
* must be in the NaN range identified above.



I am concerned that the CAS loops could in some cases loop without termination:

@ForceInline
public final float getAndAddFloat(Object o, long offset, float delta) {
    float v;
    do {
        v = getFloatVolatile(o, offset);
    } while (!weakCompareAndSwapFloatVolatile(o, offset, v, v + delta));
    return v;
}


I think this should be:

@ForceInline
public final float getAndAddFloat(Object o, long offset, float delta) {
    int expectedBits;
    float v;
    do {
        expectedBits = getIntVolatile(o, offset);
        v = Float.intBitsToFloat(bits); // May not preserve a NaN value with the same bit pattern as expectedBits
    } while (!weakCompareAndSwapIntVolatile(o, offset, expectedBits, Float.floatToRawIntBits(v + delta)));
    return v;
}

and likewise for the atomic get-and-set methods.

Paul.

> David
> -----
> 
>>  http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8158039-float-double-field-array-cas.jdk/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8158039-float-double-field-array-cas.jdk/webrev/>
>>  http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8158039-float-double-field-array-cas.hotspot/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8158039-float-double-field-array-cas.hotspot/webrev/>
>> 
>> These patches are based on those of for sub-word CAS
>> 
>>  https://bugs.openjdk.java.net/browse/JDK-8157726 <https://bugs.openjdk.java.net/browse/JDK-8157726>
>>  http://cr.openjdk.java.net/~shade/8157726/webrev.hs.03/ <http://cr.openjdk.java.net/~shade/8157726/webrev.hs.03/>
>>  http://cr.openjdk.java.net/~shade/8157726/webrev.jdk.03/ <http://cr.openjdk.java.net/~shade/8157726/webrev.jdk.03/>
>> 
>> And the two patches combined expand the support of fields/arrays.
>> 
>> 
>> New float/double CAS methods are added to Unsafe that defer to int/long equivalents. Then the other atomic methods are built from weak CAS loops.
>> 
>> No changes are required to HotSpot, but changes are required to the Unsafe tests in the hotspot repository.
>> 
>> In general the changes to VHs and tests are minimal since it is triggered from changes to the generating scripts that now include float/double into the CAS category.
>> 
>> There are some minor specification changes and a CCC has been initiated.
>> 
>> JPRT tests results show no relevant failures for hotspot and core test sets.
>> 
>> Thanks,
>> Paul.
>>