RFR (L/XXS) cleanup non-indent white space issues (8046758)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Jun 17 17:20:14 UTC 2014
On 6/17/14 11:14 AM, Coleen Phillimore wrote:
>
> Dan,
>
> This change is fine.
Thanks for the review!
> Thank you for doing the cleanups first and separating out these
> changes from the incoming content changes.
No problem. Originally all the cleanups were in one bucket, but
it just seemed way too big...
Dan
>
> 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