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
Wed Feb 13 18:23:02 UTC 2013
Since it's related to sizing policy fixes, I'd rather push it now if
possible.
Refactoring CollectorPolicy is another aspect of work, which I would
appreciate someone to do it or I can contribute as well.
Thanks.
Tao
On 2/13/2013 6:13 AM, Bengt Rutisson wrote:
>
> 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/7e921f90/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/7e921f90/attachment.jpe>
More information about the hotspot-gc-dev
mailing list