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

[serguei.spit...@oracle.com]

Hi David, Thank you for making the changes! Initially, I did not notice there are several spots with a wrong indent: 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.spit...@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