Review request: 8024547: MaxMetaspaceSize should limit the committed memory used by the metaspaces

[Coleen Phillimore]

In metaspace.cpp:

*+ static bool should_reserve_large_pages(size_t bytes) {*
*+   if (UseLargePages && UseLargePagesInMetaspace && !os::can_commit_large_page_memory()) {*

Isn't UseLargePagesInMetaspace true only if UseLargePages is true?

I like assert_size_is_aligned, assert_ptr_is_aligned now you get more 
info if assert hits too.

Below, this comment doesn't seem to match the function name even though 
inside you test whether you can commit large pages (vs. reserving large 
pages).  Putting a comment about the restriction about 
can_commit_large_pages should be inside should_reserve_large_pages.   
It's sort of surprising there when you only look once.  It looked like a 
bug at first.

*+      // Decide if large pages should be commmitted when the memory is reserved.*
*+     bool large_pages = should_reserve_large_pages(bytes);*
*+*
*+     _rs = ReservedSpace(bytes, Metaspace::reserve_alignment(), large_pages);*


1167     assert(false, "vs_word_size should always be at least _reserve_alignement large.");


Typeo.

   if (is_class()) {
     assert(false, "We currently don't support more than one 
VirtualSpace for"
                   " the compressed class space. The initialization of the"
                   " CCS uses another code path and should not hit this 
path.");
     return false;
   }

Why not just assert(!is_class(), "with the message")  It would take less 
lines so I can get the rest of the function on the page.

And the assert below it?  or guarantee() if you are worried about this 
happening in production (I hope not!)

Metachunk* VirtualSpaceList::get_initialization_chunk(size_t 
chunk_word_size,
                                                       size_t chunk_bunch) {
   // Get a chunk from the chunk freelist
   Metachunk* new_chunk = get_new_chunk(chunk_word_size,
                                        chunk_word_size,
                                        chunk_bunch);
   return new_chunk;
}

Can we just call get_new_chunk from the other get_initialization_chunk 
and remove this?  There are so many functions with the same names, and 
it looks like this used to do more by the comment.

I don't have any other comments now.  I think this looks really good and 
thank you for resolving the MaxMetaspaceSize problem!

Coleen

On 10/08/2013 02:21 AM, Stefan Karlsson wrote:
> Please, review this patch to limit the committed Metaspace memory 
> against the MaxMetaspaceSize flag.
>
> http://cr.openjdk.java.net/~stefank/8024547/webrev.00
>
> 8024547: MaxMetaspaceSize should limit the committed memory used by 
> the metaspaces
> Reviewed-by: TBD1, TBD2
>
> To simplify the code, the patch is strict about the alignments used to 
> commit and reserve memory in the metaspaces. The Metaspace 
> VirtualSpaces always have addresses and sizes that are aligned against 
> Metaspace::reserve_alignment(). The reserve alignment is 
> os::vm_allocation_granularity() if small pages are used or 
> os::large_page_size() if large pages are used in the Metaspace. The 
> memory is always committed in regions that are size aligned against 
> Metaspace::commit_alignment(). The commit alignment is os:page_size() 
> if small pages are used or os::large_page_size() if late pages are 
> used in the Metaspace. The user can specify the flag 
> -XX:+UseLargePagesInMetaspace to turn on large pages in the metaspaces.
>
> The flag initialization was moved out of the CollectorPolicy class. 
> The Metaspace specific flags have been changed to be commit/reserve 
> aligned instead of using heap specific alignments.
>
> The output from PrintHeapAtGC has been changed. The redundant "data 
> space" section has been removed. All of used, capacity, committed and 
> reserved are printed. Example:
>  Metaspace       used 7644K, capacity 7700K, committed 7808K, reserved 
> 1056768K
>   class space    used 804K, capacity 822K, committed 896K, reserved 
> 1048576K
>
> Thanks go out to the engineers who have helped out with this bug: 
> Erik, Mikael and Bengt for helping out with parts of the code and 
> testing. Bengt, Coleen, Jesper and Jon for pre-reviewing the changes 
> and providing early feedback.
>
> thanks,
> StefanK