Fwd: RFR: 8158946 - btree009 fails with assert(s > 0) failed: Bad size calculated

[Derek White]

Forward to runtime...

This bug is a race condition between allocating a java.lang.Class 
instance and concurrent GC.
https://bugs.openjdk.java.net/browse/JDK-8158946

There is an additional issue related to missing memory barriers in 
storing and loading an array's length or a java.lang.Class' oop_size 
field relative to the object's klass field, but that is handled by 
"JDK-8160369 <https://bugs.openjdk.java.net/browse/JDK-8160369> Memory 
fences needed around setting and reading object lengths."

    Context:

    "As Kim mentioned, the new version sets the object size field of a
    java.lang.Class object before it sets the object's header, so GC can
    reliably get the size of any object with it's header set.

    This fix works by adding a CollectedHeap::class_allocate() method
    and associated helpers. These are implemented in
    CollectedHeap.inline.hpp only because they share so much structure
    and code with CollectedHeap::obj_allocate() that I thought it best
    to keep them together (even though there is no performance reason to
    have the new code inlined). "

  - Derek

-------- Forwarded Message --------
Subject: 	Re: [RESUMED] RFR: 8158946 - btree009 fails with assert(s > 0) 
failed: Bad size calculated
Date: 	Tue, 28 Jun 2016 12:37:23 -0400
From: 	Derek White <derek.white at oracle.com>
Organization: 	Oracle
To: 	Thomas Schatzl <thomas.schatzl at oracle.com>, Kim Barrett 
<kim.barrett at oracle.com>
CC: 	hotspot-gc-dev at openjdk.java.net



Hi Thomas,

New webrev based on your suggestions:

Webrev: http://cr.openjdk.java.net/~drwhite/8158946/webrev.03/
Incremental: http://cr.openjdk.java.net/~drwhite/8158946/webrev.02.vs.03

jprt in progress....

Misc comments below...

   - Derek

On 6/28/16 9:09 AM, Thomas Schatzl wrote:
> Hi,
>
> On Mon, 2016-06-27 at 10:10 -0400, Derek White wrote:
>> I'd like to split out the memory fence issue from the race fixed by
>> this webrev. I think the fence issue may require more performance
>> testing and several attempts to get something satisfactory.
>>
>> New bug created:
>>      JDK-8160369 Memory fences needed around setting and reading
>> object lengths.
>>
>> How do reviewers feel about this patch to fix the initial race
>> condition?
>    looking at the 02 webrev:
>
> - http://cr.openjdk.java.net/~drwhite/8158946/webrev.02/src/share/vm/gc
> /shared/collectedHeap.inline.hpp.frames.html
> 105   // set the j.l.Class instance's oop_size field BEFORE setting the
> header:
>
> I would like to have the "why" answered here in this comment and not a
> repetition of the code. I think something like: "Concurrent readers
> expect that the size is set before the klass pointer."
>
> Maybe the comment in lines 226/227 are more appropriate here?
>
> - the "obj" parameter is cast to an oop four times in
> CollectedHeap::post_allocation_setup_class. Could you add a local
> variable?
OK, I cleaned this up by mirroring post_allocation_setup_array().
> - http://cr.openjdk.java.net/~drwhite/8158946/webrev.02/src/share/vm/oo
> ps/instanceMirrorKlass.cpp.frames.html
>
> Not sure if repeating the exit condition in line 59 makes sense.

OK.
> - http://cr.openjdk.java.net/~drwhite/8158946/webrev.02/src/share/vm/oo
> ps/oop.inline.hpp.frames.html
>
> Maybe fix the comment in 261 to a proper sentence. (And possibly 262,
> like "Oop size must be larger than zero but is %d")
OK

Thanks for the suggestions!

   - Derek


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160629/be966e6d/attachment.htm>