RFR (M): 8219747: Remove g1_ prefix to g1_remset and g1_policy members in G1CollectedHeap

Thomas Schatzl thomas.schatzl at oracle.com
Wed Feb 27 12:44:33 UTC 2019


Hi,

  thanks for your review.

On Wed, 2019-02-27 at 11:41 +0100, Aleksey Shipilev wrote:
> On 2/27/19 11:09 AM, Thomas Schatzl wrote:
> >   can I have reviews for this renaming change that removes some
> > "g1_" prefixes to some G1CollectedHeap members? They are simply
> > unnecessary within the context of G1.
> > 
> > There has also been a "g1_collector_policy" method - after looking
> > at its uses I decided to remove it and add a specific whitebox
> > support method for it to G1CollectedHeap instead.
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8219747
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8219747/webrev/
> 
> Looks good. It is a bit confusing to see the rename is_hetero_heap ->
> is_heap_heterogeneous in this
> changeset that talks about g1_ prefix removal, though.

I removed this and will file separately.

> Nits:
> 
> *) Indentation in g1CollectedHeap.cpp:
> 
>  864   if (policy()->need_to_start_conc_mark("concurrent humongous
> allocation",
>  865                                            word_size)) {
> 
> 4449   policy()->phase_times()-
> >record_fast_reclaim_humongous_time_ms((os::elapsedTime() -
> start_time) * 1000.0,
> 4450
> cl.humongous_objects_reclaimed());
> 
> *) Not sure if you want to rename locals too, like in
> g1ConcurrentMarkThread.cpp:
> 
>  246   G1Policy* g1_policy = g1h->policy();
> 

Fixed (also local names).

http://cr.openjdk.java.net/~tschatzl/8219747/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8219747/webrev.1/ (full)

Still compiles locally.

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list