On 8/10/2019 2:57 pm, [email protected] 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, [email protected] 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
