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
Wed Feb 13 14:13:18 UTC 2013
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?
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:
>>>>>>
>>>>>
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130213/f0efea00/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/20130213/f0efea00/attachment.jpe>
More information about the hotspot-gc-dev
mailing list