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

Mon Mar 31 11:11:31 UTC 2014

Thanks for reviewing Coleen!

Stefan

On 2014-03-28 20:03, Coleen Phillimore wrote:
>
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>