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

[Bengt Rutisson]

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/20130210/4ced7024/attachment.htm>