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

Bengt Rutisson bengt.rutisson at oracle.com
Fri Feb 15 08:07:34 UTC 2013


Tao,

I got some more background information on this from Jon. Thanks! Now I 
feel more confident that this change is correct. You can count me as 
reviewer.

Ship it!
Bengt

On 2/14/13 8:09 AM, Bengt Rutisson wrote:
>
> Jon and Tao,
>
> On 2/13/13 6:41 PM, Jon Masamitsu wrote:
>>
>> 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?
>>
>> Given the choices of fixing it now or adding it to the list for a 
>> general clean up
>> later, I would prefer that it be fixed now.  It's a small change and 
>> Tao already
>> has a fix.  Considering the small additional effort, why not push it?
>
> I don't want to block this change, but I just don't know how to review 
> it. I am not convinced that this is an improvement. Since I don't know 
> what the intended use case for GenCollectorPolicy::_size_policy is I 
> don't know for sure that it is better to use MaxGCPauseMillis than 
> MaxGCMinorPauseMillis. The initialize_size_policy() method has always 
> (in a mercurial sense) been using MaxGCMinorPauseMillis. Maybe someone 
> thought that if you inherit from GenCollectorPolicy you should be 
> adapting the young gen size?
>
> Anyway, I won't object to this being pushed. But I would not like to 
> be listed as reviewer since I really don't know how to verify that 
> this is an improvement and not a regression.
>
> Thanks,
> Bengt
>
>
>>
>> Jon
>>
>>>
>>> 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:
>>> >>>>>>
>>> >>>>>
>>> >>>>
>>> >>>
>>>
>>
>




More information about the hotspot-gc-dev mailing list