RFR: 7169803: Usage of pretenured value is not correct

David Lindholm david.lindholm at oracle.com
Tue Jun 16 08:31:44 UTC 2015


Tao,

Thanks for the review!


/David

On 2015-06-15 22:37, Tao Mao wrote:
> The change looks good to me.
>
> Thanks.
> Tao Mao
>
> On Mon, Jun 15, 2015 at 5:07 AM, David Lindholm 
> <david.lindholm at oracle.com <mailto:david.lindholm at oracle.com>> 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/
>     <http://cr.openjdk.java.net/%7Edavid/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/20150616/4b37eb8e/attachment-0001.html>


More information about the hotspot-gc-dev mailing list