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

[David Holmes]

On 16/02/2017 11:14 PM, Erik Helin wrote:
> 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.

I don't think it gets cumbersome. The removal of load_acquire simplifies 
things :)

David

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