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

[Erik Helin]

On Tuesday 17 June 2014 14:26:10 PM Stefan Johansson wrote:
> On 2014-06-17 13:15, Erik Helin wrote:
> > Hi Stefan,
> > 
> > sorry for the late reply, please see my comments inline.
> > 
> > On Tuesday 13 May 2014 14:31:10 PM Stefan Johansson wrote:
> >> 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.
> > 
> > Thanks!
> > 
> > On Tuesday 13 May 2014 14:31:10 PM Stefan Johansson wrote:
> >> 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.
> > 
> > I agree that this is a good idea, but I'm not fond of modifying the
> > _page_sizes array. Even though the VM is quite idle when the internal
> > vm tests are running, there is a possibility that threads running
> > concurrently might pick up a strange value from the _page_sizes
> > array. Diagnosing an eventual crash due to this might be tricky :)
> > 
> > What do you think of this?
> 
> I agree, and as we discussed off-line, this kind of tests would be
> easier to write if we had a better unit-testing framework. With that in
> mind I'm ok with leaving the test as is for now.

Thanks,
Erik

> Thanks,
> Stefan
> 
> > On Tuesday 13 May 2014 14:31:10 PM Stefan Johansson wrote:
> >>> 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!
> > 
> > Thanks,
> > Erik
> > 
> >>> 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