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

Jon Masamitsu jon.masamitsu at oracle.com
Wed Feb 13 17:41:37 UTC 2013 

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?

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