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