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

Wed May 22 09:35:43 UTC 2013

Hi Tao,

I think John and Thomas already approved this. But since I was involved 
earlier in this thread I'd just like to say that I also think it looks ok.

Thanks,
Bengt

On 5/21/13 1:53 AM, Tao Mao wrote:
> Thank you all for looking at this code.
>
> After JDK-8007763, it's easier to validate this refactoring.
>
> Please review the new webrev.
> http://cr.openjdk.java.net/~tamao/8007762/webrev.03/
>
> Thanks
> Tao
>
> On 5/15/13 4:51 PM, John Cuthbertson wrote:
>> Hi Tao,
>>
>> adaptiveSizePolicy.cpp:470
>> Shouldn't the name of the routine in the print be 
>> AdaptiveSizePolicy::check_gc_overhead_limit?
>>
>> Other than that, the changes look OK to me.
>>
>> JohnC
>>
>> On 5/6/2013 2:49 PM, Tao Mao wrote:
>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>