RFR: 8033426: Scale initial NewSize using NewRatio if not set on command line

Stefan Johansson stefan.johansson at oracle.com
Mon Feb 10 15:56:59 UTC 2014


Thanks for looking at this Jon,

On 2014-02-08 02:44, Jon Masamitsu wrote:
> Stefan,
>
>
> 1043   static void verify_scaled_initial(size_t initial_heap_size) {
> 1044     MarkSweepPolicy msp;
> 1045     msp.initialize_all();
> 1046
> 1047     size_t expected = 
> msp.scale_by_NewRatio_aligned(initial_heap_size);
> 1048     assert(msp.initial_gen0_size() == expected, err_msg("%zu != 
> %zu", msp.initial_gen0_size(), expected));
> 1049   }
>
>
>
> Does the assertion work if you added a check that NewSize was set 
> ergonomically?
>
> assert(!FLAG_IS_ERGO(NewSize) || ...)
>

I like the idea to assert on NewSize being updated, but where 
verify_scaled_initial is used I use FLAG_SET_ERGO to set the flag. So 
even if the value is changed, FLAG_SET_ERGO will be used to update it. 
To verify that NewSize was updated I added the assertion after the other 
one:
+    assert(FLAG_IS_ERGO(NewSize) && NewSize == expected,
+        err_msg("NewSize should have been set ergonomically to %zu, but 
was %zu", expected, NewSize));

Are you fine with this change?

Thanks,
Stefan

> Jon
>
>
> On 2/7/2014 4:31 AM, Stefan Johansson wrote:
>> 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:
>> http://cr.openjdk.java.net/~sjohanss/8033426/webrev.01/
>>
>> Cheers,
>> Stefan
>>
>> 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
>>>> /Jesper
>>>>
>>>>
>>>> Stefan Johansson skrev 3/2/14 4:40 PM:
>>>>> Hi,
>>>>>
>>>>> Can I have a couple of reviews for this enhancement:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8033426
>>>>>
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~sjohanss/8033426/webrev.00/
>>>>>
>>>>> Summary:
>>>>> 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.
>>>>>
>>>>> Note:
>>>>> This change has been out on review as part of the fix for 
>>>>> JDK-8028498, but after
>>>>> discussions we have decided take another approach for that bug.
>>>>>
>>>>> Testing:
>>>>> * JPRT
>>>>> * GC tests in jtreg
>>>>>
>>>>> Thanks,
>>>>> Stefan
>>>
>>
>




More information about the hotspot-gc-dev mailing list