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

[Erik Helin]

On 02/16/2017 10:06 AM, Thomas Schatzl wrote:
> Hi,
>
> On Wed, 2017-02-15 at 21:41 -0500, Kim Barrett wrote:
>>>
>>> On Feb 15, 2017, at 10:07 AM, Erik Helin <erik.helin at oracle.com>
>>> wrote:
>>>
>>> On 02/15/2017 02:48 AM, David Holmes wrote:
>>>>
>>>> Hi Erik,
>>> Hi David,
>>>
>>> thanks for having a look! Please see new patches at:
>>> - incremental: http://cr.openjdk.java.net/~ehelin/8168914/00-01/
>>> - full: http://cr.openjdk.java.net/~ehelin/8168914/01/
>> -------------------------------------------------------------------
>> -----------
>> /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.
>>
>> -------------------------------------------------------------------
>> -----------
>> /src/share/vm/classfile/classLoaderData.cpp
>>  173   VerifyContainsOopClosure cl(op);
>>  174   oops_do(&cl);
>>
>> We don't care that this iterates over the entire list even if found
>> early?
>
> Not trying to defend the change: I thought the same, but it is only
> verification code, and as Coleen and you suggested, there are very few
> entries anyway. Also, the current interface (including the used
> OopClosure) do not allow returning a value anyway.

Yep, this was my reasoning, there is no need to optimize this.

> Unless there is some AbortableOopClosure that can be used, I would
> recommend moving this to an RFE this late in the release.

We do want AbortableOopClosure (I had a patch circling around a long 
time ago), but I don't think the optimization is worth it here.

>> -------------------------------------------------------------------
>> -----------
>> /src/share/vm/classfile/classLoaderData.hpp
>>  162   struct ChunkedHandleList : public CHeapObj<mtClass> {
>>
>> As used, this doesn't seem to need to be CHeapObj.

Correct (also noticed by Coleen).

>> -------------------------------------------------------------------
>> -----------
>> /src/share/vm/classfile/classLoaderData.hpp
>>  163     struct Chunk : public CHeapObj<mtClass> {
>>  164       static const size_t CAPACITY = 64;
>>  165       oop data[CAPACITY];
>>
>> Large fixed capacity seems problematic?  I think there are lots of
>> "small" CLDs.  Don't anonymous classes have there own class loader?
>> A variable sized chunk with a capacity field doesn't seem like it
>> should introduce any new problems.
>
> While the default value might be too high, particularly a variable
> sized chunk size with very few entries will to add the overhead of the
> additional capacity field.
> For those ClassLoaders with very few entries, the existing Chunk
> already seems to have a quite high overhead.
>
> Maybe some specialization for anonymous vs. regular class loaders?
>
> Do you (Kim, Coleen) know typical sizes of this list for both kinds of
> class loaders?
>
>> The whole Chunk definition could also be moved to the .cpp file, with
>> only a forward declaration here.
>
> I would personally kind of prefer to keep them together here. I am fine
> with either way though.

I would also prefer to keep them together (but don't have a very strong 
opinion).

Thanks,
Erik

> Thanks,
>   Thomas
>