Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]
On Wed, 6 Dec 2023 02:58:19 GMT, David Holmes wrote: > The key point of this change is to split RUNNABLE into two states to > indicated "runnable after yielding" and "runnable after being unparked". That's right. This comes up with upcoming changes for monitors too - in that case it is important that a virtual thread that continues after blocking on monitorenter doesn't consume its parking permit. - PR Comment: https://git.openjdk.org/jdk/pull/16953#issuecomment-1842310534
Re: RFR: 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC [v2]
On Wed, 6 Dec 2023 05:50:02 GMT, Denghui Dong wrote: >> Hi, >> >> Could I have a review of this patch? >> >> In the current implementation, HeapDumpBeforeFullGC/AfterFullGC will >> generate dumps for every FGC, increasing the risk of disk full. >> >> This patch introduces a new option 'FullGCHeapDumpLimit' to limit the number >> of dumps triggered by HeapDumpBeforeFullGC/AfterFullGC to enhance >> production-friendliness. >> >> Best, >> Denghui > > Denghui Dong has updated the pull request incrementally with one additional > commit since the last revision: > > change the location of test Seems fine. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16976#pullrequestreview-1766708495
Re: RFR: 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC [v2]
> Hi, > > Could I have a review of this patch? > > In the current implementation, HeapDumpBeforeFullGC/AfterFullGC will generate > dumps for every FGC, increasing the risk of disk full. > > This patch introduces a new option 'FullGCHeapDumpLimit' to limit the number > of dumps triggered by HeapDumpBeforeFullGC/AfterFullGC to enhance > production-friendliness. > > Best, > Denghui Denghui Dong has updated the pull request incrementally with one additional commit since the last revision: change the location of test - Changes: - all: https://git.openjdk.org/jdk/pull/16976/files - new: https://git.openjdk.org/jdk/pull/16976/files/d62a507f..442b7f47 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16976=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16976=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16976.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16976/head:pull/16976 PR: https://git.openjdk.org/jdk/pull/16976
Integrated: 8320935: Move CDS config initialization code to cdsConfig.cpp
On Tue, 28 Nov 2023 23:24:53 GMT, Ioi Lam wrote: > This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. This pull request has now been integrated. Changeset: 4c96aac9 Author:Ioi Lam URL: https://git.openjdk.org/jdk/commit/4c96aac9c0aa450b0b6859ded8dfff856222ad58 Stats: 696 lines in 8 files changed: 346 ins; 327 del; 23 mod 8320935: Move CDS config initialization code to cdsConfig.cpp Reviewed-by: ccheung, matsaave, stuefe - PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v3]
On Sat, 2 Dec 2023 03:36:05 GMT, Calvin Cheung wrote: >> Ioi Lam 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 seven additional commits >> since the last revision: >> >> - Merge branch 'master' into 8320935-move-cds-config-code-from-arguments-cpp >> - fixed indentation >> - code alignment >> - step4 >> - step3 >> - step2 >> - step1 > > Marked as reviewed by ccheung (Reviewer). Thanks @calvinccheung @matias9927 @tstuefe for the review. - PR Comment: https://git.openjdk.org/jdk/pull/16868#issuecomment-1842107804
Re: RFR: 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC
On Tue, 5 Dec 2023 16:31:24 GMT, Denghui Dong wrote: > Hi, > > Could I have a review of this patch? > > In the current implementation, HeapDumpBeforeFullGC/AfterFullGC will generate > dumps for every FGC, increasing the risk of disk full. > > This patch introduces a new option 'FullGCHeapDumpLimit' to limit the number > of dumps triggered by HeapDumpBeforeFullGC/AfterFullGC to enhance > production-friendliness. > > Best, > Denghui Functional changes seem fine, but I think the test is in the wrong place as it is not a dcmd test. Perhaps just place it in test/hotspot/jtreg/serviceability/HeapDump? - Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16976#pullrequestreview-1766597251
Re: RFR: 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC
On Tue, 5 Dec 2023 16:31:24 GMT, Denghui Dong wrote: > Hi, > > Could I have a review of this patch? > > In the current implementation, HeapDumpBeforeFullGC/AfterFullGC will generate > dumps for every FGC, increasing the risk of disk full. > > This patch introduces a new option 'FullGCHeapDumpLimit' to limit the number > of dumps triggered by HeapDumpBeforeFullGC/AfterFullGC to enhance > production-friendliness. > > Best, > Denghui @D-D-H adding a new manageable flag requires a CSR request to be approved. - PR Comment: https://git.openjdk.org/jdk/pull/16976#issuecomment-1842046395
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]
On Tue, 5 Dec 2023 09:51:50 GMT, Alan Bateman wrote: >> When a virtual thread continues after Thread.yield it currently consumes >> thread's parking permit. This is an oversight, the parking permit should >> only be consumed when continuing after park. >> >> The changes are straight-forward. The internal "RUNNABLE" state is replaced >> with UNPARKED and YIELDED state, effectively encoding the previous state. So >> for the most part, it's just replacing the usages of RUNNABLE. The >> additional states require refactoring tryGetStackTrace, this is the method >> that is used for Thread::getStackTrace when the virtual thread is unmounted. >> It is also changed to not not swallow the REE if the reesubmit fails >> (tryStackTrace has to resubmit as the task for the thread may run, or the >> thread unparked, while "suspended" and sampling its stack trace). The >> changes are a subset of larger changes in the loom repo, future PRs will >> propose to bring in other changes to get main line up to date. >> >> For testing the existing ThreadAPI has new test cases. >> >> Testing: test1-5. This includes the JVMTI tests as it maps the thread states >> to JVMTI thread states. > > Alan Bateman 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 four additional > commits since the last revision: > > - Fix comment, remove space between case labels and colon > - Merge > - Leave onPinned to another PR > - Initial commit Sorry I've deleted my earlier embarassingly stupid comment. The key point of this change is to split RUNNABLE into two states to indicated "runnable after yielding" and "runnable after being unparked". - PR Comment: https://git.openjdk.org/jdk/pull/16953#issuecomment-1842005657
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]
On Tue, 5 Dec 2023 09:51:50 GMT, Alan Bateman wrote: >> When a virtual thread continues after Thread.yield it currently consumes >> thread's parking permit. This is an oversight, the parking permit should >> only be consumed when continuing after park. >> >> The changes are straight-forward. The internal "RUNNABLE" state is replaced >> with UNPARKED and YIELDED state, effectively encoding the previous state. So >> for the most part, it's just replacing the usages of RUNNABLE. The >> additional states require refactoring tryGetStackTrace, this is the method >> that is used for Thread::getStackTrace when the virtual thread is unmounted. >> It is also changed to not not swallow the REE if the reesubmit fails >> (tryStackTrace has to resubmit as the task for the thread may run, or the >> thread unparked, while "suspended" and sampling its stack trace). The >> changes are a subset of larger changes in the loom repo, future PRs will >> propose to bring in other changes to get main line up to date. >> >> For testing the existing ThreadAPI has new test cases. >> >> Testing: test1-5. This includes the JVMTI tests as it maps the thread states >> to JVMTI thread states. > > Alan Bateman 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 four additional > commits since the last revision: > > - Fix comment, remove space between case labels and colon > - Merge > - Leave onPinned to another PR > - Initial commit I find the switch from `RUNNABLE` to `UNPARKED` somewhat unnecessary. I don't see any problem with `RUNNABLE` that needed fixing. I found the distinction and transition between `RUNNABLE` and `RUNNING` to be very clear. ?? - PR Review: https://git.openjdk.org/jdk/pull/16953#pullrequestreview-1766538038
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v3]
On Tue, 5 Dec 2023 23:36:46 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > fixed trailing whitespace src/hotspot/share/prims/jvmtiThreadState.cpp line 561: > 559: // it is an important optimization to create JvmtiThreadState objects > lazily. > 560: // This optimization is disabled when watchpoint capabilities are > present. It is to > 561: // work around a bug with virtual thread frames which can be not > deoptimized in time. Suggestion: "This optimization is *also* disabled when ..." The phrase "which can be not deoptimized in time." is unclear. Are we racing with deoptimization? - PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1416579588
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 23:01:20 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: extended comment to cover the watchpoint extra checks > > Thumbs up. This is a trivial fix. > > You'll need to fix the whitespace complaint before integration. @dcubed-ojdk I would not consider this a trivial fix at all - the need to add the additional conditions is not at all obvious! And even if they were, that would make this a small/simple fix, not "trivial" as defined for the "one review needed" rule. - PR Comment: https://git.openjdk.org/jdk/pull/16961#issuecomment-1841982060
Integrated: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
On Tue, 5 Dec 2023 00:23:45 GMT, Serguei Spitsyn wrote: > This is a trivial fix for a regression caused by: > [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI > ClassLoad event slows down vthread creation by factor 10 > > The fix of 8308614 just triggered a known issue: > [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events > are not always posted with -Xcomp option > > The fix is just a work around with the extra checks with the > `JvmtiExport::should_post_field_access()` and > `JvmtiExport::should_post_field_modification()`. > > Testing: > - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore > with this fix > - In progress: Test with tiers 1-6 This pull request has now been integrated. Changeset: 905137d4 Author:Serguei Spitsyn URL: https://git.openjdk.org/jdk/commit/905137d4065eb40bef6946bdc6bb688d6018a89d Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected Reviewed-by: dcubed - PR: https://git.openjdk.org/jdk/pull/16961
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v3]
> This is a trivial fix for a regression caused by: > [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI > ClassLoad event slows down vthread creation by factor 10 > > The fix of 8308614 just triggered a known issue: > [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events > are not always posted with -Xcomp option > > The fix is just a work around with the extra checks with the > `JvmtiExport::should_post_field_access()` and > `JvmtiExport::should_post_field_modification()`. > > Testing: > - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore > with this fix > - In progress: Test with tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: fixed trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/16961/files - new: https://git.openjdk.org/jdk/pull/16961/files/7d47fd12..34da9c6e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16961=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16961=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961 PR: https://git.openjdk.org/jdk/pull/16961
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 22:51:47 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: extended comment to cover the watchpoint extra checks Dan, thank you a lot for quick review! I'll fix the whitespace issue. - PR Comment: https://git.openjdk.org/jdk/pull/16961#issuecomment-1841792398
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v3]
> This is a simple clean up that moves the code for initializing the CDS config > states from arguments.cpp to cdsConfig.cpp > > I renamed a few functions, but otherwise the code is unchanged. > > - `get_default_shared_archive_path()` -> `default_archive_path()` > - `GetSharedArchivePath()` -> `static_archive_path()` > - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` > > There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is > compiled only if CDS is enabled. Ioi Lam 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 seven additional commits since the last revision: - Merge branch 'master' into 8320935-move-cds-config-code-from-arguments-cpp - fixed indentation - code alignment - step4 - step3 - step2 - step1 - Changes: - all: https://git.openjdk.org/jdk/pull/16868/files - new: https://git.openjdk.org/jdk/pull/16868/files/01dd47bc..a080edeb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16868=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16868=01-02 Stats: 84382 lines in 1756 files changed: 39063 ins; 38780 del; 6539 mod Patch: https://git.openjdk.org/jdk/pull/16868.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16868/head:pull/16868 PR: https://git.openjdk.org/jdk/pull/16868
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 22:51:47 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: extended comment to cover the watchpoint extra checks Thumbs up. This is a trivial fix. You'll need to fix the whitespace complaint before integration. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16961#pullrequestreview-1766264031
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
> This is a trivial fix for a regression caused by: > [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI > ClassLoad event slows down vthread creation by factor 10 > > The fix of 8308614 just triggered a known issue: > [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events > are not always posted with -Xcomp option > > The fix is just a work around with the extra checks with the > `JvmtiExport::should_post_field_access()` and > `JvmtiExport::should_post_field_modification()`. > > Testing: > - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore > with this fix > - In progress: Test with tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: extended comment to cover the watchpoint extra checks - Changes: - all: https://git.openjdk.org/jdk/pull/16961/files - new: https://git.openjdk.org/jdk/pull/16961/files/c08484ac..7d47fd12 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16961=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16961=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961 PR: https://git.openjdk.org/jdk/pull/16961
Integrated: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS
On Thu, 30 Nov 2023 04:30:28 GMT, David Holmes wrote: > Please review this simple clarification to the JVM TI spec regarding use of > `JAVA_TOOL_OPTIONS` in regards to module options and their format. > > I do not believe this clarification needs a CSR request. > > Thanks. This pull request has now been integrated. Changeset: c8fa7581 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/c8fa7581006183d0dabe902c40ab8d7304dfd002 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS Reviewed-by: sspitsyn, alanb - PR: https://git.openjdk.org/jdk/pull/16896
Re: RFR: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS [v3]
On Mon, 4 Dec 2023 23:47:12 GMT, David Holmes wrote: >> Please review this simple clarification to the JVM TI spec regarding use of >> `JAVA_TOOL_OPTIONS` in regards to module options and their format. >> >> I do not believe this clarification needs a CSR request. >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > reviewer feedback Thanks Alan. - PR Comment: https://git.openjdk.org/jdk/pull/16896#issuecomment-1841667405
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]
On Tue, 5 Dec 2023 09:51:50 GMT, Alan Bateman wrote: >> When a virtual thread continues after Thread.yield it currently consumes >> thread's parking permit. This is an oversight, the parking permit should >> only be consumed when continuing after park. >> >> The changes are straight-forward. The internal "RUNNABLE" state is replaced >> with UNPARKED and YIELDED state, effectively encoding the previous state. So >> for the most part, it's just replacing the usages of RUNNABLE. The >> additional states require refactoring tryGetStackTrace, this is the method >> that is used for Thread::getStackTrace when the virtual thread is unmounted. >> It is also changed to not not swallow the REE if the reesubmit fails >> (tryStackTrace has to resubmit as the task for the thread may run, or the >> thread unparked, while "suspended" and sampling its stack trace). The >> changes are a subset of larger changes in the loom repo, future PRs will >> propose to bring in other changes to get main line up to date. >> >> For testing the existing ThreadAPI has new test cases. >> >> Testing: test1-5. This includes the JVMTI tests as it maps the thread states >> to JVMTI thread states. > > Alan Bateman 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 four additional > commits since the last revision: > > - Fix comment, remove space between case labels and colon > - Merge > - Leave onPinned to another PR > - Initial commit The fix looks good to me. Also, it can be more safe to run mach5 tier6 as well. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16953#pullrequestreview-1766053224
Re: RFR: 8320935: Move CDS config initialization code to cdsConfig.cpp [v2]
On Sat, 2 Dec 2023 00:38:58 GMT, Ioi Lam wrote: >> This is a simple clean up that moves the code for initializing the CDS >> config states from arguments.cpp to cdsConfig.cpp >> >> I renamed a few functions, but otherwise the code is unchanged. >> >> - `get_default_shared_archive_path()` -> `default_archive_path()` >> - `GetSharedArchivePath()` -> `static_archive_path()` >> - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()` >> >> There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is >> compiled only if CDS is enabled. > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > fixed indentation Thanks for addressing my comments. Approved! - Marked as reviewed by matsaave (Committer). PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1766049580
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v5]
On Tue, 5 Dec 2023 16:14:31 GMT, Evgeny Astigeevich
wrote:
>> src/hotspot/share/code/codeCache.cpp line 1809:
>>
>>> 1807: }
>>> 1808:
>>> 1809: void CodeCache::write_perf_map(const char* filename) {
>>
>> Why not have a `filename == nullptr` indicate that the default should be
>> used. Then you don't need CodeCache::DefaultPerfMapFile. You can just have a
>> private `CodeCache::defaultPerfmapFileName()` method.
>
> Hi Chris,
> The current design of `write_perf_map` provides a clean and explicit
> interface. The purpose of the function is evident from its signature: to
> write a perf map into a specified file. This explicitness makes the code more
> readable and self-documenting. It reduces the need for developers to go to
> the implementation to figure out: what is the meaning of `nullptr`; where a
> filename will be taken from. It also serves as a contract between the caller
> and the function itself. By explicitly requiring a filename, the function
> sets clear expectations for the caller.
>
> I think `CodeCache::write_default_perf_map` hiding the filename of the
> default perf map might not be a good idea because it makes impossible to get
> the filename used in it. I prefer either method
> `CodeCache::defaultPerfmapFileName()` or class
> `CodeCache::DefaultPerfmapFileName`. The class is simpler to implement than
> the method (like it was earlier).
The default filename was already "hidden" before these changes, so at the very
least things are not being made any worse, but I don't see why any users
`write_perf_map` would ever need the default filename. I just felt that adding
and exporting a class whose only purpose is to provide the default name seemed
like unnecessary overkill. I'm not so sure having a public
CodeCache::defaultPerfmapFileName() API and two `write_perf_map` APIs isn't
overkill also. There is nothing wrong with a null filename argument signally to
use some default name. You can also have the filename arg default to `nullptr`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15871#discussion_r1416228456
Integrated: 8315149: Add hsperf counters for CPU time of internal GC threads
On Mon, 31 Jul 2023 01:50:07 GMT, Jonathan Joo wrote: > 8315149: Add hsperf counters for CPU time of internal GC threads This pull request has now been integrated. Changeset: 9e570105 Author:Jonathan Joo Committer: Man Cao URL: https://git.openjdk.org/jdk/commit/9e570105c30a6e462d08931e2010cef9cd5a6031 Stats: 449 lines in 19 files changed: 444 ins; 4 del; 1 mod 8315149: Add hsperf counters for CPU time of internal GC threads Co-authored-by: Man Cao Co-authored-by: Stefan Johansson Reviewed-by: simonis, manc, sjohanss - PR: https://git.openjdk.org/jdk/pull/15082
RFR: 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC
Hi, Could I have a review of this patch? In the current implementation, HeapDumpBeforeFullGC/AfterFullGC will generate dumps for every FGC, increasing the risk of disk full. This patch introduces a new option 'FullGCHeapDumpLimit' to limit the number of dumps triggered by HeapDumpBeforeFullGC/AfterFullGC to enhance production-friendliness. Best, Denghui - Commit messages: - 8321404: Limit the number of heap dumps triggered by HeapDumpBeforeFullGC/AfterFullGC Changes: https://git.openjdk.org/jdk/pull/16976/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16976=00 Issue: https://bugs.openjdk.org/browse/JDK-8321404 Stats: 53 lines in 3 files changed: 51 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16976.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16976/head:pull/16976 PR: https://git.openjdk.org/jdk/pull/16976
Re: RFR: 8314029: Add file name parameter to Compiler.perfmap [v5]
On Fri, 1 Dec 2023 21:25:19 GMT, Chris Plummer wrote:
>> Yi-Fan Tsai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Apply man changes
>
> src/hotspot/share/code/codeCache.cpp line 1809:
>
>> 1807: }
>> 1808:
>> 1809: void CodeCache::write_perf_map(const char* filename) {
>
> Why not have a `filename == nullptr` indicate that the default should be
> used. Then you don't need CodeCache::DefaultPerfMapFile. You can just have a
> private `CodeCache::defaultPerfmapFileName()` method.
Hi Chris,
The current design of `write_perf_map` provides a clean and explicit interface.
The purpose of the function is evident from its signature: to write a perf map
into a specified file. This explicitness makes the code more readable and
self-documenting. It reduces the need for developers to go to the
implementation to figure out: what is the meaning of `nullptr`; where a
filename will be taken from. It also serves as a contract between the caller
and the function itself. By explicitly requiring a filename, the function sets
clear expectations for the caller.
I think `CodeCache::write_default_perf_map` hiding the filename of the default
perf map might not be a good idea because it makes impossible to get the
filename used in it. I prefer either method
`CodeCache::defaultPerfmapFileName()` or class
`CodeCache::DefaultPerfmapFileName`. The class is simpler to implement than the
method (like it was earlier).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15871#discussion_r1415892122
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v3]
On Tue, 5 Dec 2023 13:21:35 GMT, Thomas Stuefe wrote: >> Joachim Kern has updated the pull request incrementally with one additional >> commit since the last revision: >> >> encapsulate everything in os::Aix::dlopen > > src/hotspot/os/aix/os_aix.cpp line 3133: > >> 3131: return nullptr; >> 3132: } >> 3133: // library not still loaded and still place in array, so load >> library > > s/still/yet No need to be this verbose either, especially since the comment is somewhat misleading. "create entry at end of table" implies that we have a dynamically growing table and allocate new entries. Proposal: "Library not yet loaded; load it, then store its handle in handle table". - PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1415605856
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v2]
On Mon, 4 Dec 2023 12:33:26 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> improve handling of nonexisting files
src/hotspot/os/aix/os_aix.cpp line 203:
> 201: constexpr int max_handletable = 1024;
> 202: static int g_handletable_used = 0;
> 203: static struct handletableentry g_handletable[max_handletable] =
> {{0,0,0,0}};
style nits:
- we usually write the * behind type, not before var name
- `{{0,0}}` -> insert spaces
src/hotspot/os/aix/os_aix.cpp line 1159:
> 1157: result = ::dlopen(filename, dflags);
> 1158: if (result != nullptr) {
> 1159: assert(false, "dll_load: Could not stat() file %s, but dlopen()
> worked; Have to improve stat()", filename);
use assert(result != nullptr) and remove condition
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1413843503
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1413846111
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v3]
On Tue, 5 Dec 2023 12:11:46 GMT, Joachim Kern wrote:
>> On AIX, repeated calls to dlopen referring to the same shared library may
>> result in different, unique dl handles to be returned from libc. In that it
>> differs from typical libc implementations that cache dl handles.
>>
>> This causes problems in the JVM with code that assumes equality of handles.
>> One such problem is in the JVMTI agent handler. That problem was fixed with
>> a local fix to said handler
>> ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this
>> fix causes follow-up problems since it assumes that the file name passed to
>> `os::dll_load()` is the file that has been opened. It prevents efficient,
>> os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality.
>> See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it
>> is a hack that causes other, more uglier hacks to follow (see discussion of
>> https://github.com/openjdk/jdk/pull/16604).
>>
>> We propose a different, cleaner way of handling this:
>>
>> - Handle this entirely inside the AIX versions of os::dll_load and
>> os::dll_unload.
>> - Cache dl handles; repeated opening of a library should return the cached
>> handle.
>> - Increase handle-local ref counter on open, Decrease it on close
>> - Make sure calls to os::dll_load are matched to os::dll_unload (See
>> [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)).
>>
>> This way we mimic dl handle equality as it is implemented on other
>> platforms, and this works for all callers of os::dll_load.
>
> Joachim Kern has updated the pull request incrementally with one additional
> commit since the last revision:
>
> encapsulate everything in os::Aix::dlopen
Excellent, this is how I have pictured a good solution. Very nice.
A number of remarks, but nothing fundamental.
src/hotspot/os/aix/os_aix.cpp line 1137:
> 1135: if (ebuf != nullptr && ebuflen > 0) {
> 1136: ::strncpy(ebuf, "dll_load: empty filename specified", ebuflen -
> 1);
> 1137: }
Are there any cases where we don't hand in the error buffer? If so, I would
just assert ebuf and ebuflen. No need for this kind of flexibility.
src/hotspot/os/aix/os_aix.cpp line 3051:
> 3049:
> 3050: // Simulate the library search algorithm of dlopen() (in os::dll_load)
> 3051: int os::Aix::stat64x_via_LIBPATH(const char* path, struct stat64x*
> stat) {
- no need to export this, make it filescope static
- please return bool, with false = error
- please rename it to something like "search_file_in_LIBPATH"
src/hotspot/os/aix/os_aix.cpp line 3055:
> 3053: return -1;
> 3054:
> 3055: char *path2 = strdup (path);
Please use os::strdup and os::free. If you really intent to use the plain libc
versions, use `::strdup` and `::free` to make sure - and indicate to code
readers - you use the global libc variants.
src/hotspot/os/aix/os_aix.cpp line 3059:
> 3057: int idx = strlen(path2) - 1;
> 3058: if (path2[idx] == ')') {
> 3059: while (path2[idx] != '(' && idx > 0) idx--;
? Why not `strrchr()`?
src/hotspot/os/aix/os_aix.cpp line 3067:
> 3065: if (path2[0] == '/' ||
> 3066: (path2[0] == '.' && (path2[1] == '/' ||
> 3067: (path2[1] == '.' && path2[2] == '/' {
This complexity is not needed, nor is it sufficient, since it does not handle
relative paths ("mydirectory/hallo.so")
https://www.ibm.com/docs/en/aix/7.1?topic=d-dlopen-subroutine
"If FilePath contains a slash character, FilePath is used directly, and no
directories are searched. "
So, just scan for a '/' - if you find one, its a path to be opened directly:
const bool use_as_filepath = strchr(path2, '/');
src/hotspot/os/aix/os_aix.cpp line 3085:
> 3083: strcpy(libpath, env);
> 3084: for (token = strtok_r(libpath, ":", ); token != nullptr;
> token = strtok_r(nullptr, ":", )) {
> 3085: sprintf(combined, "%s/%s", token, path2);
You can save a lot of pain and manual labor by using `stringStream` here.
stringStream combined;
combined.print("%s/%s", token, path2);
const char* combined_path_string = combined.base();
no need for manual allocation and byte counting.
src/hotspot/os/aix/os_aix.cpp line 3099:
> 3097: // filled by os::dll_load(). This way we mimic dl handle equality for a
> library
> 3098: // opened a second time, as it is implemented on other platforms.
> 3099: void* os::Aix::dlopen(const char* filename, int Flags) {
Does not need to be exported, nor does os::AIX::dlclose. Make file scope
static. See my remarks in os_posix.cpp.
src/hotspot/os/aix/os_aix.cpp line 3103:
> 3101: struct stat64x libstat;
> 3102:
> 3103: if (os::Aix::stat64x_via_LIBPATH(filename, )) {
Please return bool, not unix int -1, this hurts my brain :-)
src/hotspot/os/aix/os_aix.cpp line 3108:
> 3106: if (result != nullptr) {
> 3107: assert(false, "dll_load: Could not stat() file %s, but dlopen()
> worked; Have to improve stat()", filename);
> 3108:
Integrated: 8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means
On Thu, 23 Nov 2023 10:25:31 GMT, Alan Bateman wrote: > This is a docs only change to j.l.management.ThreadInfo::isInNative. > > The method currently specifies that it tests if the thread is "executing > native code via the Java Native Interface (JNI)". It would be clearer to say > that it tests if the thread is executing a native method, and expand it to > include native code invoked using the new foreign linker APIs. For now, I've > left out the detail of a downcall handle created with the "critical" linker > option. This pull request has now been integrated. Changeset: 4fbf22b0 Author:Alan Bateman URL: https://git.openjdk.org/jdk/commit/4fbf22b002dab3c6e7e20ed9c7fa4551b6350082 Stats: 16 lines in 1 file changed: 11 ins; 0 del; 5 mod 8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/16791
Re: RFR: JDK-8320890: [AIX] Find a better way to mimic dl handle equality [v3]
> On AIX, repeated calls to dlopen referring to the same shared library may > result in different, unique dl handles to be returned from libc. In that it > differs from typical libc implementations that cache dl handles. > > This causes problems in the JVM with code that assumes equality of handles. > One such problem is in the JVMTI agent handler. That problem was fixed with a > local fix to said handler > ([JDK-8315706](https://bugs.openjdk.org/browse/JDK-8315706)). However, this > fix causes follow-up problems since it assumes that the file name passed to > `os::dll_load()` is the file that has been opened. It prevents efficient, > os_aix.cpp-local workarounds for other AIX issues like the *.so/*.a duality. > See [JDK-8320005](https://bugs.openjdk.org/browse/JDK-8320005). As such, it > is a hack that causes other, more uglier hacks to follow (see discussion of > https://github.com/openjdk/jdk/pull/16604). > > We propose a different, cleaner way of handling this: > > - Handle this entirely inside the AIX versions of os::dll_load and > os::dll_unload. > - Cache dl handles; repeated opening of a library should return the cached > handle. > - Increase handle-local ref counter on open, Decrease it on close > - Make sure calls to os::dll_load are matched to os::dll_unload (See > [JDK-8320830](https://bugs.openjdk.org/browse/JDK-8320830)). > > This way we mimic dl handle equality as it is implemented on other platforms, > and this works for all callers of os::dll_load. Joachim Kern has updated the pull request incrementally with one additional commit since the last revision: encapsulate everything in os::Aix::dlopen - Changes: - all: https://git.openjdk.org/jdk/pull/16920/files - new: https://git.openjdk.org/jdk/pull/16920/files/0f6716db..2d32c43b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16920=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16920=01-02 Stats: 175 lines in 2 files changed: 90 ins; 82 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16920.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16920/head:pull/16920 PR: https://git.openjdk.org/jdk/pull/16920
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit [v2]
> When a virtual thread continues after Thread.yield it currently consumes > thread's parking permit. This is an oversight, the parking permit should only > be consumed when continuing after park. > > The changes are straight-forward. The internal "RUNNABLE" state is replaced > with UNPARKED and YIELDED state, effectively encoding the previous state. So > for the most part, it's just replacing the usages of RUNNABLE. The additional > states require refactoring tryGetStackTrace, this is the method that is used > for Thread::getStackTrace when the virtual thread is unmounted. It is also > changed to not not swallow the REE if the reesubmit fails (tryStackTrace has > to resubmit as the task for the thread may run, or the thread unparked, while > "suspended" and sampling its stack trace). The changes are a subset of larger > changes in the loom repo, future PRs will propose to bring in other changes > to get main line up to date. > > For testing the existing ThreadAPI has new test cases. > > Testing: test1-5. This includes the JVMTI tests as it maps the thread states > to JVMTI thread states. Alan Bateman 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 four additional commits since the last revision: - Fix comment, remove space between case labels and colon - Merge - Leave onPinned to another PR - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/16953/files - new: https://git.openjdk.org/jdk/pull/16953/files/45c6072a..057c9a57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16953=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16953=00-01 Stats: 2035 lines in 49 files changed: 1448 ins; 293 del; 294 mod Patch: https://git.openjdk.org/jdk/pull/16953.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16953/head:pull/16953 PR: https://git.openjdk.org/jdk/pull/16953
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit
On Mon, 4 Dec 2023 20:25:56 GMT, ExE Boss wrote: >> When a virtual thread continues after Thread.yield it currently consumes >> thread's parking permit. This is an oversight, the parking permit should >> only be consumed when continuing after park. >> >> The changes are straight-forward. The internal "RUNNABLE" state is replaced >> with UNPARKED and YIELDED state, effectively encoding the previous state. So >> for the most part, it's just replacing the usages of RUNNABLE. The >> additional states require refactoring tryGetStackTrace, this is the method >> that is used for Thread::getStackTrace when the virtual thread is unmounted. >> It is also changed to not not swallow the REE if the reesubmit fails >> (tryStackTrace has to resubmit as the task for the thread may run, or the >> thread unparked, while "suspended" and sampling its stack trace). The >> changes are a subset of larger changes in the loom repo, future PRs will >> propose to bring in other changes to get main line up to date. >> >> For testing the existing ThreadAPI has new test cases. >> >> Testing: test1-5. This includes the JVMTI tests as it maps the thread states >> to JVMTI thread states. > > src/hotspot/share/classfile/javaClasses.cpp line 1998: > >> 1996: case UNPARKED: >> 1997: case YIELDING : >> 1998: case YIELDED: > > Suggestion: > > case UNPARKED : > case YIELDING : > case YIELDED : I need to check the Hotspot style guide but I think the prevailing convention is no space between the label and the colon. - PR Review Comment: https://git.openjdk.org/jdk/pull/16953#discussion_r1415177431
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit
On Mon, 4 Dec 2023 21:33:52 GMT, Serguei Spitsyn wrote: > Just want to make sure this change is intentional. Before the comment was: > `// runnable-mounted`. Well spotted, the wrong comment was moved. - PR Review Comment: https://git.openjdk.org/jdk/pull/16953#discussion_r1415146065
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit
On Mon, 4 Dec 2023 16:08:32 GMT, Alan Bateman wrote: > When a virtual thread continues after Thread.yield it currently consumes > thread's parking permit. This is an oversight, the parking permit should only > be consumed when continuing after park. > > The changes are straight-forward. The internal "RUNNABLE" state is replaced > with UNPARKED and YIELDED state, effectively encoding the previous state. So > for the most part, it's just replacing the usages of RUNNABLE. The additional > states require refactoring tryGetStackTrace, this is the method that is used > for Thread::getStackTrace when the virtual thread is unmounted. It is also > changed to not not swallow the REE if the reesubmit fails (tryStackTrace has > to resubmit as the task for the thread may run, or the thread unparked, while > "suspended" and sampling its stack trace). The changes are a subset of larger > changes in the loom repo, future PRs will propose to bring in other changes > to get main line up to date. > > For testing the existing ThreadAPI has new test cases. > > Testing: test1-5. This includes the JVMTI tests as it maps the thread states > to JVMTI thread states. src/hotspot/share/classfile/javaClasses.cpp line 1998: > 1996: case UNPARKED: > 1997: case YIELDING : > 1998: case YIELDED: Suggestion: case UNPARKED : case YIELDING : case YIELDED : - PR Review Comment: https://git.openjdk.org/jdk/pull/16953#discussion_r1414448245
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
On Tue, 5 Dec 2023 04:41:29 GMT, David Holmes wrote:
>> The fix is for a regression caused by:
>> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI
>> ClassLoad event slows down vthread creation by factor 10
>>
>> The fix of 8308614 just triggered a known issue:
>> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events
>> are not always posted with -Xcomp option
>>
>> The fix is just a work around with the extra checks with the
>> `JvmtiExport::should_post_field_access()` and
>> `JvmtiExport::should_post_field_modification()`.
>>
>> Testing:
>> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail
>> anymore with this fix
>> - In progress: Test with tiers 1-6
>
> src/hotspot/share/prims/jvmtiThreadState.cpp line 562:
>
>> 560: if (JvmtiThreadState::seen_interp_only_mode() ||
>> 561: JvmtiExport::should_post_field_access() ||
>> 562: JvmtiExport::should_post_field_modification()){
>
> The comment needs updating to explain the extra checks.
>
> Can't say I see the connection with
> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) as no `-Xcomp` is
> involved in the current failures AFAICS.
Thank you for the suggestion. I'll add a comment.
The `-Xcomp` flag is not a root cause but a trigger. There is a general issue
that a frame can be not deoptimized. The `-Xcomp` is a stress that helps to
reproduce the issue as the `-Xcomp` option and `interp_only_mode` works in
opposite directions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1415125933
Re: RFR: 8299426: Heap dump does not contain virtual Thread stack references [v2]
On 04/12/2023 18:59, Chris Plummer wrote: So does this mean if the application is no longer referencing the ExecutorService, then we can have unreachable virtual threads that have not completed? This is really the point I've been getting at. Yes, this is possible. It would be an unusual scenario of course. -Alan
