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