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 22:24:41 UTC 2013
Hi Bengt,
Above all, please confirm with your final say about the change.
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:
>>>>>
>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130212/7560c9b3/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/jpeg
Size: 100703 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130212/7560c9b3/attachment.jpe>
More information about the hotspot-gc-dev
mailing list