RFR (L/XXS) cleanup non-indent white space issues (8046758)
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jun 17 17:14:14 UTC 2014
Dan,
This change is fine. Thank you for doing the cleanups first and
separating out these changes from the incoming content changes.
Coleen
On 6/16/14, 7:39 PM, Daniel D. Daugherty wrote:
> Thanks Serguei!
>
> It is possible that there will be a bug_fix and cleanup round
> before each of Contended Locking project buckets... sigh...
> There's a lot of code in motion right now...
>
> Dan
>
>
> On 6/16/14 5:37 PM, serguei.spitsyn at oracle.com wrote:
>> 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