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