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

[Tao Mao]

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.
>
> 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.
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.
>
> 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.
>
> 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/20130208/eb92730f/attachment.htm>