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

[Coleen Phillimore]

Thanks - fine on all issues, ship it!
Coleen

On 10/9/2013 5:15 AM, Stefan Karlsson wrote:
> 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
>>>
>>
>