RFR: 7169803: Usage of pretenured value is not correct

Jon Masamitsu jon.masamitsu at oracle.com
Wed Jun 17 15:24:49 UTC 2015


Looks good.  Reviewed.

Sorry for the late response.

Jon


On 6/15/2015 5:07 AM, David Lindholm wrote:
> Tao and Jon,
>
> I have changed the patch now so that pretenured is not part of the 
> calculation at all:
>
> http://cr.openjdk.java.net/~david/JDK-7169803/webrev.01/
>
>
> Thanks,
> David
>
> On 2015-06-14 20:58, Tao Mao wrote:
>> The code seems to only consider survisor_size and promoted_size and 
>> ignore pretenured size for most part, except here
>>
>> avg_promoted()->sample(promoted + _avg_pretenured->padded_average());
>>
>> The naming of avg_promoted() is probably not correct either if we 
>> want to make it consistent with what it takes in.
>>
>> To take all involved sizes into considerations, we need to fix all 
>> over the places when we make decisions based on the sizes.
>>
>>
>>
>> Jon, you could be right. The chances are GCSizePolicy is just doing 
>> all right and pretenured size won't matter too much whether it's in 
>> the calculation or not. That needs to be instrumented, of course.
>>
>> Thanks.
>> Tao
>>
>>
>>
>>
>>
>> On Thu, Jun 11, 2015 at 10:22 AM, Jon Masamitsu 
>> <jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>> wrote:
>>
>>
>>
>>     On 06/11/2015 12:47 AM, Tao Mao wrote:
>>>     Hi Jon,
>>>
>>>     The definition is: "Promoted" is objects that are promoted (or
>>>     tenured) during yGC while "PRE-tenured" is objects that are
>>>     allocated directly in old gen. I believe this is so, right?
>>
>>     Tao,
>>
>>     Yes, you're right about the definitions.  I think then the
>>     question is
>>     what do you want to passed to
>>
>>     avg_promoted()->sample()
>>
>>     I would say that you want to pass promoted as calculated by
>>
>>     489       size_t promoted = old_gen->used_in_bytes() -
>>     old_gen_used_before;
>>
>>     I think the bug is that _avg_pretenured->padded_average() should
>>     not be
>>     passed to sample() in the original code
>>
>>     1307 avg_promoted()->sample(promoted +
>>     _avg_pretenured->padded_average());
>>
>>     Since _avg_pretenured() should have been calculated using bytes
>>     but was using
>>     words, the bug was had less of an affect.  Also the amount
>>     pretenured was likely
>>     small in the majority of cases.  And the end affect would be that
>>     the number
>>     used for promoted would be an over-estimate and just make the
>>     ergonomics more
>>     conservative.
>>
>>     David,
>>
>>     If you could provide me with jprt binaries for before and after
>>     your fix,
>>     I'll do some experiments to see if there are any change in the number
>>     of GC (young and full) to see if we're changing the ergonomics.  It
>>     could be that the bug is a "lucky" mistake that makes things better.
>>     Unlikely but I'd like to look.
>>
>>     Thanks.
>>
>>
>>     Jon
>>
>>
>>>
>>>     Thanks.
>>>     Tao Mao
>>>
>>>
>>>
>>>     On Wed, Jun 10, 2015 at 6:48 PM, Jon Masamitsu
>>>     <jon.masamitsu at oracle.com <mailto:jon.masamitsu at oracle.com>> wrote:
>>>
>>>
>>>
>>>         On 6/10/2015 5:51 PM, Tao Mao wrote:
>>>>         I think David's right. Promoted objects and pretenured
>>>>         objects are different guys. This would resolve the second
>>>>         issue in the ticket JDK-7169803.
>>>
>>>         Tao,
>>>
>>>         How would you define each?
>>>
>>>         Jon
>>>
>>>
>>>>
>>>>         Thanks.
>>>>         Tao Mao
>>>>
>>>>         On Wed, Jun 10, 2015 at 5:08 PM, Jon Masamitsu
>>>>         <jon.masamitsu at oracle.com
>>>>         <mailto:jon.masamitsu at oracle.com>> wrote:
>>>>
>>>>
>>>>
>>>>             On 6/10/2015 1:30 AM, David Lindholm wrote:
>>>>
>>>>                 Jon,
>>>>
>>>>                 Thanks for taking a look at this. No, I don't think
>>>>                 this will lead to double counting. This calculation
>>>>                 is already there, it is just buggy. Before this
>>>>                 patch the code added the total amount of promoted
>>>>                 memory this collection with the average size of the
>>>>                 pretenured objects, and used this as a sample. Now
>>>>                 the code adds the total amount of promoted memory
>>>>                 with the total size of the pretenured objects since
>>>>                 last collection, and uses this as a sample instead.
>>>>
>>>>
>>>>             Aren't "promoted" and
>>>>             "total_pretenured_since_last_promotion()" approximately
>>>>             the same
>>>>             thing?  In share/vm/gc/parallel/psScavenge.cpp
>>>>
>>>>             489       size_t promoted = old_gen->used_in_bytes() -
>>>>             old_gen_used_before;
>>>>             490  size_policy->update_averages(_survivor_overflow,
>>>>             survived, promoted);
>>>>
>>>>             so "promoted" is the change in the old gen between the
>>>>             before and after the
>>>>             young gen collection.
>>>>
>>>>             "total_pretenured_size_last_promotion()" is
>>>>
>>>>              258   void tenured_allocation(size_t size) {
>>>>              259  _avg_pretenured->sample(size);
>>>>              260  add_pretenured_since_last_promotion(size)
>>>>
>>>>
>>>>             which seems to me to be calculating the same thing (sum
>>>>             of allocations into the old gen).
>>>>
>>>>             Not so?
>>>>
>>>>             Jon
>>>>
>>>>
>>>>
>>>>
>>>>                 Thanks,
>>>>                 David
>>>>
>>>>                 On 2015-06-09 21:37, Jon Masamitsu wrote:
>>>>
>>>>                     David,
>>>>
>>>>                     http://cr.openjdk.java.net/~david/JDK-7169803/webrev.00/src/share/vm/gc/parallel/psAdaptiveSizePolicy.cpp.frames.html
>>>>                     <http://cr.openjdk.java.net/%7Edavid/JDK-7169803/webrev.00/src/share/vm/gc/parallel/psAdaptiveSizePolicy.cpp.frames.html>
>>>>
>>>>
>>>>                     1308  avg_promoted()->sample(promoted +
>>>>                     total_pretenured_since_last_promotion());
>>>>
>>>>
>>>>                     Is including both "promoted" and
>>>>                     "total_pretenured_since_last_promotion()"
>>>>                     double counting?
>>>>
>>>>                     Jon
>>>>
>>>>                     On 06/09/2015 02:06 AM, David Lindholm wrote:
>>>>
>>>>                         Hi,
>>>>
>>>>                         Please review this patch that corrects the
>>>>                         usage of the pretenured value. There were 2
>>>>                         issues: words and bytes were mixed up and
>>>>                         the addition was done with the wrong value.
>>>>                         See bug for details.
>>>>
>>>>                         Webrev:
>>>>                         http://cr.openjdk.java.net/~david/JDK-7169803/webrev.00/
>>>>                         <http://cr.openjdk.java.net/%7Edavid/JDK-7169803/webrev.00/>
>>>>                         Bug:
>>>>                         https://bugs.openjdk.java.net/browse/JDK-7169803
>>>>
>>>>
>>>>                         Testing: Passed JPRT
>>>>
>>>>
>>>>                         Thanks,
>>>>                         David
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20150617/cf496ee3/attachment.html>


More information about the hotspot-gc-dev mailing list