On Sat, 20 Apr 2024 08:44:45 GMT, Lei Zaakjyu <d...@openjdk.org> wrote:
>> follow up 8267941
>
> Lei Zaakjyu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> also tidy up
Indentation issues. I will run the higher tier SA tests just for verification.
There are still more classes related to `HeapRegion` that need the G1 prefix.
Not entirely sure they should be changed with this change (probably not
exhaustive) but it would be desirable to change them as well rather sooner than
later:
HeapRegionClosure
HeapRegionRange
HeapRegionIndexClosure
HeapRegionRemSet
HeapRegionSetBase
HeapRegionSetChecker
HeapRegionSet
FreeRegionListIterator
FreeRegionList
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 157:
> 155:
> 156: G1HeapRegion* G1CollectedHeap::new_heap_region(uint hrs_index,
> 157: MemRegion mr) {
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 166:
> 164: HeapRegionType type,
> 165: bool do_expand,
> 166: uint node_index) {
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 999:
> 997: size_t aligned_expand_bytes =
> ReservedSpace::page_align_size_up(expand_bytes);
> 998: aligned_expand_bytes = align_up(aligned_expand_bytes,
> 999: G1HeapRegion::GrainBytes);
Just use a single line, otherwise indent correctly.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1044:
> 1042: ReservedSpace::page_align_size_down(shrink_bytes);
> 1043: aligned_shrink_bytes = align_down(aligned_shrink_bytes,
> 1044: G1HeapRegion::GrainBytes);
Indentation/use single line.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2862:
> 2860: HeapRegionType::Eden,
> 2861: false /* do_expand */,
> 2862: node_index);
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2916:
> 2914: type,
> 2915: true /* do_expand */,
> 2916: node_index);
Indentation
src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 396:
> 394: HeapRegionType type,
> 395: bool do_expand,
> 396: uint node_index = G1NUMA::AnyNodeIndex);
Indentation
src/hotspot/share/gc/g1/g1CollectionSetChooser.hpp line 43:
> 41: public:
> 42: static size_t mixed_gc_live_threshold_bytes() {
> 43: return G1HeapRegion::GrainBytes * (size_t)
> G1MixedGCLiveThresholdPercent / 100;
Suggestion:
return G1HeapRegion::GrainBytes * (size_t)G1MixedGCLiveThresholdPercent /
100;
Pre-existing
src/hotspot/share/gc/g1/g1HeapRegionManager.cpp line 537:
> 535: // committed, expand at that index.
> 536: for (uint curr = reserved_length(); curr-- > 0;) {
> 537: G1HeapRegion *hr = _regions.get_by_index(curr);
Suggestion:
G1HeapRegion* hr = _regions.get_by_index(curr);
Pre-existing
src/hotspot/share/gc/g1/g1HeapRegionManager.cpp line 805:
> 803: FreeRegionList *free_list = worker_freelist(worker_id);
> 804: for (uint i = start; i < end; i++) {
> 805: G1HeapRegion *region = _hrm->at_or_null(i);
Suggestion:
G1HeapRegion* region = _hrm->at_or_null(i);
src/hotspot/share/gc/g1/g1HeapRegionSet.hpp line 248:
> 246: private:
> 247: FreeRegionList* _list;
> 248: G1HeapRegion* _curr;
Suggestion:
G1HeapRegion* _curr;
src/hotspot/share/gc/g1/g1HeapVerifier.cpp line 201:
> 199: G1CollectedHeap* _g1h;
> 200: size_t _live_bytes;
> 201: G1HeapRegion *_hr;
Suggestion:
G1HeapRegion* _hr;
pre-existing
src/hotspot/share/gc/g1/g1HeapVerifier.cpp line 205:
> 203:
> 204: public:
> 205: VerifyObjsInRegionClosure(G1HeapRegion *hr, VerifyOption vo)
Suggestion:
VerifyObjsInRegionClosure(G1HeapRegion* hr, VerifyOption vo)
src/hotspot/share/gc/g1/vmStructs_g1.hpp line 38:
> 36:
> \
> 37: static_field(G1HeapRegion, GrainBytes, size_t)
> \
> 38: static_field(G1HeapRegion, LogOfHRGrainBytes, uint)
> \
Suggestion:
static_field(G1HeapRegion, GrainBytes, size_t) \
static_field(G1HeapRegion, LogOfHRGrainBytes, uint) \
src/hotspot/share/gc/g1/vmStructs_g1.hpp line 44:
> 42: nonstatic_field(G1HeapRegion, _top, HeapWord* volatile)
> \
> 43: nonstatic_field(G1HeapRegion, _end, HeapWord* const)
> \
> 44: volatile_nonstatic_field(G1HeapRegion, _pinned_object_count, size_t)
> \
Suggestion:
nonstatic_field(G1HeapRegion, _type, HeapRegionType) \
nonstatic_field(G1HeapRegion, _bottom, HeapWord* const) \
nonstatic_field(G1HeapRegion, _top, HeapWord* volatile) \
nonstatic_field(G1HeapRegion, _end, HeapWord* const) \
volatile_nonstatic_field(G1HeapRegion, _pinned_object_count, size_t) \
src/hotspot/share/gc/g1/vmStructs_g1.hpp line 96:
> 94: declare_type(G1CollectedHeap, CollectedHeap)
> \
> 95:
> \
> 96: declare_toplevel_type(G1HeapRegion)
> \
Suggestion:
declare_toplevel_type(G1HeapRegion) \
src/hotspot/share/gc/g1/vmStructs_g1.hpp line 106:
> 104:
> \
> 105: declare_toplevel_type(G1CollectedHeap*)
> \
> 106: declare_toplevel_type(G1HeapRegion*)
> \
Suggestion:
declare_toplevel_type(G1HeapRegion*) \
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/gc/g1/G1HeapRegionTable.java
line 93:
> 91: }
> 92:
> 93: private class HeapRegionIterator implements Iterator<G1HeapRegion> {
Should probably be `G1HeapRegionIterator`
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/HeapSummary.java line
90:
> 88: printValMB("MaxMetaspaceSize = ",
> getFlagValue("MaxMetaspaceSize", flagMap));
> 89: if (heap instanceof G1CollectedHeap) {
> 90: printValMB("G1HeapRegionSize = ",
> G1HeapRegion.grainBytes());
Suggestion:
printValMB("G1HeapRegionSize = ", G1HeapRegion.grainBytes());
-------------
Changes requested by tschatzl (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2013971784
PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2014068153
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574262994
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574263232
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574264505
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574264972
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574266207
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574266517
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574267075
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574271446
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574277670
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574278045
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574280865
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574281852
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574282056
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574293124
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574293556
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574293917
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574294107
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574295929
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1574297949
