Request for review (S): 8005972: ParNew should not update the tenuring threshold when promotion failed has occurred

[Bengt Rutisson]

Hi Ramki,

Thanks for looking at this!

On 1/10/13 9:28 PM, Srinivas Ramakrishna wrote:
> Hi Bengt --
>
> The change looks reasonable, but I have a comment and a follow-up 
> question.
>
> Not your change, but I'd elide the "half the real survivor size" since 
> it's really a configurable parameter based on TargetSurvivorRatio with 
> default half.
> I'd leave the comment as "set the new tenuring threshold and desired 
> survivor size".

I'm fine with removing this from the comment, but I thought the "half 
the real survivor size" aimed at the fact that we pass only the "to" 
capacity and not the "from" capacity in to compute_tenuring_threshold(). 
With that interpretation I think the comment is correct.

Would you like me to remove it anyway? Either way is fine with me.

> I'm curious though, as to what performance data prompted this change,
Good point. This change was preceded by an internal discussion in the GC 
team, so I should probably have explained the background more in my 
review request to the open.

I was comparing the ParNew and DefNew implementation since I am seeing 
some strange differences in some SPECjbb2005 results. I am running 
ParNew with a single thread and get much better score than with DefNew. 
But I also get higher average GC times. So, I was trying to figure out 
what DefNew and ParNew does differently.

When I was looking at DefNewGeneration::collect() and 
ParNewGeneration::collect() I saw that they contain a whole lot of code 
duplication. It would be tempting to try to extract the common code out 
into DefNewGeneration since it is the super class. But there are some 
minor differences. One of them was this issue with how they handle the 
tenuring threshold.

We tried to figure out if there is a reason for ParNew and DefNew to 
behave different in this regard. We could not come up with any good 
reason for that. So, we needed to figure out if we should change ParNew 
or DefNew to make them consistent. The decision to change ParNew was 
based on two things. First, it seems wrong to use the data from a 
collection that got promotion failure. This collection will not have 
allowed the tenuring threshold to fulfill its purpose. Second, 
ParallelScavenge works the same way as DefNew.

BTW, the difference between DefNew and ParNew seems to have been there 
from the start. So, there is no bug or changeset in mercurial or 
TeamWare to explain why the difference was introduced.

(Just to be clear, this difference was not the cause of my performance 
issue. I still don't have a good explanation for how ParNew can have 
longer GC times but better SPECjbb score.)

> and whether it might make sense, upon a promotion failure to do 
> something about the tenuring threshold for the next scavenge (i.e. for 
> example make the tenuring threshold half of its current value as a 
> reaction to the fact that promotion failed). Is it currently left at 
> its previous value or is it asjusted back to the default max value 
> (which latter may be the wrong thing to do) or something else?

As far as I can tell the tenuring threshold is left untouched if we get 
a promotion failure. It is probably a good idea to update it in some 
way. But I would prefer to handle that as a separate bug fix.

This change is mostly a small cleanup to make 
DefNewGeneration::collect() and ParNewGeneration::collect() be more 
consistent. We've done the thinking so, it's good to make the change in 
preparation for the next person that comes a long and has a few cycles 
over and would like to merge the two collect() methods in some way.

Thanks again for looking at this!
Bengt

>
> -- ramki
>
> On Thu, Jan 10, 2013 at 1:30 AM, Bengt Rutisson 
> <bengt.rutisson at oracle.com <mailto:bengt.rutisson at oracle.com>> wrote:
>
>
>     Hi everyone,
>
>     Could I have a couple of reviews for this small change to make
>     DefNew and ParNew be more consistent in the way they treat the
>     tenuring threshold:
>
>     http://cr.openjdk.java.net/~brutisso/8005972/webrev.00/
>     <http://cr.openjdk.java.net/%7Ebrutisso/8005972/webrev.00/>
>
>     Thanks,
>     Bengt
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130111/ee2bc14a/attachment.htm>