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

Thomas Schatzl thomas.schatzl at oracle.com
Wed May 15 09:50:37 UTC 2013


Hi,

On Mon, 2013-05-06 at 14:49 -0700, 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")

Not sure if this CR still needs a reviewer (Jon and Bengt in the thread
below were imo agreeing that it is okay already) - the CR is still
unresolved.

It looks okay :)

Thomas

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