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

[Jon Masamitsu]

Thomas,

Thanks for the changes.  Looks good.

Reviewed.

Jon

On 4/1/2015 5:31 AM, Thomas Schatzl wrote:
> 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
>
>