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

[Stefan Karlsson]

On 10/9/13 12:17 PM, Bengt Rutisson wrote:
>
> Hi Stefan,
>
> I think think this looks really good.

Thanks.

>
> Below are a couple of minor nits. Feel free to address them or ignore 
> them. I'm fine with you pushing the existing changeset.
>
>
>
> I don't think "special" is a very good name for signaling that we have 
> committed all large pages up front. But since that is the name used in 
> other code I find it a little confusing to suddenly call it 
> "pre_committed" in metaspace.cpp. However, "pre_committed" is a better 
> name, so I'm ok with keeping is_pre_committed().

OK. My motivation to create the is_pre_committed() function was to make 
the code in VirtualSpaceNode::expand_by easier for the casual reader to 
understand.

> But in that case we should probably use it instead of _rs.special() in 
> VirtualSpaceNode::initialize().
>
>  size_t pre_committed_size = _rs.special() ? _rs.size() : 0;

VirtualSpaceNode::is_pre_committed() operates on _virtual_space and not 
_rs, so that won't work.

>
>
> In MetaspaceGC::delta_capacity_until_GC(). Why do we pick min_delta in 
> this case?
>
>   if (delta <= min_delta) {
>     delta = min_delta;
>   }
>
> Why not expand with max_delta for the same reason as a slightly larger 
> delta?
>
>   if (delta <= max_delta) {
>     // Don't want to hit the high water mark on the next
>     // allocation so make the delta greater than just enough
>     // for this allocation.
>     delta = max_delta;
>   }

I don't know. The logic in the file pre-exists this patch.

>
>
>
> Why did you move Metadebug::deallocate_chunk_a_lot(this, 
> grow_chunks_by_words); out into it's own if section?
>
>
>   if (next != NULL) {
>     Metadebug::deallocate_chunk_a_lot(this, grow_chunks_by_words);
>   }
>
>   MetaWord* mem = NULL;
>
>   // If a chunk was available, add it to the in-use chunk list
>   // and do an allocation from it.
>   if (next != NULL) {
>     // Add to this manager's list of chunks in use.
>     add_chunk(next, false);
>     mem = next->allocate(word_size);
>   }
>
>   return mem;

I wanted to separate out the debugging code to make it easier to read 
the real allocation code in the if-statement.

thanks,
StefanK
>
> Thanks,
> Bengt
>
>
>
> On 10/8/13 8: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
>