RFR(M): 8166970: Adapt mutex padding according to DEFAULT_CACHE_LINE_SIZE

[Claes Redestad]

Hi,

(cc:ing hotspot-gc-dev)

On 2016-10-06 15:33, David Holmes wrote:
> Hi Martin,
>
> Thanks for bearing with me here, these optimizations are not really my
> forte.
>
> On 6/10/2016 9:05 PM, Doerr, Martin wrote:
>> Hi David,
>>
>> there are many Monitor instances behind each other so I think the idea
>> of padding (which was not mine) was not bad in general.
>> The ideal situation would be to have them cache line aligned and
>> sizeof(Monitor) equals the cache line size (or a multiple). This would
>> completely prevent cache line sharing.
>
> So this is for all the mutexes/monitors created in mutex_init() which
> are assumed to be laid out in a linear fashion. Ok.
>
> Has anyone actually done any metrics on this or is it all theoretical?
> ie are any adjacent, or otherwise cache-line-aligned, monitors actually
> contended at the same time? Padding to avoid false-sharing always seems
> a very local optimization to me - more obvious with hot fields in the
> same object than with distinct fields in distinct objects.
>
>> Even without having the cache line alignment, the padding does help:
>> Please note that the padding is inserted at the end. The critical
>> fields are at the beginning.
>> Especially _LockWord of 2 Monitors will never be on the same cache
>> line when sizeof(Monitor) equals the cache line size (or a multiple).
>
> Seems to me the existing code, as it doesn't take into account the size
> of the rest of the Monitor, isn't really addressing this correctly at
> all - even on platforms with a 64-byte cache line.
>
>> Padding = DEFAULT_CACHE_LINE_SIZE could prevent more sharing in case
>> of bad alignment, but I didn't want to waste more space. I'd rather
>> prefer the alignment solution.
>
> The other option is an operator new that only allocates on the desired
> alignment - as we do in some other places. That also avoids wasted space
> with Monitors embedded in other objects - not that I think we have that
> many of them.

IIRC GC code has a number of places where Monitors are created and
embedded in other, larger objects and have reported footprint overhead
issues with the current anti-sharing solution.

Additionally, if memory serves me, it appears this char[64] name field
is only ever set to an actual name for the Monitors that are allocated
globally in mutex_list, so...

... wouldn't a possibly better solution be to remove padding altogether
from the base Monitor and wrap the mutex_list Monitors in some class 
that adds the name/padding?

Thanks!

/Claes

>
> Thanks,
> David
>
>> Best regards,
>> Martin
>>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Donnerstag, 6. Oktober 2016 12:21
>> To: Doerr, Martin <martin.doerr at sap.com>;
>> hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(M): 8166970: Adapt mutex padding according to
>> DEFAULT_CACHE_LINE_SIZE
>>
>> On 6/10/2016 7:09 PM, Doerr, Martin wrote:
>>> Hi David,
>>>
>>> thanks for taking a look at my proposal.
>>>
>>> Maybe "unnecessary cache line sharing of contended memory" would be
>>> more comprehensive.
>>>
>>> The purpose of the padding is to avoid the following situation:
>>> 2 Monitor instances are located behind each other and some fields end
>>> up on the same cache line.
>>> Some threads running on some processors compete for the first Monitor
>>> while some other threads running on some processors compete for the
>>> second one.
>>> The cache line needs to get transferred between all involved processors.
>>>
>>> If we add enough padding, the fields which are accessed by many
>>> processors end up on different cache lines. This splits the problem
>>> into 2 independent problems. The threads competing for the first
>>> Monitor don't interfere with those ones competing for the second one
>>> any more.
>>
>> But that only helps for the case where the two monitors are exactly the
>> wrong distance apart. Two other monitors that previously did not share
>> cache lines may now do so if you make the monitors bigger.
>>
>> This seems completely ad-hoc. ??
>>
>> David
>> -----
>>
>>
>>
>>> The existing padding implementation is not optimal. It's a little too
>>> small on some platforms. On other platforms, it is not wrong to pad
>>> more than necessary, but ideally, one would pad to make the Monitor
>>> size equal to the cache line size. I have kept the minimum of 64
>>> because _name is not only used for padding and I guess people don't
>>> want it too short.
>>>
>>> x86_64 and SPARC have DEFAULT_CACHE_LINE_SIZE=128 in typical
>>> configurations. That's like PPC64 so the change also improves the
>>> padding on these platforms as well.
>>> (On x86_64 we get the same result as on PPC64: The length of _name
>>> gets extended from 64 to 72 in product build). The padding increase
>>> only gets huge on S390.
>>>
>>> Best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>> Sent: Donnerstag, 6. Oktober 2016 04:31
>>> To: Doerr, Martin <martin.doerr at sap.com>;
>>> hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: RFR(M): 8166970: Adapt mutex padding according to
>>> DEFAULT_CACHE_LINE_SIZE
>>>
>>> On 1/10/2016 1:48 AM, Doerr, Martin wrote:
>>>> Hi,
>>>>
>>>> the current implementation of Monitor padding (mutex.cpp) assumes
>>>> that cache lines are 64 Bytes. There's a platform dependent define
>>>> "DEFAULT_CACHE_LINE_SIZE" available which can be used. Purpose of
>>>> padding is to avoid false sharing.
>>>>
>>>> My proposed change is here:
>>>> http://cr.openjdk.java.net/~mdoerr/8166970_mutex_padding/webrev.00/
>>>
>>> Not sure I understand the existing padding code. What false sharing are
>>> we trying to avoid?
>>>
>>> And if the existing code assumes a cache line size of 64 and declares
>>> _name to be 64 chars, then why can't the new code declare name to be
>>> DEFAULT_CACHE_LINE_SIZE chars? This suggests the existing padding code
>>> is wrong (not just hard-wired).
>>>
>>> Which platforms will this cause an actual change in Monitor size other
>>> than S390?
>>>
>>> Thanks,
>>> David
>>>
>>>> Please review. If will also need a sponsor.
>>>>
>>>> Thanks and best regards,
>>>> Martin
>>>>