RFR(XXS): 8235931: add OM_CACHE_LINE_SIZE and use smaller size on SPARCv9 and X64

[David Holmes]

Hi Dan,

All my comments are addressed re motivation and performance. One follow 
up at the end.

<trimming>

On 20/12/2019 3:51 am, Daniel D. Daugherty wrote:
> And back to David:
>>
>>> Otherwise the mechanics of the change look fine, except I would 
>>> suggest that this:
>>>
>>> #ifndef OM_CACHE_LINE_SIZE
>>> // Use DEFAULT_CACHE_LINE_SIZE if not already specified for
>>> // the current build platform.
>>> #define OM_CACHE_LINE_SIZE DEFAULT_CACHE_LINE_SIZE
>>> #endif
>>>
>>> should go in the shared globalDefinitions.hpp file.
> 
> I intentionally didn't want to expose OM_CACHE_LINE_SIZE in
> globalDefinitions.hpp. I'm trying really hard to stay out of that file.
> Also, is globalDefinitions.hpp processed before or after the CPU/platform
> specific files?

globalDefinitions.hpp directly includes the CPU and toolset versions of 
the globalDefinitions_X.hpp files. So you just need to place this after 
the relevant include site.

> Obviously I need to specify OM_CACHE_LINE_SIZE in the two CPU files:
> 
>      src/hotspot/cpu/sparc/globalDefinitions_sparc.hpp
>      src/hotspot/cpu/x86/globalDefinitions_x86.hpp
> 
> and both halves of the ObjectMonitor subsystem need the above logic:
> 
>      src/hotspot/share/runtime/objectMonitor.hpp
>      src/hotspot/share/runtime/synchronizer.hpp
> 
> but we don't need to expose OM_CACHE_LINE_SIZE elsewhere. Of course,

Okay I buy that you don't want broader exposure. I just don't like that 
you had to duplicate that code in both header files where do you need to 
use OM_CACHE_LINE_SIZE. I would have expected synchronizer.hpp to 
include objectMonitor.hpp but it doesn't, it just forward declares 
"class ObjectMonitor" - otherwise you'd only need this in 
objectMonitor.hpp where it belongs.

Cheers,
David

> if we end up adding CACHE_LINE_SIZE and DOUBLE_CACHE_LINE_SIZE to
> the system *and* if we can get all the CPUs/platforms to agree to use
> CACHE_LINE_SIZE for ObjectMonitors, then OM_CACHE_LINE_SIZE can be
> retired.
> 
> Thanks again for the fast reviews!
> 
> Dan
> 
> 
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>>      JDK-8235931 add OM_CACHE_LINE_SIZE and use smaller size on 
>>>> SPARCv9 and X64
>>>>      https://bugs.openjdk.java.net/browse/JDK-8235931
>>>>
>>>> Here's the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8235931-webrev/0-for-jdk15/
>>>>
>>>> Folks that have reviewed JDK-8153224 will recognize these changes as
>>>> a subset of the cache line changes from the Async Monitor Deflation
>>>> project. It's a subset because there are more fields that need to be
>>>> on separate cache lines in the Async Monitor Deflation project.
>>>>
>>>> These changes are being tested in a Mach5 Tier[1-3] run that's still
>>>> running (JDK14 ATR has priority). I've also run these changes thru
>>>> a set of 10 SPECjbb2015 runs; I'm waiting for my various SPECjbb2015
>>>> runs to finish so I can do the analysis.
>>>>
>>>> Thanks, in advance, for comments, questions or suggestions.
>>>>
>>>> Dan
>