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

[Tao Mao]

Hi Bengt,

As I understand (since I've investigated the resizing issue for a while) 
and, in fact, if you look at the comments you quoted, the routine 
compute_generations_free_space() just does the following:

input(stats of young & old spaces) -> output(calculated eden & promo sizes).

I would say, it's valid to name it this way.

In fact, in another CR (8007763, one of the series of CR's you asked me 
to split out), we would like to split compute_generations_free_space() 
into compute_eden* and compute_old*. This also validates the renaming here.

For commenting in psGCAdaptivePolicyCounters.hpp, the comment leads a 
series of routines to update stats for compute_generations_free_space(). 
It has some value to keep it. But, I'm ok either way for the choice here 
doesn't matter too much.

Thanks.
Tao


On 2013/3/4 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/20130304/33641bdc/attachment.htm>