JDK-8204301 : Make OrderAccess functions available to hpp rather than inline.hpp files
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 11
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2018-06-04
  • Updated: 2018-06-12
  • Resolved: 2018-06-06
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 11
11 b17Fixed
Related Reports
Relates :  
JDK-8188764 - Obsolete AssumeMP and then remove all support for non-MP builds
Relates :  
JDK-8200295 - Clean up jfrTypeIds.inline.hpp
Description
Move OrderAccess::load_acquire and store_release and functions that support them into orderAccess.hpp file so that template functions needing these functions don't have to be split into .inline.hpp files, which can only be included in other .inline.hpp or .cpp files as per coding standard.

Requires removing os::is_MP from OrderAccess::fence, which is a partial fix for JDK-8188764.
Comments
Okay.
05-06-2018
I'm suggesting leaving the ARM32 checks for os::is_MP() and letting the include of os.hpp drag in all the other includes on that platform and leave it to ARM32 to find a better solution if this is a problem (and if it doesn't build). I'm suggesting removing the checks on the 3 x86 operating systems because the static variable check costs on all platforms that we currently support and eliding the barrier isn't a very valuable optimization on the platforms that could be treated as not MP. Then I can make some progress on the inline inclusion cleanup for the JFR code.
05-06-2018
That comment means the is_MP() checks exist inside the orderAccess function rather than around the call to that function. The issue on ARM32 is not the cost of the check versus always adding the barrier, it's the fact that if we still need to support ARMv5 builds (and possibly ARMv6 too) that the instructions do not exist in that ISA. So if we remove the is_MP() check (which should always be false for ARMv5 as our ARM32 code doesn't support MP ARMv5) we'd have to add another runtime check to avoid generating bad instructions.
05-06-2018
Only x86 platforms have the test for os::is_MP() in fence, and arm32 has the test before all of the OrderAccess functions. I wonder if the two global variable tests are more expensive than eliding it completely. It seems a pointless optimization - how to tell? From John Rose's comment: Item #1 is by far the most common, and is easily removed, since the memory barriers can be self-gating (expanding to no-ops on uniprocessors). The comment titled "is_MP Considered Redundant" in orderAccess.hpp reflects this point. For uses like this the is_MP can be removed without a trace.
05-06-2018
There's no decision either way, it's on the backburner. It may still go ahead. I don't see any easy fix here. Moving fence() to a .cpp file fixes the .inline issue but then fence() is not inlineable - and that may have a performance implication. How much effort is it to move to a .cpp file and test this out? Or can we fix the include of os.hpp with a copied forward declaration of os::is_MP ? No because is_MP itself is declared inline. <sigh>. This whole transitive inlining thing seems completely broken to me.
05-06-2018
So with containers/Dockers, you're going to un-deprecate AssumeMP?
05-06-2018
The check only exists in fence() in general. I thought we had a lot more checks for the other orderAccess operations as well. The "we only support MP systems" position pre-dates the rise of containers/Docker and the apparent use of such configured for single CPU use. That's why this went on the backburner. It's no longer clear that we don't need to optimize for this case.
05-06-2018
Yes, OrderAccess *does* still have this check. My reading of John's comment in the CSR was that we want to strip it out because we only support MP systems. I guess it depends on arm32, but I could put the functions in the .cpp file if needed or leave the os::is_MP() check and keep the #include os.hpp. These aren't templatized. Adding [~bulasevich]
05-06-2018
If arm32 is still used for armv5 or armv6 then it still needs the check. For armv5 the instructions don't exist and will trigger SIGILL. I'm a bit perplexed here as I don't recall the templatization of orderAccess removing existing os::is_MP() checks. I thought even for x86 etc we still checked for MP inside orderAccess. The whole point of OrderAccess is to express the required semantics in the shared code, ie insert a fence(), and then the implementation of that does what is needed for the current systems - which for UP would be nothing! I don't recall stripping this out.
04-06-2018
Yes, I saw that it was targeted to 12. I was trying to do a smaller cleanup for 11 before feature freeze and this was in the way of that. I think the inclusion of os.hpp was one reason that the functions are in orderAccess.inline.hpp (fence). Without that, the functions can be moved to orderAccess.hpp. Most of the functions use inline asm or platform intrinsics. open webrev at http://cr.openjdk.java.net/~coleenp/8204301.01/webrev This passes on our platforms. The only thing I'm worried about here is arm32 because maybe it needs os::is_MP() because some arm32 platforms might not be.
04-06-2018
JDK-8188764 is not going ahead at this time (for 11, will be revisited for 12). I thought all the template stuff we used with orderAccess is why we need these to be in a .inline.hpp file?
04-06-2018