RFR: 8252888: Simplify G1MMUTracker class hierarchy [v2]

Kim Barrett kim.barrett at oracle.com
Tue Sep 22 11:57:34 UTC 2020


> On Sep 21, 2020, at 6:02 PM, John Tortugo <github.com+2249648+JohnTortugo at openjdk.java.net> wrote:
> 
>> Relates to: https://bugs.openjdk.java.net/browse/JDK-8252888
>> Tested on: x86_64 - Linux - tier1
>> Need sponsor: yes
>> 
>> Merge `G1MMUTracker` and `G1MMUTrackerQueue` into a single class and make method members non virtual.
> 
> John Tortugo has updated the pull request incrementally with one additional commit since the last revision:
> 
>  Rename MMUTrackerQueue[Element] to MMUTracker
> 
> -------------
> 
> Changes:
>  - all: https://git.openjdk.java.net/jdk/pull/214/files
>  - new: https://git.openjdk.java.net/jdk/pull/214/files/06acb247..136a1ae7
> 
> Webrevs:
> - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=214&range=01
> - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=214&range=00-01
> 
>  Stats: 534 lines in 7 files changed: 263 ins; 263 del; 8 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/214.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/214/head:pull/214
> 
> PR: https://git.openjdk.java.net/jdk/pull/214

Generally good, and this is what I was looking for when I filed the RFE.

Just a few minor nits.

Also, please fix the github PR summary to match the JIRA bug summary, so
s/Simplify/Collapse/ or vice versa.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MMUTracker.hpp
 76   double                _time_slice;
 77   double                _max_gc_time; // this is per time slice

These variable names have been further indented to line up with declarations
some lines later across a large comment block. I think the indentation of
the variable names is not helpful and would prefer just killing it.
Especially since the indentation it is lining up with should probably be
changed (see next comment).

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MMUTracker.hpp 
 91   G1MMUTrackerElem _array[QueueLength];
 92   int                   _head_index;
 93   int                   _tail_index;
 94   int                   _no_entries;

After the name change for the Elem type the variables no longer line up.  I
think the variable indentation isn't helpful and would prefer just killing it.
Some people like lining up variables; I mostly don't.

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MMUTracker.hpp 
 88   // the oldest entry in the event that +G1UseFixedWindowMMUTracker, thus
 89   // potentially violating MMU specs for some time thereafter.

[pre-existing] 
That command line option doesnt seem to exist anymore.  I suggest removing
"in the event that +G1UseFixedWindowMMUTracker".

------------------------------------------------------------------------------
src/hotspot/share/gc/g1/g1MMUTracker.hpp 
114   inline double when_max_gc_sec(double current_time) {

[pre-existing]
inline is redundant with having the definition in the class.

------------------------------------------------------------------------------




More information about the hotspot-gc-dev mailing list