RFR: 8166862: CMS needs klass_or_null_acquire

[White, Derek]

-----Original Message-----
From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Kim Barrett
Sent: Tuesday, October 04, 2016 7:12 PM
To: David Holmes <david.holmes at oracle.com>
Cc: hotspot-dev Source Developers <hotspot-dev at openjdk.java.net>
Subject: Re: RFR: 8166862: CMS needs klass_or_null_acquire

> On Oct 4, 2016, at 5:04 AM, David Holmes <david.holmes at oracle.com<mailto: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

FYI
I'm still looking, but I think this is true:

I think a release-store also needs to be in the interpreter and jitted code, for processors that need it (e.g. aarch64 and ppc). For example in MacroAssembler::store_klass.

Those processors have a storestore after object initialization to ensure that Java threads don't see uninitialized fields, but don't have a store-release on setting the class to prevent CMS from tripping over a mis-ordered header initialization. The nominal store order may also be wrong (e.g. "length" before "klass"). See C1_MacroAssembler::initialize_header()

If this sounds right, I can file a bug...

 - Derek