Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 20:02:44 GMT, Tom Rodriguez wrote: >> It's not -v that is making it do that. It does it without -v also. It can be >> a very slow operation, and I recently was running into cases where even >> after 10 minutes it was not done, so I gave up waiting. I'm about to file a >> CR to make it so it does not do that by default, and add a -l argument to >> optionally enable it. This will make it consistent with what bin/jstack is >> doing. I already have the fix ready. Just need to get the CR filed and and >> publish the PR. > > Ok. I was assuming that the `-v` option was like the `-l` option. Anyway, > TestDebugInfoDecode doesn't use jstack so it doesn't need the problem list > entry. I think generally speaking we problem list all SA tests with generational ZGC, but probably those that don't deal with the heap at all (which means they don't even use jstack) are ok. - PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456488468
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 19:52:47 GMT, Chris Plummer wrote: >> No. ClhsdbTestAllocationMerge is using `jstack -v` which doesn't work with >> ZGC because the `-v` part needs to iterate the heap looking for >> AbstractQueuedSynchronizers which doesn't work with ZGC and the SA. > > It's not -v that is making it do that. It does it without -v also. It can be > a very slow operation, and I recently was running into cases where even after > 10 minutes it was not done, so I gave up waiting. I'm about to file a CR to > make it so it does not do that by default, and add a -l argument to > optionally enable it. This will make it consistent with what bin/jstack is > doing. I already have the fix ready. Just need to get the CR filed and and > publish the PR. Ok. I was assuming that the `-v` option was like the `-l` option. Anyway, TestDebugInfoDecode doesn't use jstack so it doesn't need the problem list entry. - PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456412077
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 19:44:38 GMT, Tom Rodriguez wrote: >> test/hotspot/jtreg/ProblemList-generational-zgc.txt line 108: >> >>> 106: serviceability/sa/sadebugd/ClhsdbAttachToDebugServer.java 8307393 >>> generic-all >>> 107: serviceability/sa/sadebugd/ClhsdbTestConnectArgument.java 8307393 >>> generic-all >>> 108: serviceability/sa/ClhsdbTestAllocationMerge.java 8307393 >>> generic-all >> >> Do you need to add TestDebugInfoDecode.java? > > No. ClhsdbTestAllocationMerge is using `jstack -v` which doesn't work with > ZGC because the `-v` part needs to iterate the heap looking for > AbstractQueuedSynchronizers which doesn't work with ZGC and the SA. It's not -v that is making it do that. It does it without -v also. It can be a very slow operation, and I recently was running into cases where even after 10 minutes it was not done, so I gave up waiting. I'm about to file a CR to make it so it does not do that by default, and add a -l argument to optionally enable it. This will make it consistent with what bin/jstack is doing. I already have the fix ready. Just need to get the CR filed and and publish the PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456400105
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 19:34:29 GMT, Chris Plummer wrote:
>> Tom Rodriguez has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove testdebuginfodecode command
>
> test/hotspot/jtreg/serviceability/sa/TestDebugInfoDecode.java line 109:
>
>> 107: if (args == null || args.length == 0) {
>> 108: try {
>> 109: theApp = new LingeredApp();
>
> Is there a reason why previously you had used LingeredAppWithEnum and now you
> are using LingeredApp?
Both were chosen as a result of copy/paste from existing tests and it honestly
doesn't matter. The Xcomp is what's producing nmethods so the actual test
doesn't matter.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456392560
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 17:35:39 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 one additional > commit since the last revision: > > Remove testdebuginfodecode command I pushed the removal of the empty maps as well. - PR Comment: https://git.openjdk.org/jdk/pull/17407#issuecomment-1896561935
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 19:40:03 GMT, Chris Plummer wrote: >> Tom Rodriguez has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove testdebuginfodecode command > > test/hotspot/jtreg/ProblemList-generational-zgc.txt line 108: > >> 106: serviceability/sa/sadebugd/ClhsdbAttachToDebugServer.java 8307393 >> generic-all >> 107: serviceability/sa/sadebugd/ClhsdbTestConnectArgument.java 8307393 >> generic-all >> 108: serviceability/sa/ClhsdbTestAllocationMerge.java 8307393 >> generic-all > > Do you need to add TestDebugInfoDecode.java? No. ClhsdbTestAllocationMerge is using `jstack -v` which doesn't work with ZGC because the `-v` part needs to iterate the heap looking for AbstractQueuedSynchronizers which doesn't work with ZGC and the SA. - PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456389516
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
On Wed, 17 Jan 2024 17:35:39 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 one additional
> commit since the last revision:
>
> Remove testdebuginfodecode command
test/hotspot/jtreg/ProblemList-generational-zgc.txt line 108:
> 106: serviceability/sa/sadebugd/ClhsdbAttachToDebugServer.java 8307393
> generic-all
> 107: serviceability/sa/sadebugd/ClhsdbTestConnectArgument.java 8307393
> generic-all
> 108: serviceability/sa/ClhsdbTestAllocationMerge.java 8307393
> generic-all
Do you need to add TestDebugInfoDecode.java?
test/hotspot/jtreg/serviceability/sa/TestDebugInfoDecode.java line 109:
> 107: if (args == null || args.length == 0) {
> 108: try {
> 109: theApp = new LingeredApp();
Is there a reason why previously you had used LingeredAppWithEnum and now you
are using LingeredApp?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456382492
PR Review Comment: https://git.openjdk.org/jdk/pull/17407#discussion_r1456375716
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v5]
> 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 one additional commit since the last revision: Remove testdebuginfodecode command - Changes: - all: https://git.openjdk.org/jdk/pull/17407/files - new: https://git.openjdk.org/jdk/pull/17407/files/1a4e625e..07eefccd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17407=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17407=03-04 Stats: 223 lines in 3 files changed: 119 ins; 103 del; 1 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
