RFR 8085965: VM hangs in C2Compiler

[Jon Masamitsu]

On 06/16/2015 01:30 PM, Poonam Bajaj Parhar wrote:
> Hi Jon,
>
> On 6/16/2015 11:47 AM, Poonam Bajaj Parhar wrote:
>> Hello Jon,
>>
>> On 6/16/2015 10:59 AM, Jon Masamitsu wrote:
>>> 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.
>>>
>> For CMS, ExplicitGCInvokesConcurrent won't get set to true if 
>> ClassUnloading is false. In the argument parsing we would set 
>> ExplicitGCInvokesConcurrentAndUnloadsClasses to false and while 
>> constructing CMSCollector, we set ExplicitGCInvokesConcurrent.
>>
>>   if (ExplicitGCInvokesConcurrentAndUnloadsClasses) {
>>     ExplicitGCInvokesConcurrent = true;
>>   }
>>
>> I am testing it to confirm.
>>
> Here's the result:
>
> java -XX:+ExplicitGCInvokesConcurrentAndUnloadsClasses 
> -XX:-ClassUnloading -XX:+UseConcMarkSweepGC  -XX:+PrintFlagsFinal 
> -version
> [Global flags]
>     uintx AdaptiveSizeDecrementScaleFactor          = 
> 4                                   {product}
>     ...
>     bool CMSClassUnloadingEnabled                 := 
> false                               {product}
>     ...
>     bool ClassUnloading                           := 
> false                               {product}
>     ...
>     bool ExplicitGCInvokesConcurrent               = 
> false                               {product}
>     bool ExplicitGCInvokesConcurrentAndUnloadsClasses := 
> false                               {product}
>
>

Poonam,

Thanks.  That's the right answer.  I'll go finish the review.

Jon

>
> Thanks,
> Poonam
>
>
>> Thanks,
>> Poonam
>>
>>> 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.
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>