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

Fri Jan 17 21:16:52 UTC 2020

One last reply on this review thread to report my SPECjbb2015 results
for this fix (confusing as they are). I'm continuing to use the
tuned G1 on Linux-X64 server config in Aurora...

With a jdk-14+26 baseline and 10 runs:

     criticalJOPS                -0.40%  (Non-significant)
                    66482.80   66215.90
                   ± 1908.72  ± 1716.12
                              p = 0.746

     maxJOPS                      0.23%  (Non-significant)
                    90725.30   90929.60
                   ± 1914.63  ± 1274.21
                              p = 0.782

With a jdk-14+26 baseline and 15 runs:

     criticalJOPS                 0.87%  (Non-significant)
                    66627.53   67210.33
                   ± 2054.53  ± 1601.67
                              p = 0.394

     maxJOPS                      0.86%  (Non-significant)
                    90491.27   91270.20
                   ± 1581.13  ± 2173.06
                              p = 0.272

With a jdk-14+28 baseline and 20 runs:

     criticalJOPS                 4.63%  (Non-significant)
                    63742.70   66691.75
                  ± 10596.31  ± 1810.80
                              p = 0.234

     maxJOPS                      4.17%  (Non-significant)
                    87047.20   90679.55
                  ± 15988.73  ± 1757.86
                              p = 0.325

All of these results were flagged as "Non-significant" by the perf
testing system. Looks like "p" values are still too high.

I was really hoping that the "p" values would get lower as the
run count increased:

                        10 runs  15 runs  20 runs
         criticalJOPS     0.746    0.394    0.234
              maxJOPS     0.782    0.272    0.325

and for criticalJOPS that was the case, but not for maxJOPS.

I guess the "good news" is that we don't have a significant degradation
with the change from 128 byte cache lines to 64 byte cache lines.

I will likely do one more round of SPECjbb2015 testing with 25 runs
per set...

Dan

On 12/19/19 6:32 PM, Daniel D. Daugherty wrote:
> 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
>>>
>
>