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

[Daniel D. Daugherty]

I was going to bring up PaddedEnd, but decided not to since the example
closest to my fingertips is what we did with ObjectMonitor and PaddedEnd...
I didn't think you liked that one either... maybe I'm just confused... :-)

Dan


On 10/6/16 3:51 PM, Claes Redestad wrote:
> Hi Dan,
>
> yes, I'm slighly unhappy with this change... :-)
>
> ... and would rather see a reuse of PaddedEnd<[Monitor|Mutex]> from
> share/vm/memory/padded.hpp in the places where padding makes sense,
> such as the globally allocated lists, rather than perpetuating the wart
> of dual-purposing the name field for padding.
>
> This is sort of like what you're already suggesting, except that
> PaddedEnd uses template magic to actually add nothing if we're already
> cache aligned, as well as allowing us to not add any footprint overhead
> to existing uses where Monitors and Mutexes are already embedded (and
> there are a number of existing uses in key places in both GC and
> compiler code, see, e.g., CompileTask).
>
> Thanks!
>
> /Claes
>
> On 2016-10-06 23:13, Daniel D. Daugherty wrote:
>> On 9/30/16 9: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/
>>
>> src/share/vm/runtime/mutex.hpp
>>      Please update the copyright year before pushing.
>>
>>      L172:   // The default length of monitor name is chosen to avoid
>> false sharing.
>>      L173:   enum {
>>      L174:     CACHE_LINE_PADDING = DEFAULT_CACHE_LINE_SIZE -
>> sizeof(MonitorBase),
>>      L175:     MONITOR_NAME_LEN = CACHE_LINE_PADDING > 64 ?
>> CACHE_LINE_PADDING : 64
>>      L176:   };
>>      L177:   char _name[MONITOR_NAME_LEN];          // Name of mutex
>>
>>          I have to say that I'm not fond of the fact that 
>> MONITOR_NAME_LEN
>>          can vary between platforms; I like that it is a minimum of 64
>> bytes
>>          and is still a constant.
>>
>>          I'm also not happy that the resulting sizeof(Monitor) may not
>> be a multiple
>>          of the DEFAULT_CACHE_LINE_SIZE. However, I have to mitigate
>> that unhappiness
>>          with the fact that sizeof(Monitor) hasn't been a multiple of
>> the cache line
>>          size since at least 2008 and no one complained (that I know 
>> of).
>>
>>          So if I was making this change, I would make MONITOR_NAME_LEN
>> 64 bytes
>>          (like it was) and add a pad field that would bring up
>> sizeof(Monitor)
>>          to be a multiple of DEFAULT_CACHE_LINE_SIZE. Of course, Claes
>> would be
>>          unhappy with me and anyone embedding a Monitor into another 
>> data
>>          structure would be unhappy with me, but I'm used to that :-)
>>
>>          So what you have is fine, especially for JDK9.
>>
>>      L180:  public:
>>      L181: #ifndef PRODUCT
>>      L182:   debug_only(static bool contains(Monitor * locks, Monitor *
>> lock);)
>>      L183:   debug_only(static Monitor * get_least_ranked_lock(Monitor *
>> locks);)
>>      L184:   debug_only(Monitor *
>> get_least_ranked_lock_besides_this(Monitor * locks);)
>>      L185: #endif
>>      L186:
>>      L187:   void set_owner_implementation(Thread*
>> owner)                        PRODUCT_RETURN;
>>      L188:   void check_prelock_state     (Thread*
>> thread)                       PRODUCT_RETURN;
>>      L189:   void check_block_state       (Thread* thread)
>>
>>          These were all "protected" before. Now they are "public".
>>          Any particular reason?
>>
>> Thumbs up on the mechanics of this change. I'm interested in the
>> answer to the "protected" versus "public" question, but don't
>> considered that query to be a blocker.
>>
>>
>> The rest of this isn't code review, but some of this caught
>> my attention.
>>
>> src/share/vm/runtime/mutex.hpp
>>
>>      old L84: // The default length of monitor name is chosen to be 64
>> to avoid false sharing.
>>      old L85: static const int MONITOR_NAME_LEN = 64;
>>
>> I had to look up the history of this comment:
>>
>> $ hg log -r 55 src/share/vm/runtime/mutex.hpp
>> changeset:   55:2a8eb116ebbe
>> user:        xlu
>> date:        Tue Feb 05 23:21:57 2008 -0800
>> summary:     6610420: Debug VM crashes during monitor lock rank checking
>>
>> $ hg diff -r5{4,5} src/share/vm/runtime/mutex.hpp
>> diff -r d4a0f561287a -r 2a8eb116ebbe src/share/vm/runtime/mutex.hpp
>> --- a/src/share/vm/runtime/mutex.hpp    Thu Jan 31 14:56:50 2008 -0500
>> +++ b/src/share/vm/runtime/mutex.hpp    Tue Feb 05 23:21:57 2008 -0800
>> @@ -82,6 +82,9 @@ class ParkEvent ;
>>   // *in that order*.  If their implementations change such that these
>>   // assumptions are violated, a whole lot of code will break.
>>
>> +// The default length of monitor name is choosen to be 64 to avoid
>> false sharing.
>> +static const int MONITOR_NAME_LEN = 64;
>> +
>>   class Monitor : public CHeapObj {
>>
>>    public:
>> @@ -126,9 +129,8 @@ class Monitor : public CHeapObj {
>>     volatile intptr_t _WaitLock [1] ;      // Protects _WaitSet
>>     ParkEvent * volatile  _WaitSet ;       // LL of ParkEvents
>>     volatile bool     _snuck;              // Used for sneaky locking
>> (evil).
>> -  const char * _name;                    // Name of mutex
>>     int NotifyCount ;                      // diagnostic assist
>> -  double pad [8] ;                       // avoid false sharing
>> +  char _name[MONITOR_NAME_LEN];          // Name of mutex
>>
>>     // Debugging fields for naming, deadlock detection, etc. (some only
>> used in debug mode)
>>   #ifndef PRODUCT
>> @@ -170,7 +172,7 @@ class Monitor : public CHeapObj {
>>      int  ILocked () ;
>>
>>    protected:
>> -   static void ClearMonitor (Monitor * m) ;
>> +   static void ClearMonitor (Monitor * m, const char* name = NULL) ;
>>      Monitor() ;
>>
>> So the original code had an 8-double pad for avoiding false sharing.
>> Sounds very much like the old ObjectMonitor padding. I'm sure at the
>> time that Dice determined that 8-double value, the result was to pad
>> the size of Monitor to an even multiple of a particular cache line
>> size.
>>
>> Xiobin changed the 'name' field to be an array so that the name
>> chars could serve double duty as the cache line pad... pun intended.
>> Unfortunately that pad doesn't make sure that the resulting Monitor
>> size is a multiple of the cache line size.
>>
>> Dan
>>
>>
>>>
>>> Please review. If will also need a sponsor.
>>>
>>> Thanks and best regards,
>>> Martin
>>>
>>