RFR: 7169803: Usage of pretenured value is not correct

David Lindholm david.lindholm at oracle.com
Wed Jun 17 15:26:19 UTC 2015


Jon,

Thanks for the review!


/David

On 2015-06-17 17:24, Jon Masamitsu wrote:
> 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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150617/56e0d06a/attachment.htm>


More information about the hotspot-gc-dev mailing list