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

Tue Mar 5 09:33:06 UTC 2013

Hi Tao,

First, I realize that I am very late with this feedback. Sorry for that.

On 3/5/13 8:43 AM, Tao Mao wrote:
> 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 disagree. The method has lots of side effects. It does not just take 
some input and produce some output. It updates the global state in many 
ways. For example through the statistics gathering with at least 10-20 
instance variables such as:

_avg_base_footprint
_avg_young_live
_avg_eden_live
_avg_old_live
_decide_at_full_gc
_change_old_gen_for_maj_pauses
_change_young_gen_for_maj_pauses

..and more

It also logs messages associated with PrintAdaptiveSizePolicy.

In fact I would consider the calculation of free size as a very small 
part of what the method does.

The name compute_generations_free_space() indicates to me that it would 
be a simple input->output method just as you suggested. But in that case 
I would expect that it would be safe to call it several times in a row. 
This is not the case with the current implementation. We can only call 
it exactly once at the exact right moment in the during GC.

So, again, I'm fine with leaving the name untouched as 
compute_generation_free_space(). But if we should change it I think we 
should make a more meaningful change than what you proposed.

Bengt

>
> 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/20130305/e44ff54a/attachment.htm>