Request for review: 8007762: Rename a bunch of methods in size policy across collectors

[Jon Masamitsu]

Tao,

Bengt was ok with leaving the name compute_generation_free_space()
as is so I think we should do that.   Bengt's point about methods
needing better names probably applies to several methods and
I'd rather consider a renaming scheme after your refactoring
changes have been integrated.

Jon

On 3/5/2013 10:57 AM, Bengt Rutisson wrote:
>
> Jon,
>
> On 3/5/13 5:31 PM, Jon Masamitsu wrote:
>> Bengt,
>>
>> Would a less specific name such as "update" be
>> better?  Something that says to the reader
>> "you have to go look at what the method is
>> doing because a name is not going to be
>> descriptive enough".
>
> Yes, I think so. Something that indicates that this will have side 
> effects. In my earlier email I suggested 
> update_adaptive_size_values(). If you come up with something better 
> I'm happy too. :)
>
> Bengt
>
>>
>> Jon
>>
>> On 03/04/13 22:56, Bengt Rutisson wrote:
>>>
>>> Hi Tao,
>>>
>>> The method PSAdaptiveSizePolicy::compute_generation_free_space() is 
>>> a monster method. It does all kinds of things. Its comment kind of 
>>> reveals this:
>>>
>>>  339   // Calculates optimial free space sizes for both the old and 
>>> young
>>>  340   // generations.  Stores results in _eden_size and _promo_size.
>>>  341   // Takes current used space in all generations as input, as well
>>>  342   // as an indication if a full gc has just been performed, for 
>>> use
>>>  343   // in deciding if an OOM error should be thrown.
>>>
>>> So, I think that if we should rename it we should give it a name 
>>> that reflects this. The current name is not good, but I don't think 
>>> compute_generations_free_space() is much better. I would suggest to 
>>> either leave it unchanged or change to a more descriptive name.
>>>
>>> Maybe gether_adapitve_size_statistics() or 
>>> update_adaptive_size_values(). Not very happy with those names 
>>> either, but at least they indicate that ther eis more than just free 
>>> space calculations in there.
>>>
>>>
>>> In psGCAdaptivePolicyCounters.hpp you updated a method name that has 
>>> been commented out. I would suggest removing it rather than updating 
>>> it.
>>>
>>> Thanks,
>>> Bengt
>>>
>>> On 3/5/13 1:46 AM, Tao Mao wrote:
>>>> Oops...sent to open list.
>>>>
>>>> Thanks.
>>>> Tao
>>>>
>>>> On 3/4/2013 4:38 PM, Tao Mao wrote:
>>>>> This is the proposed final webrev, with copyright dated 
>>>>> appropriately.
>>>>> http://cr.openjdk.java.net/~tamao/8007762/webrev.02/
>>>>>
>>>>> Jon,
>>>>> A patch is rendered below. Please take the patch and help push the 
>>>>> changeset.
>>>>> http://cr.openjdk.java.net/~tamao/8007762/webrev.02/8007762RenameMethodsInSizePolicy.patch 
>>>>>
>>>>>
>>>>> Thank you.
>>>>> Tao
>>>>>
>>>>> On 2/26/13 8:37 AM, Jon Masamitsu wrote:
>>>>>> Thanks.
>>>>>>
>>>>>> Looks good.
>>>>>>
>>>>>> Jon
>>>>>>
>>>>>> On 02/25/13 11:09, Tao Mao wrote:
>>>>>>> A new webrev is updated.
>>>>>>> Thanks.
>>>>>>> Tao
>>>>>>>
>>>>>>> On 2/18/2013 7:46 AM, Jon Masamitsu wrote:
>>>>>>>> Tao,
>>>>>>>>
>>>>>>>> Why this change from resize_old_gen() to
>>>>>>>> resize_tenured_gen()?  There are many
>>>>>>>> variable names and comments referring to
>>>>>>>> "old gen" still in ParallelScavengeHeap.
>>>>>>> Reverted.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~tamao/8007762/webrev.00/src/share/vm/gc_implementation/parallelScavenge/parallelScavengeHeap.cpp.udiff.html 
>>>>>>>>
>>>>>>>>
>>>>>>>> Jon
>>>>>>>>
>>>>>>>> On 02/14/13 11:18, Tao Mao wrote:
>>>>>>>>> 8007762: Rename a bunch of methods in size policy across 
>>>>>>>>> collectors
>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8007762
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~tamao/8007762/webrev.00/
>>>>>>>>>
>>>>>>>>> changeset:
>>>>>>>>> The motivation of this change is associated with CR 7098155 
>>>>>>>>> (https://jbs.oracle.com/bugs/browse/JDK-7098155). For this, we 
>>>>>>>>> need to split compute_generations_free_space into two routine 
>>>>>>>>> (one for compute_eden_space_size and the other for 
>>>>>>>>> compute_tenured_generation_free_space) in order to save some 
>>>>>>>>> overhead when we want to compute only one of the two. The next 
>>>>>>>>> step is to split 
>>>>>>>>> PSAdaptiveSizePolicy::compute_generations_free_space(), which 
>>>>>>>>> will be resolved in CR 8007763.
>>>>>>>>>
>>>>>>>>> To unify the naming, a bunch of routines are renamed here.
>>>>>>>>> (1) compute_*:
>>>>>>>>>       compute_survivor_space_size_and_threshold()
>>>>>>>>>       compute_generations_free_space() = 
>>>>>>>>> compute_eden_space_size() + 
>>>>>>>>> compute_tenured_generation_free_space()
>>>>>>>>> (2) resize_*_gen:
>>>>>>>>>       resize_young_gen()
>>>>>>>>>       resize_tenured_gen()
>>>>>>>>> (3) rename judging routine resize_geneneration() to 
>>>>>>>>> need_resize() to avoid ambiguity
>>>>>>>>>
>>>>>>>>> testing:
>>>>>>>>> purely renaming; passed JPRT
>>>>>>>>>
>>>>>>>
>>>>
>>>
>
>