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

[Stefan Johansson]

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