On Sat, 20 Apr 2024 02:04:20 GMT, Lei Zaakjyu <d...@openjdk.org> wrote:
> follow up 8267941
Functionally the SA changes look good, but I pointed out a few other places
that you might also want to consider doing renames for.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerFinder.java
line 123:
> 121: if (heap instanceof G1CollectedHeap) {
> 122: G1CollectedHeap g1 = (G1CollectedHeap)heap;
> 123: loc.hr = g1.heapRegionForAddress(a);
"g1HeapRegionForAddress"?
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerFinder.java
line 126:
> 124: // We don't assert that loc.hr is not null like we do for the
> SerialHeap. This is
> 125: // because heap.isIn(a) can return true if the address is
> anywhere in G1's mapped
> 126: // memory, even if that area of memory is not in use by a G1
> G1HeapRegion. So there
Suggestion:
// memory, even if that area of memory is not in use by a G1HeapRegion.
So there
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java
line 131:
> 129: }
> 130:
> 131: public G1HeapRegion getHeapRegion() {
Do we want to rename to getG1HeapRegion?
test/hotspot/jtreg/serviceability/sa/TestG1HeapRegion.java line 62:
> 60: agent.attach(Integer.parseInt(pid));
> 61: G1CollectedHeap heap =
> (G1CollectedHeap)VM.getVM().getUniverse().heap();
> 62: G1HeapRegion hr = heap.hrm().heapRegionIterator().next();
"g1HeapRegionIterator"?
-------------
PR Review: https://git.openjdk.org/jdk/pull/18871#pullrequestreview-2013005488
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1573125957
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1573124198
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1573124436
PR Review Comment: https://git.openjdk.org/jdk/pull/18871#discussion_r1573125643
