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

[Erik Helin]

Hi Kim,

thanks for reviewing!

On 02/16/2017 03:41 AM, 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.

Please see my reply to my David. I agree that this can be optimized, but 
I'm not sure if it is worth it (the code will not become harder to 
understand)?

> ------------------------------------------------------------------------------
> /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?

Please see my reply to Thomas. We don't care :)

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

Yep, agree!

> ------------------------------------------------------------------------------
> /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.
>
> The whole Chunk definition could also be moved to the .cpp file, with
> only a forward declaration here.

Please see my reply to Coleen, I wanted to keep the same characteristics 
as the code has today when it uses a JNIHandleBlock.

> ------------------------------------------------------------------------------
> /src/share/vm/classfile/classLoaderData.cpp
>  626   return (jobject) _handles.add(h());
> and
>  631   *((oop*) h) = NULL;
>
> Having spent a bunch of time recently dealing with and cleaning up a
> bunch of places that break the jobject abstraction (though by no means
> all), I'm not happy to see new ones.
>
> As Coleen said, there's discussion about how to refer to indirect
> pointers to oops in runtime code.  jobject has been used as an answer
> for that in some places, but they might need to be changed.  Hiding
> the conversions as casts will just make that harder.  I'm thinking
> some named conversion functions are in order, instead of bare casts.

Hmm, I would not like to introduce too much of Coleen's work in this 
patch. Do you have something minimal in mind that you think improves the 
situation? Or should we just note this place in the code and come back 
to it in 10?

Thanks,
Erik