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

[Claes Redestad]

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