request for review (s) - 8005452: Create new flags for Metaspace resizing policy

[Stefan Karlsson]

On 01/24/2013 05:35 PM, Jon Masamitsu wrote:
>
>
> On 1/24/2013 5:32 AM, Stefan Karlsson wrote:
>> On 01/24/2013 05:38 AM, Jon Masamitsu wrote:
>>> Coleen,
>>>
>>> Thanks for the review.
>>>
>>> I delete the print at 1013 (instead of moving it) and reverted 
>>> get_new_chunk().  I left in the
>>> print at 1014.
>>>
>>> I have 2 webrevs for  these changes now.
>>
>> Thanks for splitting this into two changes.
>>
>>> Your suggested changes are in
>>>
>>> http://cr.openjdk.java.net/~jmasa/8006815/webrev.00/
>>
>> I don't know if this is a reasonable change or not.
>>
>> Why are you checking if we should expand, before trying to allocate 
>> in the current virtual space?
>>
>>  982   // The next attempts at allocating a chunk will expand the
>>  983   // Metaspace capacity.  Check first if there should be an 
>> expansion.
>>  984   if (!MetaspaceGC::should_expand(this, word_size, 
>> grow_chunks_by_words)) {
>>  985     return next;
>>  986   }
>
> Do you mean that I should put the check should_expand() after
> line 991.  That would be better.

Yes. It seemed like you could return NULL without using all memory in 
the VirtualSpace.

StefanK

>
> Or do you mean that the test should look at the current capacity (as it
> did before) and not at the capacity after the addition of the chunk (for
> comparing to the HWM)?
>
>>  987
>>  988   // Allocate a chunk out of the current virtual space.
>>  989   if (next == NULL) {
>>  990     next = 
>> current_virtual_space()->get_chunk_vs(grow_chunks_by_words);
>>  991   }
>>
>> Shouldn't this line be checking "less than or equal":
>>
>> *!    if (_(_vsl->capacity_words_sum(_) + expansion_word_size_) < 
>> metaspace_size_words ||*
>>         capacity_until_GC() == 0) {
>>       set_capacity_until_GC(metaspace_size_words);
>>       return true;
>>     }
>
> Yes. Fixed.
>
>>
>>> Webrev with the Min/MaxMetaspaceFreeRatio changes is
>>>
>>> http://cr.openjdk.java.net/~jmasa/8005452/webrev.02/
>>
>> This looks good.
>>
>> Though, I think you need to update the descriptions of the new flags:
>> +   product(uintx, MinMetaspaceFreeRatio, 
>> 10,                              \
>> +           "Min percentage of heap free after GC to avoid 
>> expansion")        \
>> + \
>> +   product(uintx, MaxMetaspaceFreeRatio, 
>> 20,                              \
>> +           "Max percentage of heap free after GC to avoid 
>> shrinking")        \
>
> Fixed.
>
> Jon
>>
>> thanks,
>> StefanK
>>
>>>
>>> Jon
>>>
>>> On 1/23/2013 9:01 AM, Coleen Phillimore wrote:
>>>>
>>>> It looks okay except I think passing SpaceManager* to 
>>>> get_new_chunk() is really gross just to do a print out.  I'd rather 
>>>> see the printing at line 1014 moved to 2170 whether you get a new 
>>>> virtual space or not.  There's already a ton of output from 
>>>> TraceMetadataChunkAllocation && Verbose, but you can leave the 
>>>> printing at 1013 so you know which one created a new virtual space.
>>>>
>>>> Coleen
>>>>
>>>>
>>>> On 01/14/2013 10:30 AM, Jon Masamitsu wrote:
>>>>> 8005452: Create new flags for Metaspace resizing policy
>>>>>
>>>>> Previously the calculation of the metadata capacity at which
>>>>> to do a GC (high water mark, HWM) to recover
>>>>> unloaded classes used the MinHeapFreeRatio
>>>>> and MaxHeapFreeRatio to decide on the next HWM.  That
>>>>> generally left an excessive amount of unused capacity for
>>>>> metadata.  This change adds specific flags for metadata
>>>>> capacity with defaults more conservative in terms of
>>>>> unused capacity.
>>>>>
>>>>> Added an additional check for doing a GC before expanding
>>>>> the metadata capacity.  Required adding a new parameter to
>>>>> get_new_chunk().
>>>>>
>>>>> Added some additional diagnostic prints.
>>>>>
>>>>> http://cr.openjdk.java.net/~jmasa/8005452/webrev.00/
>>>>>
>>>>> Thanks.
>>>>
>>
>>