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

[Stefan Karlsson]

Here's the fixes for the issues I agreed with you on:
http://cr.openjdk.java.net/~stefank/8024547/webrev.01.delta/

Here's the full patch:
http://cr.openjdk.java.net/~stefank/8024547/webrev.01/

Jon suggested that I should document in globals.hpp that 
UseLargePagesInMetaspace is only used when UseLargePages is true.

thanks,
StefanK

On 10/9/13 8:56 AM, Stefan Karlsson wrote:
>
> 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
>>
>