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

[Jon Masamitsu]

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.

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