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

[Thomas Schatzl]

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.

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

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

Thanks,
  Thomas