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
