RFR (L/XXS) cleanup non-indent white space issues (8046758)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Jun 16 23:26:43 UTC 2014
Thanks for the fast review!
On 6/16/14 4:16 PM, serguei.spitsyn at oracle.com wrote:
> I Dan,
>
> You are crazy enough to fix these! ;)
Yes I am, but only one (large) chunk at a time...
> No need to re-review if you decide to fix the above.
For your comments about the incorrect indents, I'm going to
requote this (in particular the last sentence):
> 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.
I don't have an automated fix for the indent problem yet, but I'm working
on it. Because the Contended Locking project is being done in phases, I
need to have script do the work every time I rebase all the buckets.
For the XXX is no longer aligned comments, I'm going to have to make a
manual pass for such things, but that's not what this webrev is for.
This webrev is for the things that can be automatically fixed by my
do_space_filter.ksh script; no manual changes in this round.
Please let me know if you're OK with these resolutions.
More replies embedded below...
>
> 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); }
Aligning the closing brackets will away in the manual fixes stage.
> 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
During the reorder fields and cache line alignment bucket, a bunch of the
fields will move around and I'll have to make a manual pass to align the
comments consistently. Of course, the same comment indent is not used
consistently throughout the entire class.
> 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);
These will get fixed automatically when I get the next tool
working...
> 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)
The backslashes will be fixed when I do a manual pass.
> Unaligned comments:
> 983 OrderAccess::release_store_ptr(&_owner, NULL); // drop the lock
> 984 OrderAccess::storeload(); // See if we need to wake a successor
This will be fixed when I do a manual pass.
> Extra spaces before the comma:
> 1092 assert(w != NULL , "invariant");
> 1131 assert(w != NULL , "invariant");
Fixing interior parameter and comma spacing will have to be done
manually. The script can handle patterns that are well anchored
at the beginning and end of a line, but the interior stuff is
much more difficult to get right without unintended side effects.
> 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 }
These will get fixed automatically when I get the next tool
working...
> 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
This will get fixed during a manual pass.
> 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 }
These will get fixed automatically when I get the next tool
working...
>
> 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