RFR: 8027915: TestParallelHeapSizeFlags fails with unexpected heap size on sparcv9

[Stefan Johansson]

On 2014-05-08 17:34, Erik Helin wrote:
> Hi Stefan,
>
> thanks for your thorough review! Please see new webrev at:
> http://cr.openjdk.java.net/~ehelin/8027915/webrev.01/
>
> and incremental webrev at:
> http://cr.openjdk.java.net/~ehelin/8027915/webrev.00-01/
>
> and comments inline!
>
> On 2014-05-06 13:12, Stefan Johansson wrote:
>> Hi Erik,
>>
>> Thanks for making page_size_for_region more strict. A few comments:
>>
>> src/share/vm/runtime/os.cpp
>>
>> With this fix page_size_for_region doesn't care about alignment at all
>> (old version wasn't much better) and to me that feels wrong. I would
>> expect to get a page size that the given region sizes is aligned to, but
>> I might be missing some use case where that isn't needed.
>
> Agree, page_size_for_region should check alignment for large pages. 
> Added fix and regression tests.
Good fix.

Regarding the testing, I looked at the code and the _page_sizes array is 
public. You could start of the testing with recording all "real" values 
and then set up an array that allows more testing. It would be nice to 
have a case in the alignment test that makes sure for example that for a 
10M region you can get 2M pages but not 4M pages. If doing this you need 
to make sure the values in the _page_sizes array are set back to normal 
after the test. This would also allow the test to run even if 
UseLargePages isn't set.

>
> On 2014-05-06 13:12, Stefan Johansson wrote:
>> src/share/vm/gc_implementation/parallelScavenge/generationSizer.cpp
>>
>> For the heap we want both min and max to be a multiple of the page size
>> so we shoud call page_size_for_region for both _min_heap_byte_size and
>> _max_heap_byte_size and use the smallest of the two returned page sizes.
>
> Agree, fixed.
Great!
>
> On 2014-05-06 13:12, Stefan Johansson wrote:
>> src/share/vm/memory/heap.cpp
>>
>> The current patch will not change the default behavior so I'm fine with
>> just calling page_size_for_region once using reserved_size, but someone
>> with more insight might know if we should handle it the same way as the
>> generationSizer and use both committed and reserved.
>
> Since I'm not sure how the compiler guys want to handle this, I opted 
> for the safe alternative and updated the patch to be backwards 
> compatible with the old code (except slightly more strict).
I agree that this is probably the correct solution, but it would be nice 
to hear what the compiler guys think. As you say, this is a little more 
strict and there might be cases where we now stop getting large pages 
where we used to before.

To summarize, I think the changes are good but wouldn't mind some 
improved tests for it.

Stefan
>
> Thanks,
> Erik
>
>> Thanks,
>> Stefan
>>
>>
>> On 2014-05-06 10:44, Erik Helin wrote:
>>> Hi all,
>>>
>>> this patch changes
>>> os::page_size_for_region(min_size, max_size, min_pages) to always
>>> guarantee that the returned page size <= max_size / min_pages. Prior
>>> to this patch, page_size_for_region could return a page size larger
>>> than max_size / min_pages. This can cause some unexpected behavior
>>> for code using page_size_for_region, such as the following regression
>>> tests:
>>>
>>> http://cr.openjdk.java.net/~ehelin/8027915/regression-test/
>>>
>>> As the test shows, if you have a 2 MB region and you must use at least
>>> 2 pages, you would still get back 2 MB as the page size on a Linux
>>> machine that supports 2 MB large pages. The reason for this is
>>> explained in a comment above the function in os.hpp:
>>>
>>> // The current implementation ignores min_pages if a larger page
>>> // size is an exact multiple of both region_min_size and
>>> // region_max_size.  This allows larger pages to be used when doing
>>> // so would not cause fragmentation; in particular, a single page can
>>> // be used when region_min_size == region_max_size == a supported page
>>> // size.
>>>
>>> Reducing fragmentation of large pages is good but the varying return
>>> value makes it very hard to depend on os::page_size_for_region in
>>> calculations (for example when sizing the heap).
>>>
>>> This patch removes the fragmentation prevention mechanism, simplifies
>>> the function and adds a regression tests:
>>>
>>> http://cr.openjdk.java.net/~ehelin/8027915/webrev.00/
>>>
>>> Testing:
>>> - JPRT
>>>
>>> Thanks,
>>> Erik
>>