RFR (L): 8058354: SPECjvm2008-Derby -2.7% performance regression on Solaris-X64 starting with 9-b29

[Jon Masamitsu]

Thomas,

Looks good.  Some throw-away suggestions.

Reviewed.

http://cr.openjdk.java.net/~tschatzl/8058354/webrev/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.cpp.frames.html

>    75   BitMap::idx_t size_in_pages = align_size_up(rs.size(), commit_size) / commit_size;

Can you use round_to() in "share/vm/utilities/globalDefinitions.hpp"

> // returns integer round-up to the nearest multiple of s (s must be a 
> power of two)
> inline intptr_t round_to(intptr_t x, uintx s) {
>   #ifdef ASSERT
>     if (!is_power_of_2(s)) basic_fatal("s must be a power of 2");
>   #endif
>   const uintx m = s - 1;
>   return mask_bits(x + m, ~m);
> }

or change the name "size_in_pages" to "size_in_commit_pages"?

http://cr.openjdk.java.net/~tschatzl/8058354/webrev/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.cpp.frames.html

Is  commit_int()

>   143 void G1PageBasedVirtualSpace::commit_int(char* start, char* end) {


short for commit_internal().  If yes, I find commit_internal() clearer.

http://cr.openjdk.java.net/~tschatzl/8058354/webrev/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.hpp.frames.html

>    81   // Returns the address of the given page index ranging from 0..size_in_pages-1.
>    82   char*  page_end(uintptr_t index);

I'd change comment to

// Returns the address of the end of the page given the page index in range
// 0..size_in_pages-2.  For last page return _high_boundary.

   37 // We only support reservations which base address is aligned to a given commit
   38 // size. The length of the area managed need not commit size aligned (but OS default
   39 // page size aligned) because some OSes cannot provide a an os_commit_size aligned
   40 // reservation without also being size-aligned. Any tail area is committed using OS
   41 // small pages.

Is this below accurate?  If so, I'd suggest it.

// For systems that only allow commits of memory in a given size (always 
greater than the page size),
// the base address is required to be aligned to that commit size.  The 
actual size requested need not
// be aligned to the commit size, but the size of the reservation passed 
to the system may be rounded
// up to the commit size.   Any fragment (less than the commit size) of 
the actual size at the tail
// of the request, will be committed using OS small pages.

http://cr.openjdk.java.net/~tschatzl/8058354/webrev/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html

Please verify that this should be a call to the _unaligned version.

> 1840   size_t const commit_size = os::page_size_for_region_unaligned(size, 1);


Jon

On 1/29/2015 2:30 AM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for the following change that fixes the use of
> large pages for auxiliary data on G1?
>
> In JDK-8038423 there has been a large change in how G1 handles virtual
> memory, and overlooked that auxiliary data may use large pages. This
> caused some performance regressions after introducing that build.
>
> This change fixes this problem: G1 is now more flexible in using large
> pages: particularly auxiliary data that often is not sized to multiples
> of (large) page size suffers from that. By allowing the virtual space
> implementation to use small pages on the tail (upper) end of the virtual
> space, everything else can use large pages.
>
> There is one limitation to that: the start address of the used virtual
> spaces must be aligned to large page size to use large pages in
> auxiliary data. This is to simplify commit and uncommit within the
> regions, since very small areas in the auxiliary data can map to large
> areas in the heap (e.g. for the BOT, at 4k page size, one page maps to
> 2M of memory, 2M pages map to 1G of memory).
>
> The problem is, if the start address of such auxiliary data were not
> aligned to requested page size, we would potentially need to split
> neighbouring large pages if we tried to uncommit one.
>
> I.e. some ascii art showing the problem.
>
> AAAAAA AAAAAA AAAAAA  // heap area, each AAAAAA is a single region
>     |      |      |    // area covered by auxilary pages
>        1       2       // auxiliary data pages
>
> So if auxiliary data pages were unaligned, so that they correspond to
> uneven multiples of the heap, when uncommitting e.g. the second region
> (second set of AAAAAA), we would have to split the auxiliary data pages
> 1 and 2 into smaller ones.
>
> That does not seem to be a good tradeoff in complexity, given that the
> waste is at most one large page in reserved space (and unfortunately,
> due to the Linux large page implementation also in actually used space).
>
> Changes in detail containing some additional fixes:
>   - page selection corresponds to other collectors, i.e. if some
> auxiliary data covers at least one large page, try to use large pages if
> available.
>   - fix CMBitMap::compute_size() to align to alignment granularity (this
> has not been a real problem because the actual size is always a multiple
> of that)
>   - allow (very restricted) mixed use of small and large pages in the G1
> heap.
>   - pass on alignment hints to os::commit()
>   - some refactoring extracting out the code to reserve auxiliary memory
> structures
>
> With these changes, performance when using large pages is at least as
> good as before 9b29.
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8058354/webrev/
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8058354
>
> Testing:
> jprt, specjbb*, specjvm*, vm.quick.testlist, some large benchmarks, test
> case
>
> Thanks,
>    Thomas
>
>