[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: RFR: 8027915: TestParallelHeapSizeFlags fails with unexpected heap size on sparcv9
From:       Stefan Johansson <stefan.johansson () oracle ! com>
Date:       2014-06-24 7:57:52
Message-ID: 53A92F80.7020103 () oracle ! com
[Download RAW message or body]

On 2014-06-23 14:18, Erik Helin wrote:
> Stefan, Jon,
>
> I forgot to remove the "@ignore" line from the actual test,
> TestParallelHeapSizeFlags.java. Please see new webrev at:
> http://cr.openjdk.java.net/~ehelin/8027915/webrev.02/
>
> and the very small (one line) incremental webrev at:
> http://cr.openjdk.java.net/~ehelin/8027915/webrev.01-02/
Good catch, ship it!

StefanJ
>
> Thanks,
> Erik
>
> On Thursday 08 May 2014 17:34:56 PM 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.
>>
>> 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

[prev in list] [next in list] [prev in thread] [next in thread]