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

[Bengt Rutisson]

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

Ah. Of course. I give up ;-)

Ship it!
Bengt

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