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

Thu Mar 6 18:35:16 UTC 2014

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