RFR 8085965: VM hangs in C2Compiler

[Jon Masamitsu]

On 06/16/2015 01:31 PM, Jon Masamitsu wrote:
>
>
> 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.

Poonam,

I think the rest of the change looks good.

Reviewed.

Jon

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