RFR (S) cleanup misc issues prior to Contended Locking reorder and cache line bucket (8047104)

[Daniel D. Daugherty]

Greetings,

I realized that I forgot to do proper attribution for the these code
changes. So in the credit where credit is due department, here's the
Summary blurb from my Contended Locking Improvements 1-Pager:

 > The purpose of this project is the productization of bug fixes and
 > contended Java Monitor performance improvements developed by Dave
 > Dice of Oracle Labs. This project is based on the port done by
 > Karen Kinnear back in late 2010.

My apologies for the omission.


David,

Thanks for the review. Replies embedded below.

Just to be clear. I'm the shepherd for these changes and only a small
part of the work is mine so it will take me longer than usual to chase
down information in order to respond to review questions. So I'm not
ignoring the queries, I'm just herding sheep. I guess I could be
herding cats... :-)



On 6/19/14 8:52 PM, David Holmes wrote:
> Hi Dan,
>
> Generally looks fine.

Thanks!


> A few nits and queries and one more significant issue, which happens 
> to be first:
>
> src/share/vm/runtime/atomic.hpp
>
> +   // add* provide full bidirectional fence, i.e. 
> storeload/read-modify-write/storeload
>
> That's not a "full bi-directional fence". A fence is: 
> loadLoad|loadStore|storeLoad|storeStore - as per orderAccess.hpp. The 
> cmpxchg wording:
>
> "Guarantees a two-way memory barrier across the cmpxchg.  I.e., it's 
> really a 'fence_cmpxchg_acquire'."
>
> was accurate in terms of required behaviour for cmpxchg. Whether the 
> fence and acquire needed to be explicit depends on the platform.
>
> It is also not clear to me that other than cmpxchg* those atomic ops 
> need "full bi-directional fences", or whether the current 
> implementations actually provide them.

This item will take some research to resolve so I'll have to reply
again on this one later.


> ---
>
> src/share/vm/runtime/globals.hpp
>
> I wouldn't have classified the WorkAroundNPTLTimedWaitHang as unstable 
> even though flagged as such. But this will be gone "soon" anyway.

Since it was described as "unstable", I made it experimental. I'm
planning to leave it as experimental until the other changeset
removes it. Are you OK with that?


> ---
>
> src/share/vm/runtime/objectMonitor.cpp
>
> Wondering if "Need to inhibit inlining for older versions of GCC to 
> avoid build-time failures" is actually still relevant? Something for a 
> later set of changes.

Good question. I'll chase down the history on that one and try
to narrow down the GCC era/version where this used to happen.
Will probably result in a new follow up bug to propose removal
of the inhibit inlining stuff.


> ---
>
> src/share/vm/runtime/synchronizer.cpp
>
> 579       // Handle for oop obj in case of STW safepoint
>
> STW is redundant - all safepoints are STW.

Yes, currently, but folks keep talking about adding per-thread
safepoints. I see that 'STW <this-and-that>' seems to be a common
comment in the monitor subsystem. I'll chase down what Dave D is
really trying to communicate here and see what I can do to clarify
the comments.


> 896 // Too slow for general assert or debug
> 897 #if 0
>
> I thought the trend these days was to delete unused code?

Yes, I've been observing that trend and I find it "interesting".
This particular code is useful when debugging experimental
optimizations in the monitor subsystem, but there might be a
better way to keep the code around. I'll investigate.


> -----
>
> src/share/vm/runtime/thread.cpp
>
> 4402   // That is, all the LDs and STs within the critical section must
> 4403   // be globally visible before the ST that releases the lock 
> becomes visible.
>
> Nit: LDs don't become visible.

I wonder if Dave D was trying to say that the "LDs" need to see
the changes made by the "ST". I'll get clarification.

Side note: I've read that note every time that I've rebased this
project on a new baseline (~6 times or so) and I've breezed right
past it. Always useful to have a new pair eyes look at stuff.


> 4404   // Conceptually we need a #storeload|#storestore "release" 
> MEMBAR before
> 4405   // the ST of 0 into the lock-word which releases the lock, so 
> fence
> 4406   // more than covers this on all platforms.
>
> I think that should be loadStore|storeStore. In which case the fence() 
> is completely overkill as we only need a release_store(addr, 0). ??

This item will also take some research  to resolve so I'll have to reply
again on this one later.


> 4606     // The CAS() also ratifies the previously fetched lock-word 
> value.
>
> Don't know what this means. What is the previously fetched lock-word 
> value ??

4594 void Thread::muxRelease (volatile intptr_t * Lock)  {
4595   for (;;) {
4596     const intptr_t w = Atomic::cmpxchg_ptr(0, Lock, LOCKBIT);

<snip>

4605     // The following CAS() releases the lock and pops the head element.
4606     // The CAS() also ratifies the previously fetched lock-word value.
4607     if (Atomic::cmpxchg_ptr (intptr_t(nxt), Lock, w) != w) {
4608       continue;
4609     }

So the lock-word is first fetched on line 4596. When we
release the lock and pop the head element on line 4607,
if the cmpxchg_ptr() return doesn't match 'w', then we
loop around try it all again. Basically, it's a check
to see if the list was modified between line 4596 and
line 4607 and if the list was modified we have to try
again or we'll lose the element that was added to the
list while we were racing.

Dan



>
> ---
>
> Thanks,
> David
>
>
> On 20/06/2014 2:26 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have the fix for the following bug ready for JDK9 RT_Baseline:
>>
>>      JDK-8047104 cleanup misc issues prior to Contended Locking
>>                  reorder and cache line bucket
>>      https://bugs.openjdk.java.net/browse/JDK-8047104
>>
>> Here is the URL for the webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8047104-webrev/0-jdk9-hs-rt/
>>
>> Summary of the cleanups:
>>
>> - change Java Monitor related 'unstable' and 'unsafe' options from
>>    'product' to 'experimental'; experimental didn't exist back when
>>    these options were added to the VM
>> - delete os::PlatformEvent::TryPark(), 
>> SharedRuntime::_monitor_enter_ctr,
>>    and SharedRuntime::_monitor_exit_ctr
>> - add 'const' to some places
>> - tidy some variable decls/inits, rename some parameters to proper
>>    HotSpot style
>> - add some assert()s and guarantee()s
>> - add/correct/update comments
>>
>> Testing:
>>
>> - JPRT test job
>> - Aurora AdHoc vm.quick with Server VM fastdebug bits on all the
>>    usual platforms is in process
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>