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

[Claes Redestad]

Hi,

On 2019-12-18 06:50, David Holmes wrote:
> Hi Dan,
> 
> On 18/12/2019 7:34 am, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I'm extracting another standalone fix from the Async Monitor Deflation
>> project (JDK-8153224) and sending it out for review (and testing)
>> separately. This is an XXS sized fix, but since it involves cache
>> line sizes I don't think a trivial review is appropriate.
> 
> Can you elaborate on why a smaller cache-line-size assumption is 
> beneficial here? Do you have performance numbers for the bug report?

I did numerous experiments on this during work on JEP-143 and saw no
improvement on neither "real" benchmarks nor stress tests that try to
explicitly provoke cross-monitor false sharing on various fields in
ObjectMonitors. I've not re-run those experiments on new hardware, so
YMMV. Should probably check if they're still usable..

Specifically, for padding values below 64 we could provoke false sharing
effects on stress tests, with negligible movement on "real" benchmarks.
For values above 64 we just waste space.

There exists a legend that you need double the physical cache line
padding to not provoke the wrath of false sharing in the presence of
aggressive prefetching, which is the reason why the
"DEFAULT_CACHE_LINE_SIZE" is set to twice the typical physical cache
line size in bytes. If we explicitly think we need to pad two lines,
then the constant DEFAULT_CACHE_LINE_SIZE should be named differently.

The patch in question looks good to me, although I'd prefer something
like:

CACHE_LINE_SIZE 64
DOUBLE_CACHE_LINE_SIZE 128

.. and then use whichever is deemed more appropriate on a case by case
basis. (Hint: it's probably CACHE_LINE_SIZE)

$.02

/Claes


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