RFR: 7169803: Usage of pretenured value is not correct

[Tao Mao]

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/20150614/7f8df142/attachment.htm>