RFR 8085965: VM hangs in C2Compiler

[Jon Masamitsu]

Poonam,

With your changes try

java -XX:+ExplicitGCInvokesConcurrentAndUnloadsClasses 
-XX:-ClassUnloading -XX:+UseConcMarkSweepGC  -XX:+PrintFlagsFinal -version

and check if ExplicitGCInvokesConcurrent is true or false.

If it is true, I don't know if that is the right thing to do.  In this 
case on the
command line ExplicitGCInvokesConcurrentAndUnloadsClasses has been
set to true and ClassUnloading has been set to false.  As a side effect,
ExplicitGCInvokesConcurrent is set to true.  Whereas 
ExplicitGCInvokesConcurrentAndUnloadsClasses
has been set back to false (probably) That may be a surprise to the
users.

Jon

On 06/15/2015 11:36 AM, Poonam Bajaj Parhar wrote:
> Hello,
>
> Here's the updated webrev:
> http://cr.openjdk.java.net/~poonam/8085965/webrev.03/
> - Set ExplicitGCInvokesConcurrentAndUnloadsClasses too to false when 
> ClassUnloading is turned off.
> - removed the changes from unpdate_should_unload_classes() in 
> concurrentMarkSweepGeneration.cpp
> - Added the change to mark the classes in genMarkSweep.cpp when 
> ClassUnloading is off to make sure that classes don't get unloaded 
> with -XX:-ClassUnloading.
>
> Thanks,
> Poonam
>
> On 6/15/2015 7:04 AM, Poonam Bajaj Parhar wrote:
>> Hello Kim,
>>
>> Thanks for the review!
>>
>> On 6/13/2015 11:53 PM, Kim Barrett wrote:
>>> On Jun 11, 2015, at 3:34 PM, Jon Masamitsu 
>>> <jon.masamitsu at oracle.com> wrote:
>>>> Poonam,
>>>>
>>>> Thanks for the changes.
>>>>
>>>> What do you think about deleting this part of the
>>>> change (line 2759) from the flag processing?
>>>> It feels like we're fixing something in two places where
>>>> as we only really need the change in
>>>> Arguments::set_cms_and_parnew_gc_flags().
>>>>
>>>> http://cr.openjdk.java.net/~poonam/8085965/webrev.01/src/share/vm/runtime/arguments.cpp.frames.html 
>>>>
>>>>
>>>>
>>>> 2759       FLAG_SET_CMDLINE(bool, CMSClassUnloadingEnabled, false);
>>> That seems wrong to me.
>>>
>>> It seems to me that the problem here is inconsistencies in the 
>>> (final) CLA values.
>>> I think no change would be needed in 
>>> concurrentMarkSweepGeneration.cpp if the CLA
>>> values were consistent.  By consistent here I mean:
>> Yes, the problem here is the inconsistent values of ClassUnloading 
>> and the concurrent-class-unloading options.
>>
>>> If ClassUnloading is false, then all CLA variations on class 
>>> unloading must also be false.
>>> There are at least:
>>>
>>> ClassUnloadingWithConcurrentMark
>>> CMSClassUnloadingEnabled
>>> ExplicitGCInvokesConcurrentAndUnloadsClasses
>>>
>>> I don’t know if there are more.
>>>
>>> At least some of those might be false when ClassUnloading is true.  
>>> And of course, all three of
>>> those should be treated as effectively false (and perhaps set false) 
>>> when a non-concurrent collector
>>> is invoked.
>>
>> I agree that that all the concurrent class unloading options should 
>> be set false when ClassUnloading is switched off. I will set 
>> ExplicitGCInvokesConcurrentAndUnloadsClasses too to false as part of 
>> this fix but would avoid changing the G1 specific option as I won't 
>> be able to verify the effects of that change for G1.
>>
>> During the testing another issue was found where class unloading was 
>> still occurring during Full GC events when running with CMS and 
>> -XX:-ClassUnloading. And the reason for that was that currently we 
>> are not marking the classes during the roots processing(in 
>> mark_sweep_phase1()) even when ClassUnloading is off. The following 
>> change will fix that problem. The changes are currently under testing 
>> and I will resubmit the webrev after verification of this change.
>>
>>   gch->gen_process_roots(level,
>>                          false, // Younger gens are not roots.
>>                          true,  // activate StrongRootsScope
>>                          SharedHeap::SO_None,
>>                          GenCollectedHeap::StrongRootsOnly, <----
>>                          &follow_root_closure,
>>                          &follow_root_closure,
>>                          &follow_cld_closure);
>>
>> to
>>   gch->gen_process_roots(level,
>>                          false, // Younger gens are not roots.
>>                          true,  // activate StrongRootsScope
>>                          SharedHeap::SO_None,
>>                          ClassUnloading,  <-----
>>                          &follow_root_closure,
>>                          &follow_root_closure,
>>                          &follow_cld_closure);
>>
>> Thanks,
>> Poonam
>>
>>>
>>> I think Gerard’s argument checking work will have some impact here.  
>>> I’d suggest that there should
>>> be constraint functions for these arguments that verify consistency 
>>> at the end of argument processing.
>>>
>>>
>>
>