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

[Derek White]

On 6/30/16 12:28 AM, David Holmes wrote:
> 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?

Short answer is I didn't think so. collectedHeap.inline.hpp has a very 
small include set, and I was concerned about circular dependencies. 
Another approach would be to push ::set_oop_size() over to 
instanceKlassMirror.hpp, and see if the includes seem saner that way. 
I'll try some alternatives...

Thanks!

- Derek

>
> 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
>>
>>