Review request: 8024547: MaxMetaspaceSize should limit the committed memory used by the metaspaces
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Oct 9 06:24:16 PDT 2013
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?
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