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

[Stefan Karlsson]

On 2013-10-09 00:33, Coleen Phillimore wrote:
>
> 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?

Not in the current version of the code.

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

OK. How about a longer name: 
should_commit_large_pages_when_reserving(bytes), and get rid of/move the 
comment?

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

Thanks.

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

It's just defensive programming. If we end up hitting this code path in 
production builds, the code will still work as it's currently written.

>
>
> 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.

Will do.

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

Thanks, Coleen!

StefanK

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