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

[Bengt Rutisson]

Hi Stefan,

On 10/9/13 2:16 PM, Stefan Karlsson wrote:
> 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.

OK.

>
>> 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.

Right, but _virtual_space._special is initialized to _rs.special in 
VirtualSpace::initialize_with_granularity(), so shouldn't they be the same?

>
>>
>>
>> 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.

OK. It just looks odd to me. But it should not be fixed in this patch I 
guess.

>
>>
>>
>>
>> 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.

That's fine. I was just curious.

And as I said earlier, I'm fine with this being pushed as is.

Bengt

>
> 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
>>
>