Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v4]
On Tue, 16 Jan 2024 22:47:04 GMT, Chris Plummer wrote: >> Tom Rodriguez has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Update >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java >> >>Co-authored-by: Andrey Turbanov >> - Update >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java >> >>Co-authored-by: Andrey Turbanov > > test/hotspot/jtreg/serviceability/sa/ClhsdbTestAllocationMerge.java line 58: > >> 56: Map> expStrMap = new HashMap<>(); >> 57: Map> unExpStrMap = new HashMap<>(); >> 58: test.run(theApp.getPid(), cmds, expStrMap, unExpStrMap); > > You can just pass `null` for the two Map args, and no need to import > java.util.HashMap or java.util.Map. This still needs to be taken care of. - PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456380163
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v4]
On Tue, 16 Jan 2024 22:36:55 GMT, Chris Plummer wrote: >> Tom Rodriguez has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Update >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java >> >>Co-authored-by: Andrey Turbanov >> - Update >> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java >> >>Co-authored-by: Andrey Turbanov > > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java > line 1644: > >> 1642: } >> 1643: }, >> 1644: new Command("testdebuginfodecode", "testdebuginfodecode", >> false) { > > This doesn't belong in clhsdb. You can do all of this directly in the test if > you launch it properly. > See for example `TestObjectAlignment.java`. Yes that's much better. I've updated the test and renamed it. Please rereview. - PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456153937
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v4]
On Mon, 15 Jan 2024 16:03:03 GMT, Tom Rodriguez wrote: >> The changes for JDK-8287061 didn't update the SA decoding logic and there >> are other places where the decoding has gotten out of sync with HotSpot. >> Some of them can't be tested because they are part of JVMCI but I've added a >> directed test for the JDK-8287061 code and a more brute force test that >> tries to decode everything. > > Tom Rodriguez has updated the pull request incrementally with two additional > commits since the last revision: > > - Update > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java > >Co-authored-by: Andrey Turbanov > - Update > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java > >Co-authored-by: Andrey Turbanov I can't really review the implementation details because this requires compiler expertise I don't have. I did review the parts that were understandable without compiler knowledge and made a few suggestions. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java line 1644: > 1642: } > 1643: }, > 1644: new Command("testdebuginfodecode", "testdebuginfodecode", > false) { This doesn't belong in clhsdb. You can do all of this directly in the test if you launch it properly. See for example `TestObjectAlignment.java`. test/hotspot/jtreg/serviceability/sa/ClhsdbTestAllocationMerge.java line 58: > 56: Map> expStrMap = new HashMap<>(); > 57: Map> unExpStrMap = new HashMap<>(); > 58: test.run(theApp.getPid(), cmds, expStrMap, unExpStrMap); You can just pass `null` for the two Map args, and no need to import java.util.HashMap or java.util.Map. test/hotspot/jtreg/serviceability/sa/ClhsdbTestDebugInfodDecode.java line 52: > 50: System.out.println("Started LingeredAppWithEnum with pid " + > theApp.getPid()); > 51: > 52: List cmds = List.of("testdebuginfodecode"); This will need to change given my suggestion to instead include the contents of clshdb command within this test. test/hotspot/jtreg/serviceability/sa/ClhsdbTestDebugInfodDecode.java line 58: > 56: Map> expStrMap = new HashMap<>(); > 57: Map> unExpStrMap = new HashMap<>(); > 58: test.run(theApp.getPid(), cmds, expStrMap, unExpStrMap); You can just pass null for the two Map args, and no need to import java.util.HashMap or java.util.Map. - Changes requested by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17407#pullrequestreview-1825478257 PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454151509 PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454163583 PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454171208 PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1454166498
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v4]
> The changes for JDK-8287061 didn't update the SA decoding logic and there are > other places where the decoding has gotten out of sync with HotSpot. Some of > them can't be tested because they are part of JVMCI but I've added a directed > test for the JDK-8287061 code and a more brute force test that tries to > decode everything. Tom Rodriguez has updated the pull request incrementally with two additional commits since the last revision: - Update src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java Co-authored-by: Andrey Turbanov - Update src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/17407/files - new: https://git.openjdk.org/jdk/pull/17407/files/49477222..1a4e625e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17407=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17407=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17407.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17407/head:pull/17407 PR: https://git.openjdk.org/jdk/pull/17407