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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150616/4b37eb8e/attachment.htm>
More information about the hotspot-gc-dev
mailing list