RFR: 8066875: VirtualSpace does not use large pages

[Tobias Hartmann]

Hi Erik,

looks good (not a reviewer).

Thanks,
Tobias

On 15.12.2014 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/
> 
> 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
>>