RFR: 8066875: VirtualSpace does not use large pages

[Stefan Karlsson]

On 2015-01-15 16:36, Erik Helin wrote:
> On 2015-01-08, Stefan Karlsson wrote:
>> Hi Erik,
> Hi StefanK,
>
> thanks for reviewing!
>
>> On 2014-12-15 15:36, Erik Helin wrote:
>>> On 2014-12-12, Albert Noll wrote:
>>>> Hi Erik,
>>>>
>>>> thanks a lot for taking care of this. I have one minor comment (not a
>>>> reviewer):
>>>>
>>>> 1418 size_t os::largest_page_size_less_than(size_t sz) {
>>>> 1419   if (UseLargePages) {
>>>> 1420     // The page sizes are sorted descendingly.
>>>> 1421     for (size_t i = 0; _page_sizes[i] != 0; ++i) {
>>>> 1422       if (_page_sizes[i] <= sz) {
>>>> 1423         return _page_sizes[i];
>>>> 1424       }
>>>> 1425     }
>>>> 1426   }
>>>> 1427   return vm_page_size();
>>>> 1428 }
>>>>
>>>> The function name suggests that the function returns a page size that is
>>>> *less* than the value in the argument (sz).
>>>> However, in line 1422 you check for '<=' . I think you should either fix the
>>>> name of the function or the check in line 1422.
>>> Yeah, I wasn't too fond of that name either. I discussed some potential
>>> names with the guys in the office and ended up with:
>>> - os::page_size_for_region_aligned
>>> - os::page_size_for_region_unaligned
>>>
>>> os::page_size_for_region_aligned and os::page_size_for_region_unaligned
>>> takes the same number of parameters, a region size and minimum number of
>>> pages. The difference is that page_size_for_region_aligned guarantees
>>> that returned page size divides the given region size, whereas
>>> page_size_for_region_unaligned does not guarantee this.
>>>
>>> These names were chosen because a call to page_size_for_region_unaligned
>>> indicates that the surrounding code must handle any alignment on its
>>> own.
>>>
>>> Webrevs:
>>> - incremental: http://cr.openjdk.java.net/~ehelin/8066875/webrev.00-01/
>>> - full: http://cr.openjdk.java.net/~ehelin/8066875/webrev.01/
> New webrevs (with fixes to comments below):
> - incremental: http://cr.openjdk.java.net/~ehelin/8066875/webrev.01-02/

Looks good.

Thanks,
StefanK

> - full: http://cr.openjdk.java.net/~ehelin/8066875/webrev.02/
>
>> http://cr.openjdk.java.net/~ehelin/8066875/webrev.01/src/share/vm/runtime/virtualspace.cpp.udiff.html
>>
>> I think this code should use the unaligned version:
>>
>>   ReservedSpace::ReservedSpace(size_t size) {
>> -  size_t page_size = os::page_size_for_region(size, 1);
>> +  size_t page_size = os::page_size_for_region_aligned(size, 1);
>>     bool large_pages = page_size != (size_t)os::vm_page_size();
>>     // Don't force the alignment to be large page aligned,
>>     // since that will waste memory.
>>     size_t alignment = os::vm_allocation_granularity();
>>     initialize(size, alignment, large_pages, NULL, 0, false);
>>
>> As the comment hints, 'size' might not be large pages aligned, but we might still want to use large pages for the middle part of the reserved memory.
> Agree, fixed.
>
>> http://cr.openjdk.java.net/~ehelin/8066875/webrev.01/src/share/vm/runtime/os.cpp.udiff.html
>>
>> Could this code:
>>
>> -      if (page_size <= max_page_size && is_size_aligned(region_size, page_size)) {
>> +      if (page_size <= max_page_size) {
>> +        if (!must_be_aligned) {
>>           return page_size;
>>         }
>> +        if (is_size_aligned(region_size, page_size)) {
>> +          return page_size;
>> +        }
>> +      }
>>
>> be changed into one return statement?:
>>
>>         if (page_size <= max_page_size) {
>> 	 if (!must_be_aligned || is_size_aligned(region_size, page_size)) {
>>             return page_size;
>>           }
>>         }
> Agree, this reads much nicer!
>
>> http://cr.openjdk.java.net/~ehelin/8066875/webrev.01/src/share/vm/runtime/os.cpp.udiff.html
>>
>> The comments in the tests would love some periods. :)
> Sure, the comments now have periods :)
>
> Thanks,
> Erik
>
>> Thanks,
>> StefanK
>>
>>> Testing:
>>> - JPRT
>>> - Verified that the code cache now uses large pages even if
>>>    ReservedCodeCacheSize is 241 MB (see bug for more details).
>>> - Added new internal vm tests (also run on SPARC machine with large
>>>    pages)
>>> - Run hotspot jtreg tests on SPARC machine with large pages
>>>
>>> Thanks,
>>> Erik
>>>
>>>> Best,
>>>> Albert
>>>>
>>>>
>>>> On 12/10/2014 10:39 AM, Tobias Hartmann wrote:
>>>>> Hi Erik,
>>>>>
>>>>> looks good (not a reviewer).
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>>
>>>>> On 09.12.2014 20:35, Erik Helin wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> the fix for JDK-8049864 [0] made os::page_size_for_region slightly more strict
>>>>>> since the function now demands that the given region_size is size aligned with
>>>>>> respect to the chosen page_size. The reason for doing this was that
>>>>>> os::page_size_for_region was used in two ways:
>>>>>> 1. Give me a suitable page size for this amount of memory
>>>>>> 2. Give me the largest page size for this amount of memory
>>>>>> The fix for JDK-8049864 fixed os::page_size_for_region for the "suitable page
>>>>>> size" scenario, but is too strict for the "largest page size" scenario. This
>>>>>> caused a regression in VirtualSpace::initialize, which only needs the largest
>>>>>> possible page size, since VirtualSpace::initialize_with_granularity takes care
>>>>>> of any alignment issues.
>>>>>>
>>>>>> This patch adds the function os::largest_page_size_less_than and updates
>>>>>> VirtualSpace::initialize to use this new function instead of
>>>>>> os::page_size_for_region.
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~ehelin/8066875/webrev.00/
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8066875
>>>>>>
>>>>>> Testing:
>>>>>> - JPRT
>>>>>> - Verified that the code cache now uses large pages even if
>>>>>>    ReservedCodeCacheSize is 241 MB (see bug for more details).
>>>>>> - Added new internal vm tests (also run on SPARC machine with large
>>>>>>    pages)
>>>>>>
>>>>>> Thanks,
>>>>>> Erik
>>>>>>
>>>>>> [0]: http://hg.openjdk.java.net/jdk9/hs-gc/hotspot/rev/b326a3e8dcab