RFR: 7169803: Usage of pretenured value is not correct

[Tao Mao]

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>
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>
> 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>
>> 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
>>> > 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
>>>>>>
>>>>>> 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/
>>>>>>> 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/20150615/e30d9e47/attachment.htm>