Claes posted this comment in response to a comment
from David Holmes in theJDK-8235931 review:
On 12/18/19 10:03 AM, Claes Redestad wrote:
> 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
The renaming of the existing DEFAULT_CACHE_LINE_SIZE
CACHE_LINE_SIZE or DOUBLE_CACHE_LINE_SIZE is
beyond the scope of what I was doing withJDK-8235931
so I'm filing this followup RFE.
Since DEFAULT_CACHE_LINE_SIZE is used by several
of the subsystems in HotSpot, it's not really clear where
this bug should be filed. I've started with hotspot/runtime,
but the triage team may move this elsewhere.
from David Holmes in the
On 12/18/19 10:03 AM, Claes Redestad wrote:
> 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 (
>>> 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
The renaming of the existing DEFAULT_CACHE_LINE_SIZE
CACHE_LINE_SIZE or DOUBLE_CACHE_LINE_SIZE is
beyond the scope of what I was doing with
so I'm filing this followup RFE.
Since DEFAULT_CACHE_LINE_SIZE is used by several
of the subsystems in HotSpot, it's not really clear where
this bug should be filed. I've started with hotspot/runtime,
but the triage team may move this elsewhere.
- blocks
-
JDK-8321481 Reconsider default x86_64 padding of double the cache line size
-
- Open
-
-
JDK-8321137 Reconsider ICStub alignment
-
- Resolved
-
- is blocked by
-
JDK-8321269 Require platforms to define DEFAULT_CACHE_LINE_SIZE
-
- Resolved
-
- relates to
-
JDK-8235931 add OM_CACHE_LINE_SIZE and use smaller size on SPARCv9 and X64
-
- Resolved
-
(1 links to)