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
Thu Feb 14 07:09:52 UTC 2013
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