RFR (L/XXS) cleanup non-indent white space issues (8046758)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Jun 16 23:37:14 UTC 2014
Dan,
I'm Ok with another automatic + manual fix round.
In fact, it is something that was expected. :)
Thumbs up.
Thanks,
Serguei
On 6/16/14 4:26 PM, Daniel D. Daugherty wrote:
> 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