Review request: 8024547: MaxMetaspaceSize should limit the committed memory used by the metaspaces
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Oct 9 06:42:02 PDT 2013
On 10/9/13 3:24 PM, Coleen Phillimore wrote:
> On 10/9/2013 9:19 AM, Stefan Karlsson wrote:
>> I just talked to Jon. He proposed that I should rename
>> UseLargePagesInMetaspace to UseLargePagesMetaspace. What do you think?
>
> Sure, if Jon think so.
>
>> Jon suggested that I should document in globals.hpp that
>> UseLargePagesInMetaspace is only used when UseLargePages is true.
>
> Don't we enforce this in arguments.cpp?
No.
StefanK
>
> Coleen
>
>>
>> diff -r 1ada3c6d8cdd src/share/vm/memory/metaspace.cpp
>> --- a/src/share/vm/memory/metaspace.cpp Wed Oct 09 10:40:12 2013
>> +0200
>> +++ b/src/share/vm/memory/metaspace.cpp Wed Oct 09 15:16:40 2013
>> +0200
>> @@ -362,7 +362,7 @@
>>
>> // Decide if large pages should be committed when the memory is
>> reserved.
>> static bool should_commit_large_pages_when_reserving(size_t bytes) {
>> - if (UseLargePages && UseLargePagesInMetaspace &&
>> !os::can_commit_large_page_memory()) {
>> + if (UseLargePages && UseLargePagesMetaspace &&
>> !os::can_commit_large_page_memory()) {
>> size_t words = bytes / BytesPerWord;
>> bool is_class = false; // We never reserve large pages for the
>> class space.
>> if (MetaspaceGC::can_expand(words, is_class) &&
>> @@ -3054,11 +3054,11 @@
>> void Metaspace::ergo_initialize() {
>> if (DumpSharedSpaces) {
>> // Using large pages when dumping the shared archive is
>> currently not implemented.
>> - FLAG_SET_ERGO(bool, UseLargePagesInMetaspace, false);
>> + FLAG_SET_ERGO(bool, UseLargePagesMetaspace, false);
>> }
>>
>> size_t page_size = os::vm_page_size();
>> - if (UseLargePages && UseLargePagesInMetaspace) {
>> + if (UseLargePages && UseLargePagesMetaspace) {
>> page_size = os::large_page_size();
>> }
>>
>> diff -r 1ada3c6d8cdd src/share/vm/runtime/globals.hpp
>> --- a/src/share/vm/runtime/globals.hpp Wed Oct 09 10:40:12 2013 +0200
>> +++ b/src/share/vm/runtime/globals.hpp Wed Oct 09 15:16:40 2013 +0200
>> @@ -500,8 +500,9 @@
>> develop(bool, LargePagesIndividualAllocationInjectError,
>> false, \
>> "Fail large pages individual
>> allocation") \
>> \
>> - product(bool, UseLargePagesInMetaspace,
>> false, \
>> - "Use large page memory in
>> metaspace") \
>> + product(bool, UseLargePagesMetaspace,
>> false, \
>> + "Use large page memory in the Metaspace.
>> " \
>> + "Only used if UseLargePages is
>> enabled.") \
>> \
>> develop(bool, TracePageSizes,
>> false, \
>> "Trace page size selection and
>> usage.") \
>>
>> I'm going to push this when the flag naming is resolved.
>>
>> thanks,
>> StefanK
>>
>> On 10/9/13 3:04 PM, Coleen Phillimore wrote:
>>>
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list