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

Tao Mao tao.mao at oracle.com
Mon May 6 21:49:56 UTC 2013


JDK-8007762 and JDK-8007763 are coupled as sort of "chicken-egg" problem.

Reviewers for this webrev, please review JDK-8007763 first. (Search 
mails for "Request for review: 8007763")

Thanks.
Tao

On 3/5/13 8:46 PM, Jon Masamitsu wrote:
> 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
>>>>>>>>>>
>>>>>>>>
>>>>>
>>>>
>>
>>
>



More information about the hotspot-gc-dev mailing list