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

[Stefan Karlsson]

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

> 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")        \

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130124/95d0043c/attachment.htm>