RFR: 8168914: Crash in ClassLoaderData/JNIHandleBlock::oops_do during concurrent marking (original) (raw)

Erik Helin erik.helin at oracle.com
Thu Feb 16 10:20:34 UTC 2017

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::loadacquire(&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 oopsdo(&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 {

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 { 164 static const sizet 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

More information about the hotspot-dev mailing list