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

[David Holmes]

Hi Dan,

Generally looks fine. 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.

---

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.

---

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.

---

src/share/vm/runtime/synchronizer.cpp

579       // Handle for oop obj in case of STW safepoint

STW is redundant - all safepoints are STW.

896 // Too slow for general assert or debug
897 #if 0

I thought the trend these days was to delete unused code?

-----

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.

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). ??

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 ??

---

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
>