RFR: 7143664: Clean up OrderAccess implementations and usage

[Erik Österlund]

Hi Karen,

On 23/02/15 19:46, Karen Kinnear wrote:
>
> On Feb 20, 2015, at 1:24 PM, Erik Österlund wrote:
>
> And truly many thanks - in my mind you have made this much more conceptually clean and safer.
> I am good with your checking in the parts I have reviewed.

Thank you for reviewing this!

>> Hi Karen,
>>
>> On 20/02/15 17:04, Karen Kinnear wrote:
>>> Erik,
>>>
>>> 1. orderAccess.hpp
>>> Were you going to add a comment about MSVC assumptions under C++ Volatile Semantics?
>>>
>>> Is it worth documenting in source somewhere what minimal versions of compilers are assumed?
>>> Or which versions of compilers were tested? I think that would help us in future please.
>>> Maybe in the platform-specific files?
>>
>> I did not intend to add this comment to shared. I thought that it would
>> be enough to say that non-volatile to volatile constraints can not be
>> trusted in general. Instead there is a comment explaining this anomaly
>> in orderAccess_windows_x86.inline.hpp where it is exploited, since it is
>> the only documented exception I know of.
>>
>> If we start picking certain examples, it feels like we should rather
>> have either 1) a comprehensive table of what volatile actually means for
>> different compilers with references or question marks where we don't
>> know (and hence need to be conservative assuming the worst), or 2)
>> putting such comments in platform specific files (like MSVC currently).
>>
>> Do you agree? In that case, which one do you prefer?
>>
>> As for documenting compiler versions assumed, would it be enough to
>> stamp the code with the currently used compilers we used now, even
>> though it might actually have worked with earlier versions? The
>> reasoning is that it's unlikely we will go back to using earlier
>> compilers anyway, so finding out when exactly certain compilers started
>> doing what we want them to could possibly be unnecessary, while still
>> having enough information to allow checking if future compilers have
>> provided improved functionality from this version of OrderAccess?
> It would make more sense to document for the individual platforms.
> I'm trying to prevent future "what were they thinking" questions by recording
> for each platform the specific compiler version tested. Not all versions that
> should work, just the ones we tested it with.

I agree this would be nice. I spoke to David who knows better than me 
which compiler versions were used. Big thanks for that! I updated 
comments in the platform specific files to reflect this.

>>
>>> 2. orderAccess_windows_x86.inline.hpp
>>> Could you possibly add to the comment about MSVC, that this assumes all of the Scoped templates explicitly cast the addresses to volatiles?
>>> (In case someone changes that some day?)
>>
>> Sure! But is this not clear given the comment in
>> orderAccess_windows_x86.inline.hpp where this is exploited next to the
>> scope template specialization:
>>
>> // Note that in MSVC, volatile memory accesses are explicitly
>> // guaranteed to have acquire release semantics (w.r.t. compiler
>> // reordering) and therefore does not even need a compiler barrier
>> // for normal acquire release accesses.
>> template<> inline void ScopedFence<X_ACQUIRE>::postfix()       { }
>> template<> inline void ScopedFence<RELEASE_X>::prefix()        { }
>> template<> inline void ScopedFence<RELEASE_X_FENCE>::prefix()  { }
>> template<> inline void ScopedFence<RELEASE_X_FENCE>::postfix() {
>> OrderAccess::fence(); }
>>
>> If you don't think this is clear enough, I would happily improve the
>> description.
> Perhaps I am being extra cautious since the very original set of calls all assumed the passed in arguments were
> already volatile, I wanted to save us this problem in 10 years by stating that this assumes that the Scoped templates explicitly
> cast the addresses to volatiles  (which is different than the caller passing in volatiles). So to me there were two ways to get there
> and I thought it would be valuable to specify this is the assumption. Not a big deal.

Agreed, made it even more explicit in the comment now.

>>
>>> 3. orderAccess_windows_x86.inline.hpp
>>> So I looked up _ReadWriteBarrier and VS2012 and VS2013 say this is deprecated.
>>> JDK8 builds on VS2010, so ok. JDK9 appears to be planning to move to VS2013, so we may need to change this.
>>> It appears that the recommendation is volatile (with default on x86 of /volatile:ms). Can we use __asm__volatile("" : : : "memory";
>>> compiler _barrier for VS2013 or is there an equivalent you know about?
>>
>> Unfortunately I currently have no access to windows platforms so I can't
>> experiment around. But I did some research when I looked into it and it
>> seems like _ReadWriteBarrier is all we got for now until we adopt C++11
>> std::atomic that Microsoft in the deprecation message on their website
>> suggests we should use instead.
>>
>> It seems to have been deprecated because programmers found it confusing
>> - the name does not suggest it's a compiler-only ordering constraint.
>> But that's fine for our purposes.
>>
>> I just assumed that for now we probably don't want to make such a
>> radical move to C++11, right? Since it was deprecation or C++11 I
>> intentionally used _ReadWriteBarrier anyway even though it is
>> deprecated. Is this a problem?
>>
>> When we need to switch, I believe we will be forced to use C++11 on
>> windows whether we like it or not. It has std::atomic_signal_fence which
>> should work as a drop-in replacement for _ReadWriteBarrier AFAIK. But
>> then again, we might as well just map all calls to std::atomic if we
>> gotta use C++11 anyway...
> Thank you. Looks like an area we will need to follow-up on.
> The C++ documentation says atomic_signal_fence just does atomic reordering.
> The Visual Studio 2012 documentation is not clear on whether it also adds hardware fences.
>
> So - go ahead and leave this as is and we will need to switch later.

Really? Hmm that's too bad. :/ Oh well the problem can probably be 
solved (in the future) directly by simply forwarding things to atomic 
using std::memory_order_acquire etc. I intentionally updated the memory 
model acquire/release definitions to make this easier in the future.

Big thanks for the review!

/Erik