RFR: 8168914: Crash in ClassLoaderData/JNIHandleBlock::oops_do during concurrent marking

[David Holmes]

On 21/02/2017 11:57 AM, Kim Barrett wrote:
>> On Feb 20, 2017, at 5:52 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 02 -> 03:
>>> - Only use load_acquire when reading the size for the "head" chunk
>>
>> Sorry but I don't like the way this is done - the conditional may end up being more expensive than the unnecessary load-acquire. Unrolling the first loop iteration, as per the email discussion, is a better way to go IMO.
>
> +1
>
>> And as Kim indicated you shouldn't need any casts for the OrderAccess ops as you should be using the "void*" versions.
>>
>>> - Fix grammar in comment
>>> - Refer to the correct lock in comment
>>>
>>> AFAICS (scanning through all the emails) there are now only three
>>> remaining comments:
>>> - Thomas: can we assert in ChunkedHandeList::add that the correct mutex
>>>          is locked?
>>> - Then we would have to pass the mutex along as a parameter to `add`,
>>
>> Why do you need to pass it in?
>>
>> oop* ClassLoaderData::ChunkedHandleList::add(oop o) {
>>  assert(ClassLoaderData::metaspace_lock()->owned_by_self(), "should own lock");
>
> The metaspace_lock() is per-CLD, so that doesn’t work.

Ah! The use of ClassLoaderData::metaspace_lock() in the comment threw 
me. So we could have the ChunkHandleList store a back-pointer to the CLD 
that owns it. But for a single method with a single caller this seems 
like overkill.

BTW not sure about nested classes in C++ but does anything in 
ChunkHandleList need to be public?

Thanks,
David