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

[Jon Masamitsu]

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);

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 }


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.

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.  I'd suggest adding a comment

// Helper function for committing memory.

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.

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.

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.

Jon

On 03/18/2015 06:05 AM, Thomas Schatzl wrote:
> Hi all,
>
>    I got a few more internal reviews and suggestions for this change, so
> I would like to ask for re-reviews.
>
> http://cr.openjdk.java.net/~tschatzl/8058354/webrev.3/ (full)
> http://cr.openjdk.java.net/~tschatzl/8058354/webrev.2_to_3/ (diff)
>
> Changes:
>
> - changed " SIZE_FORMAT" strings in gclog_or_tty->print_cr() format
> strings to " SIZE_FORMAT " since this is the correct style.
>
> - moved setup of reserved space with the requirements we have to the
> ReservedSpace constructor.
>
> - improve readability: in G1PageBasedVirtualSpace: renamings of
> "actual_size" -> "used_size" parameter, _commit_size -> _page_size
> member
>
> - G1PageBasedVirtualSpace: changed uintptr_t parameter types to more
> fitting size_t
>
> - some refactorings, extracting a few methods to make control flow more
> clear
>
> Testing:
> jprt, aurora
>
> Thanks,
>    Thomas
>
>