RFR (S/T): 8231737: Cleanup JvmtiRawMonitor code

[David Holmes]

On 8/10/2019 2:57 pm, serguei.spitsyn at oracle.com wrote:
> Hi David,
> 
> Thank you for making the changes!
> 
> Initially, I did not notice there are several spots with a wrong indent:

Well spotted! I thought I had done a global re-indent - oops. Fixed now.

Thanks,
David

> 124 if (Atomic::replace_if_null(self, &_owner)) {
> 125 return;
>   126     }
>   ...
> 136 if (_owner == NULL && Atomic::replace_if_null(self, &_owner)) {
> 137 _entryList = node._next;
> 138 RawMonitor_lock->unlock();
> 139 return;
>   140     }
> 141 RawMonitor_lock->unlock();
> 142 while (node.tState == QNode::TS_ENTER) {
> 143 self->_ParkEvent->park();
>   144     }
>   ...
> 
>   158   if (w != NULL) {
> 159 _entryList = w->_next;
>   160   }
> 161 RawMonitor_lock->unlock();
>   162   if (w != NULL) {
> 163 guarantee(w ->tState == QNode::TS_ENTER, "invariant");
> 164 // Once we set tState to TS_RUN the waiting thread can complete
> 165 // simpleEnter and 'w' is pointing into random stack space. So we have
>   166       // to ensure we extract the ParkEvent (which is in type-stable memory)
>   167       // before we set the state, and then don't access 'w'.
> 168 ParkEvent* ev = w->_event;
>   169       OrderAccess::loadstore();
> 170 w->tState = QNode::TS_RUN;
> 171 OrderAccess::fence();
> 172 ev->unpark();
>   173   }
>   ...
>   250   for (;;) {
> 251 QNode* w = _waitSet;
> 252 if (w == NULL) break;
> 253 _waitSet = w->_next;
> 254 if (ev != NULL) {
> 255 ev->unpark();
> 256 ev = NULL;
> 257 }
> 258 ev = w->_event;
> 259 OrderAccess::loadstore();
> 260 w->tState = QNode::TS_RUN;
>   261       OrderAccess::storeload();
> 262 if (!all) {
> 263 break;
> 264 }
> 265 } ... 294 if (contended == self) {
> 295 _recursions++;
> 296 return;
>   297   }
>   298
> 299 if (contended == NULL) {
> 300 guarantee(_owner == self, "invariant");
> 301 guarantee(_recursions == 0, "invariant");
> 302 return;
>   303   }
>   ...
> 307 if (!self->is_Java_thread()) {
> 308 simpleEnter(self);
>   309   } else {
> 
> 
> Thanks,
> Serguei
> 
> 
> On 10/7/19 21:42, David Holmes wrote:
>> Hi Serguei,
>>
>> On 8/10/2019 2:18 pm, serguei.spitsyn at oracle.com wrote:
>>> Hi David,
>>>
>>> Looks good in general.
>>
>> Thanks for looking at it.
>>
>>> A couple of places still need a cleanup.
>>>
>>> http://cr.openjdk.java.net/~dholmes/8231737/webrev/src/hotspot/share/prims/jvmtiRawMonitor.hpp.frames.html 
>>>
>>>
>>>
>>> QNode pointer is not unified:
>>>
>>> 47 QNode * volatile _next;
>>> 48 QNode * volatile _prev;
>>> 68 void simpleEnter(Thread * self) ;
>>> 69 void simpleExit(Thread * self) ;
>>> but:53 QNode(Thread* thread); 56 Thread* volatile _owner; // pointer 
>>> to owning thread 58 QNode* volatile _entryList; // Threads blocked on 
>>> entry or reentry.
>>>     61 QNode* volatile _waitSet; // Threads wait()ing on the monitor 
>>> There are more places where a space is placed or not between the type 
>>> name and the '*'.
>>
>> Okay I have switched everything to the:
>>
>> type* var
>>
>> style. It seems most prevelant based on a grep for "Thread* t" type 
>> usage.
>>
>>> Missed space before ';':
>>>
>>> 68 void simpleEnter(Thread * self) ;
>>> 69 void simpleExit(Thread * self) ;
>>> 70 int simpleWait(Thread * self, jlong millis) ;
>>> 71 void simpleNotify(Thread * self, bool all) ;
>>
>> Fixed.
>>
>>> http://cr.openjdk.java.net/~dholmes/8231737/webrev/src/hotspot/share/prims/jvmtiRawMonitor.cpp.frames.html 
>>>
>>>
>>> Space is not needed after the cast: 280 jt = (JavaThread*) self; 370 
>>> jt = (JavaThread*) self;
>>>
>>> Other places do not have it:
>>>
>>> 150 OrderAccess::release_store(&_owner, (Thread*)NULL); 288 contended 
>>> = Atomic::cmpxchg(jt, &_owner, (Thread*)NULL); 291 contended = 
>>> Atomic::cmpxchg(self, &_owner, (Thread*)NULL);
>>
>> Fixed.
>>
>> webrev updated in place.
>>
>> Thanks,
>> David
>> -----
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> On 10/7/19 18:58, David Holmes wrote:
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8231737
>>>> webrev: http://cr.openjdk.java.net/~dholmes/8231737/webrev/
>>>>
>>>> Stylistic code cleanup of JvmtiRawMonitor code as previously promised:
>>>> - Self -> self
>>>> - SimpleX -> simpleX
>>>> - Contended -> contended
>>>> - Node -> node
>>>> - TState -> tState (variable name)
>>>> - _WaitSet -> _waitSet
>>>> - _EntryList -> _entryList;
>>>> - All -> all
>>>> - remove extra space before ( in function calls
>>>> - remove extra space before ;
>>>> - remove extra space before ++ and --
>>>> - add spaces around binary operators
>>>> - use { } on all blocks
>>>> - use one statement per line.
>>>> - fix indent and alignment (ie remove artificial alignment)
>>>>
>>>> Probably simplest to look at new code and see if it looks okay 
>>>> rather than trying to spot each individual change. :)
>>>>
>>>> Thanks,
>>>> David
>>>
>