RFR (M): 8184334: Generalizing Atomic with templates

[Erik Österlund]

Hi Roman,

Thank you for the review.

On 2017-07-17 12:00, Roman Kennke wrote:
> Hi Erik,
>
> I like the stuff. Cannot find anything wrong in the patch (i.e. ok from
> me as non-reviewer), but have some questions:
> - Do you plan to fix+remove the now deprecated methods, e.g. store_ptr()
> soon?

Yes, unless anyone else picks it up before me, I will do it. For now I 
have a bunch of more important stuff though that takes precedence to get 
the GC barrier interface out.
The ones that involve pointer arithmetic need to keep in mind during the 
conversion that add_ptr uses unscaled pointer arithmetic, whereas add() 
uses scaled pointer arithmetic.

> - Have you given any thought about C++11 atomics? I don't know if that
> will ever be feasible, but part of me would find it nice to just use
> that in the future? Or at least get/stay as close to it API-wise as
> possible?

Let's see if I can answer this question without getting into a long 
argument with Andrew Haley... ;)

1) I think since the frontends of Atomic/OrderAccess are used all over 
hotspot, it would take a lot of good motivation to change everything.
2) I do like the declarative semantics of C++11 semantics. However, I 
want declarative semantics to cover more than just the memory ordering 
semantics. That's why the Access API uses what I refer to as 
"decorators" to specify both memory ordering related semantics as well 
as other JVM-specific semantics. So you can do e.g. Atomic<MO_SEQ_CST | 
ACCESS_ON_WEAK>::oop_cas(...) for a sequentially consistent CAS on a 
weak reference, rather than having one system per orthogonal concern 
w.r.t. access semantics.
3) As a backend, I find C++11 atomics not always reliable as there is no 
contract as to what fencing convention is being used, e.g. whether 
leading-sync or trailing-sync is used for non-multiple copy atomic 
architectures, which may or may not imply "visibility" of stores, which 
may or may not matter when combined with weaker accesses like acquire. 
The C++11 atomic API was designed for having concurrent accesses in C++ 
playing well with other concurrent accesses in C++, where all the 
accesses involved go through the same atomic API, same convention, same 
rules. So stores could be designed in a certain way, knowing that 
corresponding loads have been designed in a certain way. That is not the 
situation in hotspot. Some accesses come from the C++ runtime, others 
come from hand written assembly or JITed code that makes subtle 
assumptions about the fencing conventions used in the VM. As we have no 
contract with what fencing C++11 atomic will generate, and it has 
changed over time, I feel hesitant about using C++11 atomic as a 
backend. The lack of contract about fencing convention makes this 
behaviour undefined. Perhaps that is okay, but I do not know how I feel 
about it. The possibility of silent and subtle changes in fencing 
generated by C++11 not playing well with JITed code seems scary to me. 
But perhaps I am just paranoid.

I hope this answers your questions.

Thanks,
/Erik

> Roman
>
> Am 17.07.2017 um 11:04 schrieb Erik Österlund:
>> Hi Kim,
>>
>> Thank you for the review. I have now fixed all the issues you mentioned.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.01/
>>
>> New incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00_01/
>>
>> Thanks,
>> /Erik
>>
>> On 2017-07-14 20:56, Kim Barrett wrote:
>>>> On Jul 14, 2017, at 12:28 PM, Erik Österlund
>>>> <erik.osterlund at oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8184334
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8184334/webrev.00/
>>> ------------------------------------------------------------------------------
>>>
>>> Another pre-review comment that seems to have been missed.  (Your
>>> reply indicated the suggestion was accepted.)
>>>
>>> src/share/vm/runtime/atomic.hpp
>>>    250   typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>>> intptr_t => U*
>>>
>>>    269     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>>>    286     typedef typename IntegerTypes::Signed<intptr_t>::type Raw;
>>> intptr_t => T*
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>