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

[Jon Masamitsu]

On 05/08/2014 08:34 AM, 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/

Looks good.

Reviewed.

Jon

>
> 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.
>
> 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.
>
> 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).
>
> 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
>>