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

[Stefan Karlsson]

On 10/9/13 2:37 PM, Bengt Rutisson wrote:
>
> 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?

The code looks like this:
   size_t pre_committed_size = _rs.special() ? _rs.size() : 0;

   bool result = virtual_space()->initialize_with_granularity(_rs, 
pre_committed_size,
Metaspace::commit_alignment());

I can't use _virtual_space->special() since _virtual_space has not been 
initialized at that point.

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

Thanks,
StefanK

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