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

[Erik Helin]

On Wednesday 18 June 2014 09:25:18 AM Jon Masamitsu wrote:
> 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.

Thanks!

Erik

> 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