RFR: 8236778: Add Atomic::fetch_and_add

[Kim Barrett]

> 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.

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.