RFR (L/XXS) cleanup non-indent white space issues (8046758)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Jun 16 22:16:21 UTC 2014
I Dan,
You are crazy enough to fix these! ;)
It looks good.
Some nits...
src/share/vm/runtime/objectMonitor.hpp
The style in this file is to align the closing brackets:
static int recursions_offset_in_bytes() { return offset_of(ObjectMonitor, _recursions); }
- static int cxq_offset_in_bytes() { return offset_of(ObjectMonitor, _cxq) ; }
- static int succ_offset_in_bytes() { return offset_of(ObjectMonitor, _succ) ; }
+ static int cxq_offset_in_bytes() { return offset_of(ObjectMonitor, _cxq); }
+ static int succ_offset_in_bytes() { return offset_of(ObjectMonitor, _succ); }
static int EntryList_offset_in_bytes() { return offset_of(ObjectMonitor, _EntryList); }
The style in this file is to align the comments:
volatile intptr_t _recursions; // recursion count, 0 for first entry
private:
- int OwnerIsThread ; // _owner is (Thread *) vs SP/BasicLock
- ObjectWaiter * volatile _cxq ; // LL of recently-arrived threads blocked on entry.
+ int OwnerIsThread; // _owner is (Thread *) vs SP/BasicLock
+ ObjectWaiter * volatile _cxq; // LL of recently-arrived threads blocked on entry.
// The list is actually composed of WaitNodes, acting
// as proxies for Threads.
protected:
- ObjectWaiter * volatile _EntryList ; // Threads blocked on entry or reentry.
+ ObjectWaiter * volatile _EntryList; // Threads blocked on entry or reentry.
private:
- Thread * volatile _succ ; // Heir presumptive thread - used for futile wakeup throttling
- Thread * volatile _Responsible ;
- int _PromptDrain ; // rqst to drain cxq into EntryList ASAP
+ Thread * volatile _succ; // Heir presumptive thread - used for futile wakeup throttling
+ Thread * volatile _Responsible;
+ int _PromptDrain; // rqst to drain cxq into EntryList ASAP
- volatile int _Spinner ; // for exit->spinner handoff optimization
- volatile int _SpinFreq ; // Spin 1-out-of-N attempts: success rate
- volatile int _SpinClock ;
- volatile int _SpinDuration ;
- volatile intptr_t _SpinState ; // MCS/CLH list of spinners
+ volatile int _Spinner; // for exit->spinner handoff optimization
+ volatile int _SpinFreq; // Spin 1-out-of-N attempts: success rate
+ volatile int _SpinClock;
+ volatile int _SpinDuration;
+ volatile intptr_t _SpinState; // MCS/CLH list of spinners
src/share/vm/runtime/objectMonitor.cpp
It is surprising that there are so many fragments with incorrect indent
(3 or 4) in the loops and conditions!
Some examples are:
322 // we forgo posting JVMTI events and firing DTRACE probes.
323 if (Knob_SpinEarly && TrySpin (Self) > 0) {
324 assert(_owner == Self , "invariant");
325 assert(_recursions == 0 , "invariant");
326 assert(((oop)(object()))->mark() == markOopDesc::encode(this), "invariant");
327 Self->_Stalled = 0;
328 return;
329 }
521 for (;;) {
522 node._next = nxt = _cxq;
523 if (Atomic::cmpxchg_ptr(&node, &_cxq, nxt) == nxt) break;
524
525 // Interference - the CAS failed because _cxq changed. Just retry.
526 // As an optional optimization we retry the lock.
527 if (TryLock (Self) > 0) {
528 assert(_succ != Self , "invariant");
529 assert(_owner == Self , "invariant");
530 assert(_Responsible != Self , "invariant");
531 return;
532 }
533 }
584 if ((SyncFlags & 2) && _Responsible == NULL) {
585 Atomic::cmpxchg_ptr(Self, &_Responsible, NULL);
586 }
587
588 // park self
589 if (_Responsible == Self || (SyncFlags & 1)) {
590 TEVENT(Inflated enter - park TIMED);
591 Self->_ParkEvent->park((jlong) RecheckInterval);
592 // Increase the RecheckInterval, but clamp the value.
593 RecheckInterval *= 8;
594 if (RecheckInterval > 1000) RecheckInterval = 1000;
595 } else {
596 TEVENT(Inflated enter - park UNTIMED);
597 Self->_ParkEvent->park();
598 }
627 if ((Knob_ResetEvent & 1) && Self->_ParkEvent->fired()) {
628 Self->_ParkEvent->reset();
629 OrderAccess::fence();
630 }
The '_recursions' is still unaligned:
379 // thread that suspended us.
380 //
381 _recursions = 0;
382 _succ = NULL;
383 exit(false, Self);
The back slashes are supposed to be aligned:
1381 #define CHECK_OWNER() \
1382 do { \
1383 if (THREAD != _owner) { \
1384 if (THREAD->is_lock_owned((address) _owner)) { \
1385 _owner = THREAD; /* Convert from basiclock addr to Thread addr */ \
1386 _recursions = 0; \
1387 OwnerIsThread = 1; \
1388 } else { \
1389 TEVENT(Throw IMSX); \
1390 THROW(vmSymbols::java_lang_IllegalMonitorStateException()); \
1391 } \
1392 } \
1393 } while (false)
Unaligned comments:
983 OrderAccess::release_store_ptr(&_owner, NULL); // drop the lock
984 OrderAccess::storeload(); // See if we need to wake a successor
Extra spaces before the comma:
1092 assert(w != NULL , "invariant");
1131 assert(w != NULL , "invariant");
src/share/vm/runtime/synchronizer.cpp
There are so many fragments with incorrect indent (3 or 4) in the loops
and conditions.
Some examples:
163 if (dhw == NULL) {
164 // Recursive stack-lock.
165 // Diagnostics -- Could be: stack-locked, inflating, inflated.
166 mark = object->mark();
167 assert(!mark->is_neutral(), "invariant");
168 if (mark->has_locker() && mark != markOopDesc::INFLATING()) {
169 assert(THREAD->is_lock_owned((address)mark->locker()), "invariant");
170 }
171 if (mark->has_monitor()) {
172 ObjectMonitor * m = mark->monitor();
173 assert(((oop)(m->object()))->mark() == mark, "invariant");
174 assert(m->is_entered(THREAD), "invariant");
175 }
176 return;
177 }
428 struct SharedGlobals {
429 // These are highly shared mostly-read variables.
430 // To avoid false-sharing they need to be the sole occupants of a $ line.
431 double padPrefix[8];
432 volatile int stwRandom;
433 volatile int stwCycle;
434
435 // Hot RW variables -- Sequester to avoid false-sharing
436 double padSuffix[16];
437 volatile int hcSequence;
438 double padFinal[8];
439 };
498 if ((YieldThenBlock++) >= 16) {
499 Thread::current()->_ParkEvent->park(1);
500 } else {
501 os::NakedYield();
502 }
963 for (int i = Self->omFreeProvision; --i >= 0 && gFreeList != NULL;) {
964 MonitorFreeCount--;
965 ObjectMonitor * take = gFreeList;
966 gFreeList = take->FreeNext;
967 guarantee(take->object() == NULL, "invariant");
968 guarantee(!take->is_busy(), "invariant");
969 take->Recycle();
970 omRelease(Self, take, false);
971 }
1329 if (Atomic::cmpxchg_ptr (markOopDesc::encode(m), object->mark_addr(), mark) != mark) {
1330 m->set_object(NULL);
1331 m->set_owner(NULL);
1332 m->OwnerIsThread = 0;
1333 m->Recycle();
1334 omRelease(Self, m, true);
1335 m = NULL;
1336 continue;
1337 // interference - the markword changed - just retry.
1338 // The state-transitions are one-way, so there's no chance of
1339 // live-lock -- "Inflated" is an absorbing state.
1340 }
Unaligned comments:
129 static volatile intptr_t ListLock = 0; // protects global monitor free-list cache
130 static volatile int MonitorFreeCount = 0; // # on gFreeList
131 static volatile int MonitorPopulation = 0; // # Extant -- in circulation
src/share/vm/runtime/thread.cpp
Incorrect indent (also, see: 4462, 4468-4500) :
4369 if (Atomic::cmpxchg (1, adr, 0) == 0) {
4370 return; // normal fast-path return
4371 }
4372
4373 // Slow-path : We've encountered contention -- Spin/Yield/Block strategy.
4374 TEVENT(SpinAcquire - ctx);
4375 int ctr = 0;
4376 int Yields = 0;
4377 for (;;) {
4378 while (*adr != 0) {
4379 ++ctr;
4380 if ((ctr & 0xFFF) == 0 || !os::is_MP()) {
4381 if (Yields > 5) {
4382 os::naked_short_sleep(1);
4383 } else {
4384 os::NakedYield();
4385 ++Yields;
4386 }
4387 } else {
4388 SpinPause();
4389 }
4390 }
4391 if (Atomic::cmpxchg(1, adr, 0) == 0) return;
4392 }
No need to re-review if you decide to fix the above.
Thanks,
Serguei
On 6/16/14 10:45 AM, Daniel D. Daugherty wrote:
> Greetings,
>
> I have the fix for the following bug ready for JDK9 RT_Baseline:
>
> JDK-8046758 cleanup non-indent white space issues prior to Contended
> Locking cleanup bucket
> https://bugs.openjdk.java.net/browse/JDK-8046758
>
> This is both a (L)arge and e(X)tra e(X)tra (S)mall review! Here is the
> URL for the (L)arge webrev:
>
> http://cr.openjdk.java.net/~dcubed/8046758-webrev/0-jdk9-hs-rt/
>
> Since these are white space fixes, I recommend the udiff links instead
> of the frames links. Of course, some folks prefer the patch file itself
> for white space fixes.
>
> Here is the URL for the e(X)tra e(X)tra (S)mall webrev which was
> generated
> with a version of webrev that ignores all whitespace changes:
>
> http://cr.openjdk.java.net/~dcubed/8046758-webrev/0-jdk9-hs-rt-no_ws/
>
> Again, I recommend the udiff links since that's the fastest way to see
> that webrev_no_ws found no non-white space changes. The patch file for
> this webrev shows all the white space changes.
>
> These non-indent white space cleanups were done in preparation for the
> cleanup bucket from the Contended Locking project; these changes were
> made
> by my do_space_filter.ksh script. The script does not fix everything in
> these files that is not in sync with HotSpot style, but it does fix a
> large portion of them. In particular, the do_space_filter.ksh script does
> _not_ fix incorrect indenting; I'm mulling on a different solution for
> that problem.
>
> Thanks, in advance, for any comments, questions or suggestions.
>
> At the end of the review cycle for this fix, I will attach the version of
> the do_space_filter.ksh script used to do the work to JDK-8046758.
>
> Dan
More information about the hotspot-runtime-dev
mailing list