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

[Thomas Schatzl]

Hi Jon,

  thanks for your review. Sorry for the small delay, some problems with
pushing the change through jprt.

On Thu, 2015-03-26 at 10:26 -0700, Jon Masamitsu wrote:
> Thomas,
> 
> 
> http://cr.openjdk.java.net/~tschatzl/8058354/webrev.2_to_3/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.cpp.frames.html
> 
> Is rs.size() guaranteed to be "page_size" aligned?  Or might you
> need 1 more page in "size_in_pages".
> 
>    76   vmassert(_committed.size() == 0, "virtual space initialized more than once");
>    77   BitMap::idx_t size_in_pages = rs.size() / page_size;
>    78   _committed.resize(size_in_pages, /* in_resource_area */ false);

rs.size() is always page_size aligned.

> 
> I would find these easier to read if "start" was "start_page".
> 
>   122 bool G1PageBasedVirtualSpace::is_area_committed(size_t start, size_t size_in_pages) const {
>   123   size_t end = start + size_in_pages;
>   124   return _committed.get_next_zero_offset(start, end) >= end;
>   125 }
>   126
>   127 bool G1PageBasedVirtualSpace::is_area_uncommitted(size_t start, size_t size_in_pages) const {
>   128   size_t end = start + size_in_pages;
>   129   return _committed.get_next_one_offset(start, end) >= end;
>   130 }

I think I fixed all "start" parameter names to "start_page", the same
with the "end" local variable.

> http://cr.openjdk.java.net/~tschatzl/8058354/webrev.2_to_3/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.hpp.frames.html
> 
> I would prefer "commit_preferred_pages()" to "commit_full_pages()". The
> "full" in the name lead me to look for something about "full" page in 
> the code.
> This is a throw away comment if "full" makes sense to you.

Done.

> Drop the "full" in the comment.
> 
>    75   // Commit num_pages full pages of _page_size size starting from start. All argument
>    76   // checking has been performed.
>    77   void commit_full_pages(size_t start_page, size_t end_page);
> 
> 
> In the comments is the sentence with "start address" needed? Since
> these are page indexes. 

Done.

> I'd suggest adding a comment
>
> // Helper function for committing memory.

Done.

> 
> Because this code is about committing in different ranges (the whole 
> _page_size
> range and the tail range), when I read "internal" in the name, I thought 
> it had
> to do with committing "internal" pages.  After reading the code and the 
> surrounding
> code I understood the "internal" implied a helper function.
> 
>    71   // Commit the given memory range by using _page_size pages as much as possible
>    72   // and the remainder with small sized pages. The start address must be _page_size
>    73   // aligned.
>    74   void commit_internal(size_t start_page, size_t end_page);
> 
> 
> For
> 
>    78   // Commit the tail area.
>    79   void commit_tail();
> 
> would this be a correct description?
> 
> // Commit space at the high end of the VirtualSpace
> // that is smaller than the preferred page size.
> 
> Please replace with that description if you like it.

Done.

> tail_size is calculated and used in commit_tail()
> 
>   158   char* const aligned_end_address = (char*)align_ptr_down(_high_boundary, _page_size);
>   159   size_t const tail_size = pointer_delta(_high_boundary, aligned_end_address, sizeof(char));
> 
> A similar calculation is done in committed_size()
> 
>   104     char* aligned_high_boundary = (char*)align_ptr_up(_high_boundary, _page_size);
>   105     result -= pointer_delta(aligned_high_boundary, _high_boundary, sizeof(char));
> 
> 
> Maybe calculate the tail size once and save it for later uses.  Or
> have a method to calculate tail_size.

Done.

> Parameter "start" to "start_page"  seems consistent with
> other parameter naming.
> 
> 
>   114   void uncommit(size_t start, size_t size_in_pages);
> 
> 
> Rest looks good.

Thanks a lot.

I const-ified a few additional helper functions too.

Webrev:
http://cr.openjdk.java.net/~tschatzl/8058354/webrev.4/
Diff:
http://cr.openjdk.java.net/~tschatzl/8058354/webrev.3_to_4/

Testing:
jprt

Thanks,
  Thomas