RFR: 8028498: runtime/SharedArchiveFile/CdsDifferentObjectAlignment.java asserts in RT_Baseline

Fri Mar 28 19:03:01 UTC 2014

Stefan,
This looks good. I thought this got checked in already.  I think 
removing gc_locked_out_count is fine for CheckUnhandledOops, since 
you've done the testing and haven't found a use for it.  I added it a 
long time ago and the code's changed a lot, so it's nice to clean up.
Thanks!
Coleen

On 3/27/14 10:40 AM, Stefan Johansson wrote:
> I still need a review from someone with reviewer status. Would be good 
> if someone from the runtime team who feels comfortable reviewing the 
> small CheckUnhandledOops changes could have a look.
>
> Thanks,
> Stefan
>
> On 2014-03-06 19:35, Stefan Johansson wrote:
>> As Per noticed the _gc_locked_out_count isn't updated anymore when 
>> removing the GC locker lock/unlock methods. I've looked through the 
>> code to see what would be the proper action to take in 
>> clear_unhandled_oops(). The first observation was that the check for 
>> is_gc_locked_out() only had any effect during a short period of time 
>> during startup (the only place we used GC locker lock/unlock). This 
>> leads to the question, do we need to avoid clearing the unhandled 
>> oops when the GC lockers other lock mechanism is used, and I've look 
>> this through together with Per and it seems like it's not needed.
>>
>> One can argue that if we've locked out the GC using the GC locker, it 
>> should be fine to have unhandled oops, but since we don't make use of 
>> that today I see no reason for enabling it. If I haven't missed 
>> anything this should allow us to remove the check from 
>> clear_unhandled_oops() and the _gc_locked_out_count with accessors 
>> all together.
>>
>> Please let me know if you have any fears or thoughts around this. 
>> I've run both JPRT tests and some bigapps testing with 
>> +CheckUnhandledOops without any problem as sanity testing.
>>
>> Regarding the message I updated it to use the methods suggested by Per.
>>
>> Here's the updated webrev:
>> http://cr.openjdk.java.net/~sjohanss/8028498/webrev.03/
>>
>> Thanks,
>> Stefan
>>
>> On 2014-02-17 10:58, Per Liden wrote:
>>> Stefan,
>>>
>>> I think this is a nice fix for the problem and it also cleans up the 
>>> gclocker a bit. But I have a few minor comments:
>>>
>>> vmGCOperations.cpp:
>>>  110       err_msg("GC triggered before VM initialization completed. 
>>> Try increasing "
>>>  111               "NewSize, current value " UINTX_FORMAT "K.", 
>>> NewSize / K));
>>>
>>> I'd suggest you use 
>>> byte_size_in_proper_unit()/proper_unit_for_byte_size() instead of a 
>>> "K", or maybe just not print the size at all.
>>>
>>>
>>> With the removal of GC_locker::lock()/unlock() there's no longer 
>>> anyone how modifies Thread::current()->_gc_locked_out_count. So we 
>>> should be able to remove it. However, 
>>> UnhandledOops::clear_unhandled_oops() still uses is_gc_locked_out() 
>>> so that needs to be changed to check for something else, 
>>> is_init_completed maybe?
>>>
>>> cheers,
>>> /Per
>>>
>>> On 02/12/2014 06:12 PM, Stefan Johansson wrote:
>>>> Hi,
>>>>
>>>> Stefan K pointed out to me that in the Metaspace code we use
>>>> is_init_completed() to verify that the VM is fully initialized. I
>>>> started looking at where we do set_init_completed() and realized 
>>>> that it
>>>> is right where we are ready to handle a GC. It makes sense to try to
>>>> minimize the different ways we check if the VM is fully initialized.
>>>> Using is_init_completed() instead of the GC locker to prevent GCs 
>>>> during
>>>> startup has another nice effect too; we can remove one of the two
>>>> different lock mechanisms in the GC locker.
>>>>
>>>> The new proposal removes the use of GC_locker::lock()/unlock() 
>>>> which is
>>>> only used to prevent GCs during startup. Instead there is a check 
>>>> in the
>>>> GC operations prologue, to ensure that the VM is initialized when 
>>>> the GC
>>>> starts.
>>>>
>>>> Please review the new the change:
>>>> http://cr.openjdk.java.net/~sjohanss/8028498/webrev.02/
>>>>
>>>> Testing:
>>>> * JPRT
>>>> * GC locker test through UTE
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>> On 2014-02-11 16:12, Stefan Johansson wrote:
>>>>> Hi again =)
>>>>>
>>>>> Don't spend time on the current webrev (02). I've gotten some more
>>>>> comments on the proposed fix and will look at a solution that 
>>>>> allow us
>>>>> to remove the GC locker usage during initialization.
>>>>>
>>>>> Cheers,
>>>>> Stefan
>>>>>
>>>>> On 2014-02-10 18:51, Stefan Johansson wrote:
>>>>>> Hi again,
>>>>>>
>>>>>> After a couple of offline discussions with different people I've 
>>>>>> come
>>>>>> to the conclusion to split out the sizing of the young generation
>>>>>> from the fix for this issue. I've created a new bug (JDK-8033426) 
>>>>>> for
>>>>>> that, which is currently out for review.
>>>>>>
>>>>>> The discussions also lead to a different approach for fixing this
>>>>>> issue. The new proposal is to use the GC locker to prevent GCs until
>>>>>> we are ready to handle them and otherwise fail the VM 
>>>>>> initialization.
>>>>>> The gc locker has two different mechanisms for locking out the GC,
>>>>>> one which is used during initialization and one used to stall GCs
>>>>>> when doing critical JNI work. The part of the initialization that
>>>>>> currently is locked by the GC locker is to narrow and there is no
>>>>>> guarantee that a GC can be handled after the GC locker is unlocked.
>>>>>> This fix will extend the part that is locked and also add a check
>>>>>> that if a GC is initiated while locked during startup the VM exits.
>>>>>>
>>>>>> New webrev:
>>>>>> http://cr.openjdk.java.net/~sjohanss/8028498/webrev.01/
>>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>> On 2014-01-29 14:54, Stefan Johansson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this fix for:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8028498
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~sjohanss/8028498/webrev.00/
>>>>>>>
>>>>>>> Summary:
>>>>>>> The initial young generation size has been fairly small by default
>>>>>>> (1.5M) and if setting ObjectAlignmentInBytes to something larger
>>>>>>> than the default it can causes CDS to trigger a GC when dumping the
>>>>>>> archive. At this point the VM is not ready to handle a GC and we
>>>>>>> will hit an assertion. Making sure we can handle a GC at this point
>>>>>>> is not trivial and the proposed solution is to alter the default
>>>>>>> sizing of the young generation as well as adding a safety check 
>>>>>>> when
>>>>>>> dumping the archive to exit the VM if the young generation is too
>>>>>>> small.
>>>>>>>
>>>>>>> Testing:
>>>>>>> * JPRT build and test passes
>>>>>>> * Failing test now passes on all platforms (tested throught JPRT)
>>>>>>> * Verified that the GC and Runtime jtreg tests still passes
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Stefan
>>>>>>
>>>>>
>>>>
>>>
>>
>