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

David Holmes david.holmes at oracle.com
Thu Feb 16 13:15:16 UTC 2017

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::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. 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 oopsdo it may not actually matter. By “first” I mean the first chunk. only the first chunk’s size is subject to modification. The releasestore 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 loadacquire 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 loadacquire 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::loadptracquire((volatile intptrt*)&head); const juint size = (c == NULL ? 0 : OrderAccess::loadacquire(&c- size)); while (c != NULL) { size = OrderAccess::loadacquire(&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 loadacquire 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

More information about the hotspot-dev mailing list