RFR (M): 8188813: Generalize OrderAccess to use templates

[Erik Österlund]

Hi David,

Thanks for looking into this.

On 2017-10-06 10:19, David Holmes wrote:
> On 6/10/2017 4:48 PM, Erik Österlund wrote:
>> Hi David,
>>
>> On fre, 2017-10-06 at 16:01 +1000, David Holmes wrote:
>>> Hi Erik,
>>>
>>> On 5/10/2017 11:55 PM, Erik Österlund wrote:
>>>>
>>>> Hi,
>>>>
>>>> Now that Atomic has been generalized with templates, the same
>>>> should to
>>>> be done to OrderAccess.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8188813
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8188813/webrev.00/
>>> Well I didn't see anything too scary looking. :) I assume we'll drop
>>> the
>>> ptr variants at some stage.
>>
>> Yes, that is indeed the plan.
>>
>>> One query:
>>>
>>> src/hotspot/share/gc/shared/cardTableModRefBS.inline.hpp
>>>
>>> Can you declare "volatile jbyte* byte = ..." to avoid the volatile
>>> cast
>>> on the orderAccess call?
>>
>> Sure. Fixed.
>>
>>>
>>>>
>>>> Testing: mach5 hs-tier3
>>>>
>>>> Since Atomic already has a mechanism for type checking generic
>>>> arguments
>>>> for Atomic::load/store, and OrderAccess also is a bunch of
>>>> semantically
>>>> decorated loads and stores, I decided to reuse the template wheel
>>>> that
>>>> was already invented (Atomic::LoadImpl and Atomic::StoreImpl).
>>>> Therefore, I made OrderAccess privately inherit Atomic so that
>>>> this
>>>> infrastructure could be reused. A whole bunch of code has been
>>>> nuked
>>>> with this generalization.
>>> Good!
>>
>> :)
>>
>>>
>>>>
>>>> It is worth noting that I have added PrimitiveConversion
>>>> functionality
>>>> for doubles and floats which translates to using the union trick
>>>> for
>>>> casting double to and from int64_t and float to and from int32_t
>>>> when
>>>> passing down doubles and ints to the API. I need the former two,
>>>> because
>>>> Java supports volatile double and volatile float, and therefore
>>>> runtime
>>>> support for that needs to be able to use floats and doubles. I
>>>> also
>>> I didn't quite follow that. What parts of the runtime need to operate
>>> on
>>> volatile float/double Java fields?
>>
>> At the moment, there are multiple places that support the use of Java-
>> volatile float/double.
>>
>> Some examples:
>> * The static interpreter supports Java-volatile getfield/putfield (cf.
>> cppInterpreter_zero.cpp:588, bytecodeInterpreter.cpp:2023)
>
> Yes this is the _implementation_ of volatile field access for 
> floats/doubles. I don't count that as a "use". :)
>
>> * unsafe supports getters and setters of Java-volatile doubles/floats
>> (cf. unsafe.cpp:476).
>
> Yes this is more of a "use" but again very specific.

Naturally, in order to support Java-volatile doubles and floats in the 
VM, we have the choice of

1) Flicking the PrimitiveConversion double/float switch allowing this to 
be automatically solved by the API and not rewriting uses of OrderAccess 
for supporting Java-volatile, or
2) Treating ordered accesses of double/float as special cases requiring 
manual (and very specific) casting to do the same thing.

I thought alternative 1 was nicer, because I dislike unnecessary special 
cases.

>
>> This support is not accidental. The Java language allows the use of
>> volatile floats and doubles. Therefore we must support them in our
>> runtime.
>
> Not quite what I meant. :) Other than the implementation of the Java 
> volatile field accesses (direct of via Unsafe or intrinsics) I was 
> wondering where we might need to do this. The general runtime tends 
> not to do arbitary orderAccess or atomic operations on floats/doubles.

We do not need it for anything else than supporting Java-volatile in the VM.

Thanks,
/Erik

> Cheers,
> David
>
>> Thanks for the review.
>>
>> /Erik
>>
>>>
>>>>
>>>> added PrimitiveConversion functionality for the subclasses of oop
>>>> (instanceOop and friends). The base class oop already supported
>>>> this, so
>>>> it seemed natural that the subclasses should support it too.
>>> Ok.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>
>>>> Thanks,
>>>> /Erik