RFR: 8033426: Scale initial NewSize using NewRatio if not set on command line
jesper.wilhelmsson at oracle.com
Fri Feb 7 13:56:19 UTC 2014
The new test looks fine.
My internal parser would prefer if you were consistent with spaces vs no spaces
in expressions like 20*M. I like spaces. I could however live without the empty
line after the call to restore_flags().
I also fail to parse the comment:
// if NewSize set on command line should take for both min
// and initial if less than min heap.
I think it should say:
// If NewSize is set on the command line, it should be used
// for both min and initial young size if less than min heap.
With a little cleanup, ship it!
Stefan Johansson skrev 7/2/14 1:31 PM:
> Hi again,
> I've looked at how to test this in a good way and come up with this simple unit
> test to verify the basic rules for NewSize. This test doesn't strive to cover
> all possible combinations of how NewSize interacts with other flags, but instead
> focus on testing that a few well defined cases work as expected without
> duplicating a lot of logic from the product.
> Please review the updated webrev:
> On 2014-02-04 17:08, Stefan Johansson wrote:
>> Hi Jesper,
>> On 2014-02-03 17:50, Jesper Wilhelmsson wrote:
>>> Hi Stefan,
>>> Have you looked at how this change plays with the ergonomics for NewSize in
>>> Arguments::set_cms_and_parnew_gc_flags()? That code will also use NewRatio to
>>> scale NewSize, but if running with a large CMSYoungGenPerWorker it may end up
>>> with a larger NewSize. Looks like your change will handle it well, but it
>>> would be nice with some test to verify it going forward.
>> I hadn't looked into this in depth but I took a look now and it seems to be
>> handled well as you say. If the NewSize is set to something the new code will
>> never shrink it.
>> Regarding testing, Erik H will soon push his fixes for one of the HeapSize
>> tests. I will make sure nothing is broken with the improved test and also see
>> if we can add some test case to it to test the NewSize.
>>> Anyways, looks good to me.
>>> Ship it!
>> Thanks for reviewing thishttp://cr.openjdk.java.net/~sjohanss/8033426/webrev.01/.
>>> Stefan Johansson skrev 3/2/14 4:40 PM:
>>>> Can I have a couple of reviews for this enhancement:
>>>> Currently the initial young generation size is the same as the minimum. If not
>>>> using large pages this will be 1.5M for the default collector regardless of how
>>>> large the whole heap is. The proposed change is to scale the initial young size
>>>> the same way the maximum young size is scaled, using the NewRatio parameter.
>>>> This change has been out on review as part of the fix for JDK-8028498, but
>>>> discussions we have decided take another approach for that bug.
>>>> * JPRT
>>>> * GC tests in jtreg
More information about the hotspot-gc-dev