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

[David Holmes]

Hi Derek,

Understand the fix in principle.

But I don't understand why you got rid of java_lang_Class::set_oop_size, 
and instead exposed java_lang_Class::oop_size_offset(), then had 
InstanceMirrorKlass::allocate_instance pass the offset to 
CollectedHeap::class_allocate, which passes it to 
CollectedHeap::post_allocation_setup_class - that seems rather 
convoluted. Can CollectedHeap::post_allocation_setup_class not call 
java_lang_Class::set_oop_size directly?

Thanks,
David


On 30/06/2016 2:37 AM, Derek White wrote:
> 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
>
>