RFR: 8066875: VirtualSpace does not use large pages

[Erik Helin]

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