Review request: 8024547: MaxMetaspaceSize should limit the committed memory used by the metaspaces
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Oct 9 06:19:23 PDT 2013
I just talked to Jon. He proposed that I should rename
UseLargePagesInMetaspace to UseLargePagesMetaspace. What do you think?
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