Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v6]
On Wed, 17 Jan 2024 19:52:32 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: > > Review comments Let's merge this soon Tom. - PR Comment: https://git.openjdk.org/jdk/pull/17407#issuecomment-2082605948
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
On Sun, 28 Apr 2024 23:37:22 GMT, Vladimir Kozlov wrote: >> Move immutable nmethod's data from CodeCache to C heap. It includes >> `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, >> speculations`. It amounts for about 30% (optimized VM) of space in CodeCache. >> >> Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable >> nmethod's data. Call `vm_exit_out_of_memory()` if allocation failed. >> >> Shuffle fields order and change some fields size from 4 to 2 bytes to avoid >> nmethod's header size increase. >> >> Tested tier1-5, stress,xcomp >> >> Our performance testing does not show difference. >> >> Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address comments. Moved jvmci_data back to mutable data section. Marked as reviewed by dnsimon (Reviewer). JVMCI changes now look good to me. - PR Review: https://git.openjdk.org/jdk/pull/18984#pullrequestreview-2027884309 PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2082185011
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Sun, 28 Apr 2024 07:02:40 GMT, Dean Long wrote: >> Move immutable nmethod's data from CodeCache to C heap. It includes >> `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, >> speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space >> in CodeCache. >> >> Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable >> nmethod's data. Bail out compilation if allocation failed. >> >> Shuffle fields order and change some fields size from 4 to 2 bytes to avoid >> nmethod's header size increase. >> >> Tested tier1-5, stress,xcomp >> >> Our performance testing does not show difference. >> >> Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. > > It only makes sense if the immutable data heap is not also used for other > critical resources. If malloc or metaspace were used as the immutable data > heap, normally failures in those heaps are fatal, because other critical > resources (monitors, classes, etc) are allocated from there, so any failure > means the JVM is about to die. There's no reason to find a fall-back method > to allocate a new nmethod in that case. Just to be clear @dean-long , you're saying failure to allocate immutable data in the C heap should result in a fatal error? Makes sense to me as the VM must indeed be very close to crashing anyway in that case. It also, obviates the need for propagating `out_of_memory_error` to JVMCI code. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2081427477
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote: > Move immutable nmethod's data from CodeCache to C heap. It includes > `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, > speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space > in CodeCache. > > Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable > nmethod's data. Bail out compilation if allocation failed. > > Shuffle fields order and change some fields size from 4 to 2 bytes to avoid > nmethod's header size increase. > > Tested tier1-5, stress,xcomp > > Our performance testing does not show difference. > > Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. src/hotspot/share/jvmci/jvmciRuntime.cpp line 2178: > 2176: > nmethod_mirror_name, > 2177: > failed_speculations); > 2178: nmethod::ResultStatus result_status; Please propagate the new `out_of_memory` result throughout JVMCI (e.g. in `JVMCI::CodeInstallResult` enum and `HotSpotVMConfig.getCodeInstallResultDescription` method). - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581906300
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote: > Move immutable nmethod's data from CodeCache to C heap. It includes > `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, > speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space > in CodeCache. > > Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable > nmethod's data. Bail out compilation if allocation failed. > > Shuffle fields order and change some fields size from 4 to 2 bytes to avoid > nmethod's header size increase. > > Tested tier1-5, stress,xcomp > > Our performance testing does not show difference. > > Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. src/hotspot/share/code/nmethod.hpp line 476: > 474: passed, > 475: code_cache_full, > 476: out_of_memory Maybe `out_of_c_heap_memory` would be clearer? Or `out_of_immutable_data_memory` if immutable data may not always be malloc'ed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581904919
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 22:58:21 GMT, Dean Long wrote: > @dougxc should check JVMCI changes. Thanks for the heads up. I've asked @matias9927 to double check these changes against libgraal which should address any concerns about how this change impacts Graal. - PR Comment: https://git.openjdk.org/jdk/pull/18819#issuecomment-2063308834
Integrated: 8328135: javax/management/remote/mandatory/loading/MissingClassTest.java fails on libgraal
On Thu, 14 Mar 2024 10:46:10 GMT, Doug Simon wrote: > This PR increases a timeout in `MissingClassTest.java` to handle slight > slower compilation on a fastdebug build when using `-Xcomp`. > Testing on mach5 shows that the increase from 60 s to 90 s resolves the > timeouts. This pull request has now been integrated. Changeset: e6a8fdd8 Author: Doug Simon URL: https://git.openjdk.org/jdk/commit/e6a8fdd82c2b97f7ae74dfe8fbd3402718c9161c Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8328135: javax/management/remote/mandatory/loading/MissingClassTest.java fails on libgraal Reviewed-by: kevinw, never - PR: https://git.openjdk.org/jdk/pull/18297
Re: RFR: 8328135: javax/management/remote/mandatory/loading/MissingClassTest.java fails on libgraal
On Thu, 14 Mar 2024 10:46:10 GMT, Doug Simon wrote: > This PR increases a timeout in `MissingClassTest.java` to handle slight > slower compilation on a fastdebug build when using `-Xcomp`. > Testing on mach5 shows that the increase from 60 s to 90 s resolves the > timeouts. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18297#issuecomment-1997817640
RFR: 8328135: javax/management/remote/mandatory/loading/MissingClassTest.java fails on libgraal
This PR increases a timeout in `MissingClassTest.java` to handle slight slower compilation on a fastdebug build when using `-Xcomp`. Testing on mach5 shows that the increase from 60 s to 90 s resolves the timeouts. - Commit messages: - increase timeout in loop waiting for listeners to receive notifs Changes: https://git.openjdk.org/jdk/pull/18297/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18297=00 Issue: https://bugs.openjdk.org/browse/JDK-8328135 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18297.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18297/head:pull/18297 PR: https://git.openjdk.org/jdk/pull/18297
Integrated: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
On Fri, 1 Mar 2024 17:24:02 GMT, Doug Simon wrote: > To account for slightly slower compile times on libgraal + fastdebug + > `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` > from 2000 ms to 3000 ms. > With this change, the test now reliably passes. This pull request has now been integrated. Changeset: 8f0fb27d Author: Doug Simon URL: https://git.openjdk.org/jdk/commit/8f0fb27decec28f32e4d88341237189ba4a340fb Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal Reviewed-by: never, kevinw - PR: https://git.openjdk.org/jdk/pull/18085
Re: RFR: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
On Fri, 1 Mar 2024 17:24:02 GMT, Doug Simon wrote: > To account for slightly slower compile times on libgraal + fastdebug + > `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` > from 2000 ms to 3000 ms. > With this change, the test now reliably passes. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18085#issuecomment-1973749075
RFR: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
To account for slightly slower compile times on libgraal + fastdebug + `-Xcomp`, this PR increases a timeout in `NotifReconnectDeadlockTest.java` from 2000 ms to 3000 ms. With this change, the test now reliably passes. - Commit messages: - adjust timeout in NotifReconnectDeadlockTest.java Changes: https://git.openjdk.org/jdk/pull/18085/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18085=00 Issue: https://bugs.openjdk.org/browse/JDK-8327136 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18085.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18085/head:pull/18085 PR: https://git.openjdk.org/jdk/pull/18085
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 10:48:26 GMT, Kevin Walls wrote: > Or maybe they are not done the initial -Xcomp compile and > waitUntilThreadBlocked() is mistakenly thinking they are now idle... Yes, I believe this is the case. It's not really clear to me what the test is testing. As far as I can see, if the 2 timings are meant to be taken when the worker threads are idle, I would have thought the expected delta should be 0. - PR Comment: https://git.openjdk.org/jdk/pull/17675#issuecomment-1923578655
Integrated: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range
On Thu, 1 Feb 2024 18:25:33 GMT, Doug Simon wrote: > The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can transiently > fail when run with `-Xcomp`. This happens when the compilation of methods on > the worker threads interleaves with the 2 calls on the main thread to > `mbean.getThreadCpuTime` to get the CPU time for all worker threads. > > The way the test is currently written, the worker threads are all usually > waiting on a shared monitor when the 2 timings are taken. That is, in a > successful run, the delta between the timings is always 0. When `-Xcomp` > compilations kick in at the "wrong" time or take sufficiently long, the > expected delta of 100 nanoseconds is easily exceeded. > > It does not make sense to run a timing test with such a small expected delta > with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. This pull request has now been integrated. Changeset: 91d8dac9 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/91d8dac9cff5689abcf2fc8950b15d284f933afd Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range Reviewed-by: dholmes, sspitsyn - PR: https://git.openjdk.org/jdk/pull/17675
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 07:38:24 GMT, Doug Simon wrote: >> The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can >> transiently fail when run with `-Xcomp`. This happens when the compilation >> of methods on the worker threads interleaves with the 2 calls on the main >> thread to `mbean.getThreadCpuTime` to get the CPU time for all worker >> threads. >> >> The way the test is currently written, the worker threads are all usually >> waiting on a shared monitor when the 2 timings are taken. That is, in a >> successful run, the delta between the timings is always 0. When `-Xcomp` >> compilations kick in at the "wrong" time or take sufficiently long, the >> expected delta of 100 nanoseconds is easily exceeded. >> >> It does not make sense to run a timing test with such a small expected delta >> with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix date in header Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/17675#issuecomment-1923543404
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
> The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can transiently > fail when run with `-Xcomp`. This happens when the compilation of methods on > the worker threads interleaves with the 2 calls on the main thread to > `mbean.getThreadCpuTime` to get the CPU time for all worker threads. > > The way the test is currently written, the worker threads are all usually > waiting on a shared monitor when the 2 timings are taken. That is, in a > successful run, the delta between the timings is always 0. When `-Xcomp` > compilations kick in at the "wrong" time or take sufficiently long, the > expected delta of 100 nanoseconds is easily exceeded. > > It does not make sense to run a timing test with such a small expected delta > with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix date in header - Changes: - all: https://git.openjdk.org/jdk/pull/17675/files - new: https://git.openjdk.org/jdk/pull/17675/files/4ff2f7cd..a3bc2653 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17675=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17675=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17675/head:pull/17675 PR: https://git.openjdk.org/jdk/pull/17675
RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range
The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can transiently fail when run with `-Xcomp`. This happens when the compilation of methods on the worker threads interleaves with the 2 calls on the main thread to `mbean.getThreadCpuTime` to get the CPU time for all worker threads. The way the test is currently written, the worker threads are all usually waiting on a shared monitor when the 2 timings are taken. That is, in a successful run, the delta between the timings is always 0. When `-Xcomp` compilations kick in at the "wrong" time or take sufficiently long, the expected delta of 100 nanoseconds is easily exceeded. It does not make sense to run a timing test with such a small expected delta with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. - Commit messages: - disable ThreadCpuTimeArray with xcomp Changes: https://git.openjdk.org/jdk/pull/17675/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17675=00 Issue: https://bugs.openjdk.org/browse/JDK-8325137 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17675.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17675/head:pull/17675 PR: https://git.openjdk.org/jdk/pull/17675
Integrated: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Sun, 25 Jun 2023 06:58:14 GMT, Doug Simon wrote: > The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. This pull request has now been integrated. Changeset: f6bdccb4 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/f6bdccb45caca0f69918a773a9ad9b2ad91b702f Stats: 176 lines in 6 files changed: 123 ins; 21 del; 32 mod 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit Reviewed-by: never, kvn - PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
On Fri, 30 Jun 2023 15:15:16 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix warning about conversion from size_t to int - take 2 Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1615145850
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Fri, 30 Jun 2023 17:30:33 GMT, Vladimir Kozlov wrote: > > > But, please, activate GHA testing for this branch. > > > > > > Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's > > the point of doing redundant testing? > > It builds and tests configurations (32-bit) we don't have in our testing. Good to know - thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1615144598
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v8]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix warning about conversion from size_t to int - take 2 - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/5bb3b529..11a2dad5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=07 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
On Thu, 29 Jun 2023 20:06:19 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains six additional commits since > the last revision: > > - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into > JDK-8310829 > - [skip ci] handle pending HotSpot exception closer to site causing exception > - revert to lazy loading of VMSupport > - each exception translation failure should trigger a JVMCI event > - try harder to show nested exception during exception translation > - resolve VMSupport at bootstrap to avoid nested exception in > ExceptionTranslation::doit I have fixed the warning on Windows: 5bb3b529d36c906ac861e5ebf1b861dbb35bfe2c > But, please, activate GHA testing for this branch. Isn't GHA a strict subset of or equal to tier1 mach5 testing? If so, what's the point of doing redundant testing? - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1614746764
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v7]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fix warning about conversion from size_t to int - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/e46a6a17..5bb3b529 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=06 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
On Thu, 29 Jun 2023 02:13:32 GMT, David Holmes wrote: > Someone from the compiler team should review this now. @vnkozlov could you please review this or nominate someone else from the compiler team to look at it. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1613736704
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v6]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - [skip ci] Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8310829 - [skip ci] handle pending HotSpot exception closer to site causing exception - revert to lazy loading of VMSupport - each exception translation failure should trigger a JVMCI event - try harder to show nested exception during exception translation - resolve VMSupport at bootstrap to avoid nested exception in ExceptionTranslation::doit - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/9236128a..e46a6a17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=04-05 Stats: 13222 lines in 537 files changed: 6305 ins; 3442 del; 3475 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Wed, 28 Jun 2023 18:28:37 GMT, Tom Rodriguez wrote: > Why can't the pending exception handling be in the respective virtual? Done: 9236128ad1b7297c8c4e9d3022b88c3ab3278022 - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1612113760
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v5]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: [skip ci] handle pending HotSpot exception closer to site causing exception - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/734903a8..9236128a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=04 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=03-04 Stats: 145 lines in 3 files changed: 102 ins; 25 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 23:06:49 GMT, Tom Rodriguez wrote: > I don't think pushing it down that far improves things. encode always > precedes decode so why not resolve it before the encode call. Because the `VMSupport` klass is only needed if calling into HotSpot so it's better to push it down into `HotSpotToSharedLibraryExceptionTranslation::encode`. Also, if the `VMSupport` klass is used for encoding, it's *not* needed for decoding (the libgraal `VMSupport` jclass value is used instead). - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610903263
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
On Tue, 27 Jun 2023 23:08:22 GMT, Tom Rodriguez wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert to lazy loading of VMSupport
>
> src/hotspot/share/jvmci/jvmciEnv.cpp line 402:
>
>> 400: }
>> 401: int res = encode(THREAD, buffer, buffer_size);
>> 402: if (_from_env != nullptr && !_from_env->is_hotspot() &&
>> _from_env->has_pending_exception()) {
>
> Why is this check before the `HAS_PENDING_EXCEPTION` check? Couldn't you end
> up returning with a pending exception in this path?
If `encode` is `SharedLibraryToHotSpotExceptionTranslation::encode` there is no
possibility of a pending HotSpot exception upon it returning. If it's
`HotSpotToSharedLibraryExceptionTranslation::encode` then
`_from_env->is_hotspot()` is `true` and so this `if` branch is not taken.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1244794019
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > each exception translation failure should trigger a JVMCI event I've pushed the loading of `VMSupport` down into the 2 cases where it's actually needed and made it lazy again which should address all concerns about extra noise at VM startup. Thanks for the push back on this - I agree that it's the best solution. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1610243648
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v4]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: revert to lazy loading of VMSupport - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/819a24fd..734903a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=02-03 Stats: 35 lines in 5 files changed: 17 ins; 2 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:56:21 GMT, David Holmes wrote: > I would think it is likely to fail with OOME under the same GC stress test > conditions. > > But if this is just a stress test issue then bailing out when the loading > fails would be far simpler than pre-loading the class. You've added to the VM > startup cost just to avoid a stress test failure. Any app running near the GC limit can cause this and we've seen this with libgraal in practice. When the VM is near the GC limit, any compilation can cause an OOME and then when that OOME wants to be translated back to libgraal, it can be the first time VMSupport is used. The time for loading `VMSupport` eagerly is in the noise. For example, `-verbose:class` shows that about 30 classes (including `VMSupport`) are loaded in 1ms: [0.011s][info][class,load] java.lang.reflect.Executable source: shared objects file [0.012s][info][class,load] java.lang.reflect.Method source: shared objects file [0.012s][info][class,load] java.lang.reflect.Constructor source: shared objects file [0.012s][info][class,load] jdk.internal.vm.ContinuationScope source: shared objects file [0.012s][info][class,load] jdk.internal.vm.Continuation source: shared objects file [0.012s][info][class,load] jdk.internal.vm.StackChunk source: shared objects file [0.012s][info][class,load] jdk.internal.vm.VMSupport source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.MagicAccessorImpl source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.MethodAccessor source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.MethodAccessorImpl source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.DelegatingClassLoader source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.ConstantPool source: shared objects file [0.012s][info][class,load] java.lang.annotation.Annotation source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.CallerSensitive source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.ConstructorAccessor source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.ConstructorAccessorImpl source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.DirectConstructorHandleAccessor$NativeAccessor source: shared objects file [0.012s][info][class,load] jdk.internal.reflect.SerializationConstructorAccessorImpl source: shared objects file [0.012s][info][class,load] java.lang.invoke.MethodHandle source: shared objects file [0.012s][info][class,load] java.lang.invoke.DirectMethodHandle source: shared objects file [0.012s][info][class,load] java.lang.invoke.VarHandle source: shared objects file [0.012s][info][class,load] java.lang.invoke.MemberName source: shared objects file [0.012s][info][class,load] java.lang.invoke.ResolvedMethodName source: shared objects file [0.012s][info][class,load] java.lang.invoke.MethodHandleNatives source: shared objects file [0.012s][info][class,load] java.lang.invoke.LambdaForm source: shared objects file [0.012s][info][class,load] java.lang.invoke.TypeDescriptor$OfMethod source: shared objects file [0.012s][info][class,load] java.lang.invoke.MethodType source: shared objects file [0.012s][info][class,load] java.lang.BootstrapMethodError source: shared objects file [0.012s][info][class,load] java.lang.invoke.CallSite source: shared objects file [0.012s][info][class,load] jdk.internal.foreign.abi.NativeEntryPoint source: shared objects file [0.012s][info][class,load] jdk.internal.foreign.abi.ABIDescriptor source: shared objects file [0.012s][info][class,load] jdk.internal.foreign.abi.VMStorage source: shared objects file [0.013s][info][class,load] jdk.internal.foreign.abi.UpcallLinker$CallRegs source: shared objects file - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608388679
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 21:41:25 GMT, David Holmes wrote: > Then why did you not simply handle the exception from the loading of > VMSupport the same way? The only actual case seen of the original way failing is due to OOME in GC stress tests. If loading of VMSupport is done during VM startup, then no stress test code can cause an OOME while loading VMSupport. I doubt `VMSupport.` would ever fail in practice. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1608330027
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
On Mon, 26 Jun 2023 13:17:25 GMT, Doug Simon wrote: >> The VMSupport class is required for translating an exception between the >> HotSpot and libgraal heaps. >> Loading it lazily can result in a loading exception, obscuring the exception >> being translated. >> To avoid this, VMSupport is loaded eagerly along with the other vmClasses. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > each exception translation failure should trigger a JVMCI event BTW, I've manually tested this with libgraal. It's not possible to add a jtreg test until libgraal in part of OpenJDK. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1607466267
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v3]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: each exception translation failure should trigger a JVMCI event - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/614e1e9f..819a24fd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=01-02 Stats: 4 lines in 2 files changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v2]
On Mon, 26 Jun 2023 07:59:17 GMT, David Holmes wrote: > if the first decode encounters a class initialization error then it will just > return with the exception pending and no decoding will actually have occurred That's fine - if VMSupport fails class initialization, then no "rich" decoding can occur (by definition) and so throwing the clinit error is the best we can do. > If we get to the encode first and it encounters an initialization error it > will still attempt to do a decode that must in turn fail You have to keep in mind that `encode` and `decode` are called in different runtimes. If encoding an exception in the HotSpot runtime fails (e.g. due to an OOME calling `VMSupport.` in the HotSpot heap), then it's still fine to try call `VMSupport.decode` in the libgraal runtime. If `VMSupport.decode` in the libgraal runtime also fails, then it throw the exception describing the secondary failure. There's simply no way to guarantee all info is shown when secondary issues occur during exception handling. - PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1607433634
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit [v2]
> The VMSupport class is required for translating an exception between the > HotSpot and libgraal heaps. > Loading it lazily can result in a loading exception, obscuring the exception > being translated. > To avoid this, VMSupport is loaded eagerly along with the other vmClasses. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: try harder to show nested exception during exception translation - Changes: - all: https://git.openjdk.org/jdk/pull/14641/files - new: https://git.openjdk.org/jdk/pull/14641/files/fafc0aae..614e1e9f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14641=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14641=00-01 Stats: 51 lines in 4 files changed: 21 ins; 4 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
On Mon, 26 Jun 2023 07:42:48 GMT, David Holmes wrote:
> The eager loading seems reasonable, but I do not understand the details here.
> In what way was loading failing? You still have to initialize `VMSupport`
> before you can call methods on it, so that could also fail - though the code
> does not seem to notice/handle this. ??
The usages of `vmSupport` below all use `JavaCalls:call_static` which takes
care of linking and initializing the class.
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 585:
>
>> 583:
>> 584: if (class_name->utf8_length() <= 1) {
>> 585: JVMCI_THROW_MSG_0(InternalError, err_msg("Primitive type %s should
>> be handled in Java code", str));
>
> Seems unrelated to the fix at hand.
Yes, it's a minor fix up I noticed while making changes a few lines below. It
just avoids a conversion of a `Symbol` back to a C string when the original C
string is available.
-
PR Comment: https://git.openjdk.org/jdk/pull/14641#issuecomment-1606899412
PR Review Comment: https://git.openjdk.org/jdk/pull/14641#discussion_r1241766196
RFR: 8310829: guarantee(!HAS_PENDING_EXCEPTION) failed in ExceptionTranslation::doit
The VMSupport class is required for translating an exception between the HotSpot and libgraal heaps. Loading it lazily can result in a loading exception, obscuring the exception being translated. To avoid this, VMSupport is loaded eagerly along with the other vmClasses. - Commit messages: - resolve VMSupport at bootstrap to avoid nested exception in ExceptionTranslation::doit Changes: https://git.openjdk.org/jdk/pull/14641/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14641=00 Issue: https://bugs.openjdk.org/browse/JDK-8310829 Stats: 19 lines in 5 files changed: 1 ins; 11 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/14641.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14641/head:pull/14641 PR: https://git.openjdk.org/jdk/pull/14641
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Fri, 9 Jun 2023 12:14:46 GMT, Doug Simon wrote: > > Is JVMCI used by the Graal compiler only? > > So far this is true and will probably remain true for the foreseeable future. > However, the Right Thing to do long term is to add a > `jdk.test.whitebox.code.Compiler.uncommonTrapsHavePreciseBCIs()` method. I created https://bugs.openjdk.org/browse/JDK-8310346 for this enhancement. - PR Comment: https://git.openjdk.org/jdk/pull/14381#issuecomment-1598205886
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Fri, 9 Jun 2023 05:26:59 GMT, Serguei Spitsyn wrote: > Is JVMCI used by the Graal compiler only? So far this is true and will probably remain true for the foreseeable future. However, the Right Thing to do long term is to add a `jdk.test.whitebox.code.Compiler.uncommonTrapsHavePreciseBCIs()` method. - PR Comment: https://git.openjdk.org/jdk/pull/14381#issuecomment-1584480496
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 18:29:50 GMT, Yudi Zheng wrote:
>> test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java
>> line 257:
>>
>>> 255:
>>> 256: checkLines = !(enableJVMCI.getValue().equals("true")
>>> 257: && useJVMCICompiler.getValue().equals("true"));
>>
>> Is it possible to use `jdk.test.whitebox.code.Compiler.isGraalEnabled()`
>> here instead?
>
> To use whitebox I will have to update the config of every test here, which I
> think is too verbose:
>
> diff --git
> a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
>
> b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> index e08454a4857..f086f744965 100644
> ---
> a/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> +++
> b/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatIntervalTest.java
> @@ -27,11 +27,15 @@ package MyPackage;
> /**
> * @test
> * @summary Verifies the JVMTI Heap Monitor sampling interval average.
> - * @build Frame HeapMonitor
> + * @library /test/lib
> + * @build Frame HeapMonitor jdk.test.whitebox.WhiteBox
> * @compile HeapMonitorStatIntervalTest.java
> * @requires vm.jvmti
> * @requires vm.compMode != "Xcomp"
> - * @run main/othervm/native -agentlib:HeapMonitorTest
> MyPackage.HeapMonitorStatIntervalTest
> + * @run driver jdk.test.lib.helpers.ClassFileInstaller
> jdk.test.whitebox.WhiteBox
> + * @run main/othervm/native -agentlib:HeapMonitorTest -Xbootclasspath/a:.
> + * -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
> + * MyPackage.HeapMonitorStatIntervalTest
> */
Ok.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14381#discussion_r1223447124
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 17:14:39 GMT, Yudi Zheng wrote:
> HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
> not enforce checking line number derived from uncommon trap debug info.
> However, Graal does not set this property explicitly.
Marked as reviewed by dnsimon (Committer).
-
PR Review: https://git.openjdk.org/jdk/pull/14381#pullrequestreview-1470601144
Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v8]
On Tue, 2 May 2023 02:01:44 GMT, Serguei Spitsyn wrote: >> This refactoring to separate ThreadStart/ThreadEnd events posting code in >> the JVMTI VTMS transitions is needed for future work on JVMTI scalability >> and performance improvements. It is to easier put this code on slow path. >> >> Testing: mach5 tiers 1-6 were successful. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > update copyright comments src/hotspot/share/runtime/sharedRuntime.cpp line 641: > 639: JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, > jboolean hide, JavaThread* current)) > 640: assert(hide == JNI_FALSE, "must be VTMS transition finish"); > 641: jobject vthread = JNIHandles::make_local(const_cast(vt)); Since the current thread is in the `current` arg, it could be used here when creating the local handle. - PR Review Comment: https://git.openjdk.org/jdk/pull/13484#discussion_r1223444559
Re: RFR: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
On Thu, 8 Jun 2023 17:14:39 GMT, Yudi Zheng wrote:
> HeapMonitor checks if System.getProperty("jvmci.Compiler") is graal and will
> not enforce checking line number derived from uncommon trap debug info.
> However, Graal does not set this property explicitly.
test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitor.java
line 257:
> 255:
> 256: checkLines = !(enableJVMCI.getValue().equals("true")
> 257: && useJVMCICompiler.getValue().equals("true"));
Is it possible to use `jdk.test.whitebox.code.Compiler.isGraalEnabled()` here
instead?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14381#discussion_r1223342262
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote:
>> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes
>> I'd appreciate if this was considered trivial.
>
> Johan Sjölen has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Align
> - Suggestions
It may be simpler to use simple grepping + an allow list. For example using
[`ag`](https://github.com/ggreer/the_silver_searcher) and `grep` seems to catch
the few remaining offenders:
> ag NULL src/hotspot/ --cpp | grep -v _NULL | grep -v NULL_ | grep -v -E
> '[A-Z]NULL' | grep -v -E '//.*NULL' | grep -v '"NULL"'
src/hotspot/cpu/ppc/macroAssembler_ppc.hpp:735: void
load_klass_check_null(Register dst, Register src, Label* is_null = NULL);
src/hotspot/cpu/ppc/stubGenerator_ppc.cpp:4700:if (UnsafeCopyMemory::_table
== NULL) {
src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp:191:if (nop == NULL) {
src/hotspot/cpu/riscv/codeBuffer_riscv.cpp:74: if
(cb->stubs()->maybe_expand_to_ensure_remaining(total_requested_size) &&
cb->blob() == NULL) {
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp:4019:if
(UnsafeCopyMemory::_table == NULL) {
src/hotspot/cpu/riscv/stubGenerator_riscv.cpp:4077:if (bs_nm != NULL) {
src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp:125: NativeCall* call =
NULL;
src/hotspot/cpu/aarch64/jvmciCodeInstaller_aarch64.cpp:158:if (nop == NULL)
{
src/hotspot/share/jfr/dcmd/jfrDcmds.hpp:162:JavaPermission p =
{"java.lang.management.ManagementPermission", "monitor", NULL};
src/hotspot/share/jfr/dcmd/jfrDcmds.hpp:187:JavaPermission p =
{"java.lang.management.ManagementPermission", "monitor", NULL};
src/hotspot/share/include/jvm.h:423: * Find a class from a boot class loader.
Returns NULL if class not found.
src/hotspot/share/prims/jvmtiAgent.cpp:375: const jint err =
(*on_load_entry)(_vm, const_cast(agent->options()), NULL);
src/hotspot/share/prims/whitebox.cpp:1885: if (cp->cache() == NULL) {
src/hotspot/share/prims/whitebox.cpp:1894: if (cp->cache() == NULL) {
src/hotspot/share/classfile/stringTable.hpp:150: static oop
init_shared_table(const DumpedInternedStrings* dumped_interned_strings)
NOT_CDS_JAVA_HEAP_RETURN_(NULL);
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:95: #undef NULL
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:96: #define NULL 0L
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:98: #ifndef NULL
src/hotspot/share/utilities/globalDefinitions_xlc.hpp:99:#define NULL 0
src/hotspot/share/cds/filemap.cpp:363: assert(ent != NULL, "sanity");
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:65:#undef NULL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:69:#define NULL 0LL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:71:#ifndef NULL
src/hotspot/share/utilities/globalDefinitions_visCPP.hpp:72:#define NULL 0
src/hotspot/share/utilities/globalDefinitions.cpp:162:
static_assert(sizeof(NULL) == sizeof(char*), "NULL must be same size as
pointer");
src/hotspot/share/adlc/output_c.cpp:279: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:305: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:368: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:393: for (pipeline->_reslist.reset();
(resource = pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/adlc/output_c.cpp:1009: for (_pipeline->_reslist.reset();
(resource = _pipeline->_reslist.iter()) != NULL;) {
src/hotspot/share/gc/x/xBarrierSet.inline.hpp:187:return
Raw::oop_arraycopy_in_heap(nullptr, 0, src, NULL, 0, dst, length);
src/hotspot/share/gc/x/xPageTable.inline.hpp:43:if (entry != NULL && entry
!= _prev) {
src/hotspot/share/gc/x/xBarrier.cpp:242: return NULL;
src/hotspot/share/oops/cpCache.cpp:888: LogStream* log_stream = NULL;
src/hotspot/share/oops/cpCache.cpp:906:
assert(resolved_references->obj_at(appendix_index) == NULL, "init just once");
src/hotspot/share/oops/cpCache.cpp:914: if (log_stream != NULL) {
src/hotspot/share/opto/runtime.cpp:491: fields[TypeFunc::Parms+0] = NULL; //
void
src/hotspot/share/jvmci/jvmciEnv.cpp:366:if (ex != NULL) {
-
PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571756690
Re: RFR: 8306851: Move Method access flags [v3]
On Thu, 27 Apr 2023 14:21:23 GMT, Coleen Phillimore wrote: >> This change moves the flags from AccessFlags to either ConstMethodFlags or >> MethodFlags, depending on whether they are set at class file parse time, >> which makes them essentially const, or at runtime, which makes them needing >> atomic access. >> >> This leaves AccessFlags int size because Klass still has JVM flags that are >> more work to move, but this change doesn't increase Method size. I didn't >> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are >> several of these in other places, and with this change the code is benign. >> >> Tested with tier1-6, and some manual verification of printing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Remove bool argument from ConstMethodFlags.set function. Marked as reviewed by dnsimon (Committer). Thankfully all these changes only impact values read by JVMCI Java code and none in [Graal Java code](https://github.com/oracle/graal/blob/114067fc41d97e6c07f6de9bd745196d6f967ae4/compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java#L47). Looks good to me. - PR Review: https://git.openjdk.org/jdk/pull/13654#pullrequestreview-1405368377 PR Comment: https://git.openjdk.org/jdk/pull/13654#issuecomment-1527109990
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v16]
On Tue, 28 Mar 2023 19:50:36 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC port was provided by @reinrich, RISCV was provided by @DingliZhang >> and @zifeihan, and S390x by @offamitkumar. >> >> This change supports the following platforms: x86, aarch64, PPC, RISCV, and >> S390x > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > s390x NULL to nullptr It has also broken GraalVM Native Image. I'll open a JBS issue with a reproducer soon but here's hs-err from a slowdebug JDK build showing the problem: [hs_err_pid30379.log](https://github.com/openjdk/jdk/files/11122818/hs_err_pid30379.log) - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1492011186
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v5]
On Wed, 15 Mar 2023 15:41:17 GMT, Frederic Parain wrote: >> Please review this change re-implementing the FieldInfo data structure. >> >> The FieldInfo array is an old data structure storing fields metadata. It has >> poor extension capabilities, a complex management code because of lack of >> strong typing and semantic overloading, and a poor memory efficiency. >> >> The new implementation uses a compressed stream to store those metadata, >> achieving better memory density and providing flexible extensibility, while >> exposing a strongly typed set of data when uncompressed. The stream is >> compressed using the unsigned5 encoding, which alreay present in the JDK >> (because of pack200) and the JVM (because JIT compulers use it to comrpess >> debugging information). >> >> More technical details are available in the CR: >> https://bugs.openjdk.org/browse/JDK-8292818 >> >> Those changes include a re-organisation of fields' flags, splitting the >> previous heterogeneous AccessFlags field into three distincts flag >> categories: immutable flags from the class file, immutable fields defined by >> the JVM, and finally mutable flags defined by the JVM. >> >> The SA, CI, and JVMCI, which all used to access the old FieldInfo array, >> have been updated too to deal with the new FieldInfo format. >> >> Tested with mach5, tier 1 to 7. >> >> Thank you. > > Frederic Parain has updated the pull request incrementally with one > additional commit since the last revision: > > SA and JVMCI fixes Marked as reviewed by dnsimon (Committer). - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 13:37:23 GMT, Frederic Parain wrote: >> src/hotspot/share/jvmci/jvmciEnv.hpp line 149: >> >>> 147: }; >>> 148: >>> 149: extern JNIEXPORT jobjectArray c2v_getDeclaredFieldsInfo(JNIEnv* env, >>> jobject, jobject, jlong); >> >> What's the purpose of this declaration? I don't think you need it or the >> `friend` declaration below since `new_FieldInfo(FieldInfo* fieldinfo, >> JVMCI_TRAPS)` is public. > > Without this declaration, builds fail on Windows with this error: > `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` Strange - thats not needed for other `JVMCIEnv` methods called from `jvmciCompilerToVM.cpp`. There must be some way to avoid this. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 13:12:31 GMT, Frederic Parain wrote: >> src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaField.java >> line 48: >> >>> 46: * Returns VM internal flags associated with this field >>> 47: */ >>> 48: int getInternalModifiers(); >> >> We've never exposed the internal modifiers before in public JVMCI API and we >> should refrain from doing so until there's a good reason to do so. Please >> remove this method. > > Access to internal modifiers is needed in > `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved > the declaration of the method to `HotSpotResolvedJavaField`. Does this change > work for you? Just use reflection to read the internal flags (like this test already does for the `index` field). I've attached [review.patch](https://github.com/openjdk/jdk/files/10970245/review.patch) with this change and a few other changes I think should be made for better naming (plus one test cleanup). - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Thu, 9 Mar 2023 21:18:19 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Interpreter optimization and comments As communicated to Matias earlier via email, the JVMCI changes in this PR look fine. - Marked as reviewed by dnsimon (Committer). PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain wrote:
>> Please review this change re-implementing the FieldInfo data structure.
>>
>> The FieldInfo array is an old data structure storing fields metadata. It has
>> poor extension capabilities, a complex management code because of lack of
>> strong typing and semantic overloading, and a poor memory efficiency.
>>
>> The new implementation uses a compressed stream to store those metadata,
>> achieving better memory density and providing flexible extensibility, while
>> exposing a strongly typed set of data when uncompressed. The stream is
>> compressed using the unsigned5 encoding, which alreay present in the JDK
>> (because of pack200) and the JVM (because JIT compulers use it to comrpess
>> debugging information).
>>
>> More technical details are available in the CR:
>> https://bugs.openjdk.org/browse/JDK-8292818
>>
>> Those changes include a re-organisation of fields' flags, splitting the
>> previous heterogeneous AccessFlags field into three distincts flag
>> categories: immutable flags from the class file, immutable fields defined by
>> the JVM, and finally mutable flags defined by the JVM.
>>
>> The SA, CI, and JVMCI, which all used to access the old FieldInfo array,
>> have been updated too to deal with the new FieldInfo format.
>>
>> Tested with mach5, tier 1 to 7.
>>
>> Thank you.
>
> Frederic Parain has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fixes includes and style
src/hotspot/share/jvmci/jvmciEnv.cpp line 1439:
> 1437: JNIAccessMark jni(this, THREAD);
> 1438: jobject result = jni()->NewObject(JNIJVMCI::FieldInfo::clazz(),
> 1439: JNIJVMCI::VMFlag::constructor(),
`JNIJVMCI::VMFlag::constructor()` is the wrong constructor.
src/hotspot/share/jvmci/jvmciEnv.hpp line 149:
> 147: };
> 148:
> 149: extern JNIEXPORT jobjectArray c2v_getDeclaredFieldsInfo(JNIEnv* env,
> jobject, jobject, jlong);
What's the purpose of this declaration? I don't think you need it or the
`friend` declaration below since `new_FieldInfo(FieldInfo* fieldinfo,
JVMCI_TRAPS)` is public.
src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 416:
> 414: declare_constant(FieldInfo::FieldFlags::_ff_injected)
> \
> 415: declare_constant(FieldInfo::FieldFlags::_ff_stable)
> \
> 416: declare_constant(FieldInfo::FieldFlags::_ff_generic)
> \
I don't think `_ff_generic` is used in the JVMCI Java code so this entry can be
deleted. Please double check the other entries.
src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java
line 814:
> 812: HotSpotResolvedObjectTypeImpl resolvedHolder;
> 813: try {
> 814: resolvedHolder = compilerToVM().resolveFieldInPool(this,
> index, (HotSpotResolvedJavaMethodImpl) method, (byte) opcode, info);
Please update the javadoc for `CompilerToVM.resolveFieldInPool` to reflect the
expanded definition of `info`.
src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedObjectTypeImpl.java
line 88:
> 86:
> 87: /**
> 88: * Lazily initialized cache for FieldInfo
nit: missing `.` at end of sentence
src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaField.java line
48:
> 46: * Returns VM internal flags associated with this field
> 47: */
> 48: int getInternalModifiers();
We've never exposed the internal modifiers before in public JVMCI API and we
should refrain from doing so until there's a good reason to do so. Please
remove this method.
test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaField.java
line 97:
> 95:
> 96: @Test
> 97: public void getInternalModifiersTest() {
No need for this test since the `getInternalModifiers` method should be removed.
-
PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
On Thu, 15 Dec 2022 21:20:31 GMT, David Holmes wrote: >> This is a simple cleanup RFE to get rid of old-style C casts in relation to >> JavaThread. >> >> In many cases involving NULL/nullptr the cast could just be dropped. >> Sometimes a static cast is needed to disambiguate overloads. >> >> A couple of reinterpret_cast are needed when doing int/ptr conversion. >> >> static_cast is used for void* conversion. >> >> The other changes should be self explanatory. >> >> The changes in >> >> src/hotspot/os_cpu/bsd_aarch64/javaThread_bsd_aarch64.cpp >> src/hotspot/os_cpu/windows_aarch64/javaThread_windows_aarch64.cpp >> >> are a bit more extensive. That code was using an alias for `this` which is >> completely unnecessary (and the alias creation was using the cast). This >> could be factored out if thought necessary. >> >> I grep'd as best I could for the old C-style casts but can't be certain I >> got them all. >> >> Testing: >> - all builds in our tiers1-5 >> - local linux x64 product, slowdebug and fastdebug >> - GHA >> - Sanity testing tiers 1-3 >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed commented examle. Ok, thanks. That's a shame. It seems like being able to run style checkers and other linters would be useful in cases like this. - PR: https://git.openjdk.org/jdk/pull/11682
Re: RFR: 8298241: Replace C-style casts with JavaThread::cast [v2]
On Thu, 15 Dec 2022 21:20:31 GMT, David Holmes wrote: >> This is a simple cleanup RFE to get rid of old-style C casts in relation to >> JavaThread. >> >> In many cases involving NULL/nullptr the cast could just be dropped. >> Sometimes a static cast is needed to disambiguate overloads. >> >> A couple of reinterpret_cast are needed when doing int/ptr conversion. >> >> static_cast is used for void* conversion. >> >> The other changes should be self explanatory. >> >> The changes in >> >> src/hotspot/os_cpu/bsd_aarch64/javaThread_bsd_aarch64.cpp >> src/hotspot/os_cpu/windows_aarch64/javaThread_windows_aarch64.cpp >> >> are a bit more extensive. That code was using an alias for `this` which is >> completely unnecessary (and the alias creation was using the cast). This >> could be factored out if thought necessary. >> >> I grep'd as best I could for the old C-style casts but can't be certain I >> got them all. >> >> Testing: >> - all builds in our tiers1-5 >> - local linux x64 product, slowdebug and fastdebug >> - GHA >> - Sanity testing tiers 1-3 >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed commented examle. Is it possible to add a test based on grep to prevent re-introduction of the unwanted C-style casts? - PR: https://git.openjdk.org/jdk/pull/11682
