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
Tue Feb 12 20:03:49 UTC 2013
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.org/pipermail/hotspot-gc-dev/attachments/20130212/8f6bcbde/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ijahgdbe.jpg
Type: image/jpeg
Size: 100703 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130212/8f6bcbde/ijahgdbe.jpg>
More information about the hotspot-gc-dev
mailing list