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

[Erik Helin]

On 02/16/2017 10:09 AM, David Holmes wrote:
> On 16/02/2017 6:55 PM, Thomas Schatzl wrote:
>> Hi,
>>
>> On Thu, 2017-02-16 at 18:09 +1000, David Holmes wrote:
>>> Bah! didn't finish my edit ...
>>>
>>> On 16/02/2017 5:54 PM, David Holmes wrote:
>>>>
>>>> On 16/02/2017 5:46 PM, Kim Barrett wrote:
>>>>>
>>>>>>
>>>>>> On Feb 16, 2017, at 2:41 AM, David Holmes <david.holmes at oracle.
>>>>>> com>
>>>>>> wrote:
>>>>>>
>>>>>> On 16/02/2017 3:53 PM, Kim Barrett wrote:
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> /src/share/vm/classfile/classLoaderData.cpp
>>>>>>>>> 138     const juint size = OrderAccess::load_acquire(&c-
>>>>>>>>>> size);
>>>>>>>>>
>>>>>>>>> I think all but the first chunk have a constant size of
>>>>>>>>> the
>>>>>>>>> maximum size, so no need for acquire except for the
>>>>>>>>> first.
>>>>>>>> The size is incremented on each add, so this acquire sync's
>>>>>>>> with
>>>>>>>> the release of the new size, so that we are sure to see the
>>>>>>>> oop
>>>>>>>> that was just added.
>>>>>>>>
>>>>>>>> Though given the races between add and oops_do it may not
>>>>>>>> actually
>>>>>>>> matter.
>>>>>>> By “first” I mean the first chunk.  only the first chunk’s
>>>>>>> size is
>>>>>>> subject to modification.  The release_store of _head that
>>>>>>> made the
>>>>>>> first chunk accessible will be after the setting of the next
>>>>>>> chunk’s
>>>>>>> size to its final (max) value.  The corresponding
>>>>>>> load_acquire of
>>>>>>> _head ensures any _next chunks are visibly complete.  It’s
>>>>>>> the same
>>>>>>> rationale for _next not requiring any special handling. At
>>>>>>> least,
>>>>>>> that’s how it looks to me right now; am I missing something.
>>>>>> The _head chunk has its _size incremented on each add().
>>>>>>
>>>>>> David
>>>>> Agreed, and a load_acquire is needed for the _head chunk.
>>>>>
>>>>> I’m suggesting an ordinary load is sufficient for any _next
>>>>> chunks as
>>>>> we loop down the list.
>>>> Ah! Now I get what you mean - sorry. Yes only the first loop
>>>> iteration
>>>> needs the load-acquire. So just:
>>>>
>>>> Chunk* c = (Chunk*) OrderAccess::load_ptr_acquire((volatile
>>>> intptr_t*)&_head);
>>>> const juint size = (c == NULL ? 0 : OrderAccess::load_acquire(&c-
>>>>> size));
>>>> while (c != NULL) {
>>>>     size = OrderAccess::load_acquire(&c->size);
>>>        size = c->size;
>>
>> Maybe I'm wrong, but just looking at this suggestion without more
>> thought: what if the value of size changes just between the above
>> load_acquire and the re-read of the size, and the actual value in the
>> array has not been updated yet?
>
> Yeah I screwed up peeling out the initial read of size. That line should
> come at the end of the loop after c=c->next;

I agree that we can implement this optimization, but I'm not sure it is 
worth it? The code will become quite a bit more cumbersome vs the 
straightforward code (well, as straightforward as lock free code gets) 
that is in the patch right now.

Thanks,
Erik

> Thanks,
> David
>
>> If that optimization is done, please just peel out the first iteration.
>>
>> Thanks,
>>   Thomas
>>