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

[Daniel D. Daugherty]

Hi David,

Thanks for closing the loop on this review thread.


On 12/19/19 6:11 PM, David Holmes wrote:
> Hi Dan,
>
> All my comments are addressed re motivation and performance. One 
> follow up at the end.

And one reply 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.

That's good to know, for future use.


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

Thanks. I don't like it either...


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

If I remember correctly, there were massive build problems... Don't want
to go down that path either... I'll try that experiment again to refresh
my memory...

Thanks again for the many reviews on this ObjectMonitor stuff...

Dan


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