RFR(S/L): 8230184 rename, whitespace, indent and comments changes in preparation for lock free Monitor lists

[Daniel D. Daugherty]

Kim,

Thanks for reviewing this craziness!  More below...


On 8/27/19 9:04 PM, Kim Barrett wrote:
>> On Aug 27, 2019, at 6:29 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
>>
>> Greetings,
>>
>> This is my first set of JDK14 baseline cleanup changes extracted from the
>> Async Monitor Deflation project (JDK-8153224).
>>
>> This is an "S" review because it is conceptually Small. It is also an "L"
>> review because it has a mind numbingly Large number of cleanup changes.
>> Coleen, should appreciate this one since it takes care of a number of
>> naming issues that she (and others) have wanted to see fixed for years.
>>
>> As usual, the bug report has a yacky description of what I'm cleaning up
>> this round and why:
>>
>>     JDK-8230184 rename, whitespace, indent and comments changes in preparation
>>                 for lock free Monitor lists
>>     https://bugs.openjdk.java.net/browse/JDK-8230184
>>
>> Here's the webrev URL:
>>
>>      http://cr.openjdk.java.net/~dcubed/8230184-webrev/0_for_jdk14/
>>
>> This webrev is relative to jdk-14+11.
>>
>> This fix is included in my latest round of JDK-8153224 testing which
>> included Mach5 Tier[1-8], my stress kit on Linux-X64 and Solaris-X64,
>> my Inflate2 test running in parallel with Kitchensink8H on macOSX,
>> Linux-X64 and Solaris-X64. I also ran the Inflate2 and Kitchensink8H
>> combo in parallel on the jdk-14+11 baseline + this fix on macOSX,
>> Linux-X64 and Solaris-X64. No regressions were observed in any of
>> the testing.
>>
>> Thanks, in advance, for any comments, suggestions or questions.
>>
>> Dan
> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/objectMonitor.hpp
> Some variables I expected to be renamed but weren't:
>
>   167   ObjectWaiter* volatile _EntryList;  // Threads blocked on entry or reentry.
>   173   Thread* volatile _Responsible;
>   175   volatile int _Spinner;            // for exit->spinner handoff optimization
>   176   volatile int _SpinDuration;
>   182   ObjectWaiter* volatile _WaitSet;  // LL of threads wait()ing on the monitor
>   185   volatile int _WaitSetLock;        // protects Wait Queue - simple spinlock
>
> Similarly for the block of private functions at the end of the class.
>
> Maybe some of these aren't surviving future work, so not worth
> renaming now?  But then why mess with whitespace and parameter names
> now?  So that's probably not the rationale.

I added the following comments to the bug report:

Update: This isn’t a complete cleanup. I started with the list variable
names and the functions that used them and added those to the
cleanup list. So if there’s a function that didn’t intersect with the
“primaries”, then I didn’t clean it up except for some globals like
“Self” -> “self”.

Perfect example is that ObjectMonitor.cpp isn’t touched, but
ObjectMonitor.hpp is. ObjectMonitor.cpp never touched the list
variables.

> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/synchronizer.cpp
> 1013 static void InduceScavenge(Thread* self, const char * Whence) {
>
> Whence parameter name and type formatting not updated.

That's actually a function that we're planning on deleting when
we get rid of the MonitorBound option. Which reminds me that I
need to file that series of bugs to get rid of this:

$ grep MonitorBound src/hotspot/share/runtime/globals.hpp
   product(intx, MonitorBound, 0, "Bound Monitor 
population")                \

but since it is a 'product' flag, we have to do it in stages...

However, it wasn't touched because it didn't intersect with the
primaries...


> Making sure we're reading carefully? :)

And I'm trying cure insomnia at the same time!! :-)



> ------------------------------------------------------------------------------
>
> Looks good.
>
> I want to say it's trivial, but maybe it's too mind numbingly large.

Thanks! I appreciate the effort that it took to wade through this one.

Dan