RFR: 8236778: Add Atomic::fetch_and_add

[Stefan Karlsson]

On 2020-01-10 02:50, Kim Barrett wrote:
>> On Jan 9, 2020, at 5:17 PM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> Updated webrev:
>>   https://cr.openjdk.java.net/~stefank/8236778/webrev.02.delta
>>   https://cr.openjdk.java.net/~stefank/8236778/webrev.02
> Looks good except for a couple more comment tweaks described below.
>
>>> src/hotspot/share/runtime/atomic.hpp
>>> Removed:
>>>   240   // - platform_add is an object of type PlatformAdd<sizeof(D)>.
>>>   241   //
>>>   242   // Then
>>>   243   //   platform_add(dest, add_value)
>>>   244   // must be a valid expression, returning a result convertible to D.
>>>
>>> and
>>>
>>>   250   // Helper base classes for defining PlatformAdd.  To use, define
>>> ...
>>>   275   // caller.
>>>
>>> These comments should have been updated to describe the new protocol
>>> for PlatformAdd, rather than simply removed.  Documentation of
>>> extension points like this is important.
>> The comments seemed to already have started deteriorating. It's seems to be missing an *if* that matches the *then,
> The structure is "Given <list of requirements> then <implications>";
> there was never an "if".  All of the similar comments in this file
> were written that way.

Well, there's no explicit *given* in the comments either. So, to me, it 
still sounds weird. Updated webrev with your comments below:
  https://cr.openjdk.java.net/~stefank/8236778/webrev.03.delta
  https://cr.openjdk.java.net/~stefank/8236778/webrev.03

StefanK

> But there's a further update that I just noticed is needed by your changes:
>
>   233   // class is a function object that must be default constructable,
>
> Delete "is a function object that" in the above.
>
>>   and it doesn't mention *order*.
> It's true that *order* isn't mentioned; that seems to have been missed
> when the order argument was added here.  (It *is* described in the
> xchg and cmpxchg commentary, and we could do similarly here.)
>
> Add
>
>    // - order is of type atomic_memory_order.
>
> to the requirements list, and change the expressions to include an
> order argument, e.g.
>
>    // Then both
>    //   platform_add.add_and_fetch(dest, add_value, order)
>    //   platform_add.fetch_and_add(dest, add_value, order)
>
>> So, instead of trying to figure out what the comment tried to say, I opted from getting rid of those problems by getting rid of that part of the comment. I thought the code was pretty self-explanatory, and didn't need that part. I'm not opposed to fixing the comments if you have suggestions.
> Unfortunately, documenting templates can be pretty hard. But because
> of the looseness of templates, differentiating between intentional
> semantics and accidents of today's implementation based only on the
> code can also be pretty hard. Yet failure to do so leads to
> brittleness and uncertainties, especially when there are third-party
> specializations. So I think it's worth making an attempt at
> documentation, even if it's hard and the results not always perfect.
>
>> I'll update the code verbatim with what you've suggested.
> Plus the couple further modifications mentioned above…
>
>>> src/hotspot/os_cpu/windows_x86/atomic_windows_x86.hpp
>>>    54 #pragma warning(disable: 4035) // Disables warnings reporting missing return statement
>>>
>>> Pre-existing: This warning suppression (and the corresponding
>>> restoration) appears to only be needed for the !defined(AMD64) case
>>> (where __asm statements are being used), and should be moved accordingly.
>> Will you create a bug report for that?
> https://bugs.openjdk.java.net/browse/JDK-8236900
> atomic_windows_x86.hpp disables warning 4035 more widely than needed
>
>> I'll interpret your comments above that there might be opportunities to do more changes, but none of those needs to be done for this patch.
> OK.
>
>> Right. I wasn't explicit about it, but I've compiled locally (linux) for aarch64, arm32, ppc64, s390, zero, minimal.
> Ah, that wasn’t clear to me from what you said.  Good.
>