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

[Doerr, Martin]

Hi Dan,

thank you very much for reviewing and for investigating the history.

It was not intended to make the functions you mentioned public. I've fixed that.
I also updated the copyright information.

New webrev is here:
http://cr.openjdk.java.net/~mdoerr/8166970_mutex_padding/webrev.02/

@Coleen: Please use this one. I have also added reviewer attribution.

Thanks and best regards,
Martin


-----Original Message-----
From: Daniel D. Daugherty [mailto:daniel.daugherty at oracle.com] 
Sent: Donnerstag, 6. Oktober 2016 23:13
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 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
>