Request for review: 8007764: Wrong initialized value of max_gc_pause_sec for an instance of class AdaptiveSizePolicy

Tao Mao tao.mao at oracle.com
Wed Feb 13 18:23:02 UTC 2013


Since it's related to sizing policy fixes, I'd rather push it now if 
possible.

Refactoring CollectorPolicy is another aspect of work, which I would 
appreciate someone to do it or I can contribute as well.

Thanks.
Tao

On 2/13/2013 6:13 AM, Bengt Rutisson wrote:
>
> Hi Tao,
>
> On 2/12/13 11:24 PM, Tao Mao wrote:
>> Hi Bengt,
>> Above all, please confirm with your final say about the change.
>
> I think you are correct that this change won't have any effect. But in 
> that case, is it still worth doing?
>
> A short while ago, Erik sent out a suggestion for how to refactor the 
> CollectorPolicy code. Maybe we should just file a bug for cleaning 
> this up properly? In that bug we could add a comment about this 
> particular change.
>
> I'm not sure what the best approach is. I kind of prefer removing dead 
> code paths rather than trying to fix them. Also, I don't know how to 
> verify that the fix is correct if it is not being used.
>
> Anybody else have any opinions on this?
>
> Thanks,
> Bengt
>
>>
>> Thanks.
>> Tao
>>
>> On 2/12/2013 12:03 PM, Tao Mao wrote:
>>> Hi all,
>>>
>>> After some investigation of the code, I figured out that the only 
>>> opportunity where my change will affect something is in 
>>> MarkSweepPolicy, which is supposed to support sizing policy (if any) 
>>> for UseSerialGC [1].
>>>
>>> But, I think, SerialGC hasn't implemented any sizing policy 
>>> associated with flags MaxGC*PauseMillis (I'm not sure about it 
>>> tho/). So, we can conclude that if a customer uses SerialGC, they 
>>> wouldn't set MaxGC*PauseMillis anyway.
>>>
>>> (Bengt and Jon, please help verify the above conclusion.)
>>>
>>> Therefore, the change will fix the initialization bug in a safe way.
>>>
>>> It did take me a while to figure out the class and method hierarchy. 
>>> I share the diagram as a pic below. Hopefully, we can use it to 
>>> build up a better class design if possible.
>>>
>>> Sorry for the handwriting and not being able to draw a fancier 
>>> diagram in text .
>>>
>>> Thanks.
>>> Tao
>>>
>>> [1] in Universe::initialize_heap()
>>>
>>> if (UseParallelGC) {
>>> #ifndef SERIALGC
>>>     Universe::_collectedHeap = new ParallelScavengeHeap();
>>> #else  // SERIALGC
>>>     fatal("UseParallelGC not supported in this VM.");
>>> #endif // SERIALGC
>>>
>>>   } else if (UseG1GC) {
>>> #ifndef SERIALGC
>>>     G1CollectorPolicy* g1p = new G1CollectorPolicy();
>>>     G1CollectedHeap* g1h = new G1CollectedHeap(g1p);
>>>     Universe::_collectedHeap = g1h;
>>> #else  // SERIALGC
>>>     fatal("UseG1GC not supported in java kernel vm.");
>>> #endif // SERIALGC
>>>
>>>   } else {
>>>     GenCollectorPolicy *gc_policy;
>>>
>>>     if (UseSerialGC) {
>>>       gc_policy = new MarkSweepPolicy();
>>>     } else if (UseConcMarkSweepGC) {
>>> #ifndef SERIALGC
>>>       if (UseAdaptiveSizePolicy) {
>>>         gc_policy = new ASConcurrentMarkSweepPolicy();
>>>       } else {
>>>         gc_policy = new ConcurrentMarkSweepPolicy();
>>>       }
>>> #else   // SERIALGC
>>>     fatal("UseConcMarkSweepGC not supported in this VM.");
>>> #endif // SERIALGC
>>>     } else { // default old generation
>>>       gc_policy = new MarkSweepPolicy();
>>>     }
>>>
>>>     Universe::_collectedHeap = new GenCollectedHeap(gc_policy);
>>>   }
>>>
>>>
>>>
>>>
>>> On 2/10/2013 1:34 PM, Bengt Rutisson wrote:
>>>>
>>>>
>>>> Hi Tao,
>>>>
>>>> Thanks for your answers. Some comments inline.
>>>>
>>>> On 2/9/13 12:44 AM, Tao Mao wrote:
>>>>> I'm trying to answer these questions. Please see inline. Thanks.
>>>>> Tao
>>>>>
>>>>> On 2/8/13 8:17 AM, Bengt Rutisson wrote:
>>>>>>
>>>>>> Hi Tao,
>>>>>>
>>>>>> I think the change looks good and makes sense.
>>>>>>
>>>>>> But I have a hard time estimating the implications of this change.
>>>>>>
>>>>>> It seems like MaxGCMinorPauseMillis is only used by CMS and 
>>>>>> ParallelScavenge. 
>>>>> You are right. Before the fix, four occurrences below. (The one in 
>>>>> collectorPolicy.cpp is suspected to be wrong)
>>>>>
>>>>> MaxGCMinorPauseMillis is introduced in different kinds of classes 
>>>>> for CMS and PS: ConcurrentMarkSweep*Policy* and ParalleScavenge*Heap*.
>>>>>
>>>>> $ grep -nr "MaxGCMinorPauseMillis" 
>>>>> src/src/share/vm/gc_implementation/concurrentMarkSweep/cmsCollectorPolicy.cpp:87:  
>>>>> double max_gc_minor_pause_sec = ((double) 
>>>>> MaxGCMinorPauseMillis)/1000.0;
>>>>> src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.cpp:156:  
>>>>> double max_gc_minor_pause_sec = ((double) 
>>>>> MaxGCMinorPauseMillis)/1000.0;
>>>>> src/share/vm/memory/collectorPolicy.cpp:170:  const double 
>>>>> max_gc_minor_pause_sec = ((double) MaxGCMinorPauseMillis)/1000.0;
>>>>> src/share/vm/runtime/globals.hpp:2043:  product(uintx, 
>>>>> MaxGCMinorPauseMillis, max_uintx,                          \
>>>>>> For both of those the default for MaxGCMinorPauseMillis is the 
>>>>>> same as MaxGCPauseMillis so it should be fine.
>>>>> How did you infer that the default value for MaxGCMinorPauseMillis 
>>>>> is same as MaxGCPauseMillis? I didn't see such an indication in 
>>>>> the code.
>>>>
>>>> I actually ran with both collectors using -XX:+PrintFlagsFinal and 
>>>> looked at the values printed. But you find it in the code too. Both 
>>>> flags are declared in globals.hpp as having max_uintx as default value:
>>>>
>>>>
>>>>   product(uintx, MaxGCPauseMillis, 
>>>> max_uintx,                               \
>>>>           "Adaptive size policy maximum GC pause time goal in msec, 
>>>> "       \
>>>>           "or (G1 Only) the max. GC time per MMU time 
>>>> slice")               \
>>>>
>>>> product(uintx, MaxGCMinorPauseMillis, 
>>>> max_uintx,                          \
>>>>           "Adaptive size policy maximum GC minor pause time goal in 
>>>> msec")  \
>>>>
>>>>
>>>>
>>>>>>
>>>>>> But what happens if a customer is running with 
>>>>>> -XX:MaxGCMinorPauseMillis=1000 but not setting MaxGCPauseMillis 
>>>>>> on the command line? What are the implications of your change for 
>>>>>> such runs? I don't think it is very common. Just thinking.
>>>>> For ParallelScavenge, customers should distinguish the two flags.
>>>>
>>>> Yes, they should, but my point was that if a customer was just 
>>>> setting one of the flags their application might behave differently 
>>>> now. And I was trying to understand in what way it would behave 
>>>> different. But I actually think that your change will not affect 
>>>> ParallelScavenge anyway. See below.
>>>>
>>>>> For CMS, I wonder whether we have fully implemented adaptive 
>>>>> sizing policy. Jon, can you comment on this?
>>>>> In either case, customers should be able to tell apart.
>>>>
>>>> You are correct. Implementing adaptive sizing was never completed 
>>>> for CMS so, it is turned off. Let's ignore CMS for now.
>>>>
>>>>>>
>>>>>> Also, a more common case is probably to run with only 
>>>>>> -XX:MaxGCPauseMillis set on the command line. How will the 
>>>>>> adaptive sizing behave differently compared to before?
>>>>> The fix only changed the value in the routine 
>>>>> GenCollectorPolicy::initialize_size_policy, which is believed to 
>>>>> be called within
>>>>> the routine of GenCollectedHeap::post_initialize(). I'm not sure 
>>>>> whether this routine can change sizing behaviors somewhere.
>>>>
>>>> post_initialize() is called from universe_post_init() for all 
>>>> collectors. But only the CollectedHeaps that inherit from 
>>>> GenCollectedHeap will be affected by your change.
>>>>
>>>> ParallelScavengeHeap does not inherit from GenCollectedHeap and it 
>>>> implements its own post_initialize(), so I think it is unaffected 
>>>> by your change.
>>>>
>>>> So, that's why I came to the conclusion that the code that your are 
>>>> changing is actually not being used by anyone. If this is correct I 
>>>> think your change is safe and for the better. But I also think that 
>>>> we could possibly clean up the code and get rid of some code paths. 
>>>> This should probably be done as a separate change though.
>>>>
>>>> Bengt
>>>>
>>>>
>>>>
>>>>>>
>>>>>> Looking closer at the code it looks like CMS and ParallelScavenge 
>>>>>> are actually using CMSAdaptiveSizePolicy and PSAdaptiveSizePolicy 
>>>>>> respectively. Both of these already pass the correct value to the 
>>>>>> constructor of the super class AdaptiveSizePolicy. So, this bug 
>>>>>> is currently benign. The code that you are changing is actually 
>>>>>> not being used. Is this a correct conclusion?
>>>>> See above. Whether the code I'm changing is being used depends 
>>>>> upon whether GenCollectedHeap::post_initialize() is called 
>>>>> anywhere. But I can't determine that right now.
>>>>>
>>>>> Other than that, the fix should correct the bug while preserving 
>>>>> everything else.
>>>>>>
>>>>>> Thanks,
>>>>>> Bengt
>>>>>>
>>>>>> On 2/8/13 3:42 AM, Tao Mao wrote:
>>>>>>> 8007764: Wrong initialized value of max_gc_pause_sec for an 
>>>>>>> instance of class AdaptiveSizePolicy
>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8007764
>>>>>>>
>>>>>>> webrev:
>>>>>>> http://cr.openjdk.java.net/~tamao/8007764/webrev.00/
>>>>>>>
>>>>>>> changeset:
>>>>>>>
>>>>>>
>>>>>
>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20130213/7e921f90/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/jpeg
Size: 100703 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20130213/7e921f90/attachment.jpe>


More information about the hotspot-gc-dev mailing list