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

[Bengt Rutisson]

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

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