RFR: 8166862: CMS needs klass_or_null_acquire

[Kim Barrett]

> On Oct 4, 2016, at 5:04 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 2/10/2016 7:29 AM, Kim Barrett wrote:
>> Please review this change to the CMS collector to replace uses of
>> klass_or_null with klass_or_null_acquire where needed.
>> 
>> [Reviewing on hotspot-dev; only GC code is involved, but some non-GC
>> folks have expressed interest.]
>> 
>> All non-assert uses of klass_or_null are being replaced with the
>> acquire variant.  In most cases, the size is being accessed soon
>> after, and the acquire is needed to ensure order of klass and possible
>> inline size access.  In a few cases, the need for acquire is less
>> immediately obvious:
>> 
>> - CompatibleFreeListSpace::block_is_obj - Size access is not lexically
>>  apparent, but callers access the size or other data ordered with the
>>  klass.
>> 
>> - MarkFromRootsClosure::do_bit and ParMarkFromRootsClosure::do_bit:
>>  These perform other operations that need to be properly ordered wrto
>>  the klass access.
> 
> Can you point me to the release-store that is being paired with these load-acquires please?

It’s the release_set_klass recently added here:
http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/fd16b627ebc5

> Superficially this all seems fine and can't have any correctness issues, it is just a matter of where acquire semantics are actually needed. And it is hard to tell that without understanding exactly how the different read-paths may execute relative to the write-path.

In MarkFromRootsClosure::do_bit, the first one is protecting the call to scanOopsInOop.  The second is protecting the call to mark_range, which might not be strictly necessary, but seems like this should be pretty rare.  And being consistent about the use of klass_or_null_acquire in non-assert contexts seems of value.  Though I admit to finding this block of code confusing; it’s unconditional in a product build, but only executed if !_verifying in a debug build.  Strange… but it’s been like that “forever”.

In ParMarkFromRootsClosure::do_bit, protecting call to scan_oops_in_oop.

The scanners need to be protected against reading an uninitialize length or uninitialized fields / array elements.