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

[Daniel D. Daugherty]

Closing the loop on the remaining open issues from the first
round of code review...

Hopefully, I'll have an updated webrev ready for the second round of
code review later today.


On 6/20/14 9:23 AM, Daniel D. Daugherty wrote:
>
>> 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.

We've been trying to reach closure on the new wording for atomic.hpp
in off-thread e-mails for almost two weeks. At this point, I'm dropping
the atomic.hpp changes from this bug fix (8047104). Changes to the
wording in atomic.hpp may happen in a future Contended Locking bucket,
but for now it's not happening.


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

After off-thread discussions with both Dave D and David D, we're
going to leave the uses of 'STW' in the monitor code alone.


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

Will add Knob_VerifyInUse flag in the same way as the Knob_Verbose
flag, will remove the '#if 0 ... #endif' to enable compiling
ObjectSynchronizer::verifyInUse(), and will uncomment the calls
to verifyInUse() and make the calls conditional on Knob_VerifyInUse.


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

Dave D has proposed the following rewrite:

 > Loads and stores in the critical section — which appear in program
 > order before the store that releases the lock — must also appear
 > before the store that releases the lock in memory visibility order.

David H and I are good with the above.


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

Two resolutions here. First, fix this typo:

4404 // Conceptually we need a #storeload|#storestore "release" MEMBAR 
before

by changing it to:

4404 // Conceptually we need a #loadstore|#storestore "release" MEMBAR 
before

Second, consider changing the following code:

4397 OrderAccess::fence(); // guarantee at least release consistency.
4407 *adr = 0;

to this code in the "adaptive spin" bucket:

4407 OrderAccess::release_store(adr, 0); // guarantee at least release 
consistency.

where the change can undergo more extensive testing than in this cleanup
bucket.

Dan