RFR (L/XXS) cleanup non-indent white space issues (8046758)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Jun 16 23:39:47 UTC 2014
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