Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]

[Tobias Hartmann]

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

I think if we just rely on reviews, NULLs will slip through again and we would 
need to have regular cleanup PRs. 

Doug's idea seems simple enough to implement in Skara/jcheck. An alternative to 
whitelisting would be a warning in the offending PR or a requirement for 
"special approvement" of such changes (for example, via a Skara command).

-

PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1573169963

Re: RFR: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit [v2]

[Chris Plummer]

> Virtual threads are always daemon threads, so tests that previously did not 
> explicitly wait for test threads to exit sometimes fail with virtual threads 
> due to the test exiting before the test threads have exited. A join() for 
> each test thread is needed to fix this issue.
> 
> com/sun/jdi/DeferredStepTest.java is one such tests. I looked at the other 
> com/sun/jdi failures listed in 
> [JDK-8285422](https://bugs.openjdk.org/browse/JDK-8285422) and didn't see any 
> others that might be failing for this same reason.
> 
> I tested locally with `JTREG_TEST_THREAD_FACTORY=Virtual`. I'll also run the 
> appropriate mach5 tier that tests com/sun/jdi with virtual threads.

Chris Plummer has updated the pull request incrementally with one additional 
commit since the last revision:

  some variable renaming

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14275/files
  - new: https://git.openjdk.org/jdk/pull/14275/files/24dfa505..f83cd766

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14275=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=14275=00-01

  Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/14275.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14275/head:pull/14275

PR: https://git.openjdk.org/jdk/pull/14275

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v6]

[Chris Plummer]

On Fri, 2 Jun 2023 03:44:48 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> The CSR is:
>> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
>> JVMTI.agent_load should obey EnableDynamicAgentLoading
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn 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:
> 
>  - Merge
>  - review: use output.shouldHaveExitValue(0) in the test
>  - review: use output.shouldContain()
>  - move check for EnableDynamicAgentLoading to invoke_Agent_OnAttach
>  - minor renaming in new test TestJcmdNoAgentLoad.java
>  - 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading

Looks good.

-

Marked as reviewed by cjplummer (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14244#pullrequestreview-1456621661

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v6]

[Serguei Spitsyn]

> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
> allow deployment to choose whether to allow agents to be loaded/started in 
> the VM. The VM option does the right thing for tools using the Attach API but 
> jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading 
> JVMTI agents when the EnableDynamicAgentLoading is false.
> 
> The CSR is:
> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
> JVMTI.agent_load should obey EnableDynamicAgentLoading
> 
> Testing:
>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

Serguei Spitsyn 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:

 - Merge
 - review: use output.shouldHaveExitValue(0) in the test
 - review: use output.shouldContain()
 - move check for EnableDynamicAgentLoading to invoke_Agent_OnAttach
 - minor renaming in new test TestJcmdNoAgentLoad.java
 - 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14244/files
  - new: https://git.openjdk.org/jdk/pull/14244/files/6712f24a..18cad064

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14244=05
 - incr: https://webrevs.openjdk.org/?repo=jdk=14244=04-05

  Stats: 8499 lines in 119 files changed: 7217 ins; 645 del; 637 mod
  Patch: https://git.openjdk.org/jdk/pull/14244.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14244/head:pull/14244

PR: https://git.openjdk.org/jdk/pull/14244

Re: RFR: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit

[Serguei Spitsyn]

On Thu, 1 Jun 2023 23:03:47 GMT, Chris Plummer  wrote:

> Virtual threads are always daemon threads, so tests that previously did not 
> explicitly wait for test threads to exit sometimes fail with virtual threads 
> due to the test exiting before the test threads have exited. A join() for 
> each test thread is needed to fix this issue.
> 
> com/sun/jdi/DeferredStepTest.java is one such tests. I looked at the other 
> com/sun/jdi failures listed in 
> [JDK-8285422](https://bugs.openjdk.org/browse/JDK-8285422) and didn't see any 
> others that might be failing for this same reason.
> 
> I tested locally with `JTREG_TEST_THREAD_FACTORY=Virtual`. I'll also run the 
> appropriate mach5 tier that tests com/sun/jdi with virtual threads.

This looks good modulo the comment from Alex about renaming.
Thanks,
Serguei

-

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14275#pullrequestreview-1456594772

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v5]

[Serguei Spitsyn]

> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
> allow deployment to choose whether to allow agents to be loaded/started in 
> the VM. The VM option does the right thing for tools using the Attach API but 
> jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading 
> JVMTI agents when the EnableDynamicAgentLoading is false.
> 
> The CSR is:
> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
> JVMTI.agent_load should obey EnableDynamicAgentLoading
> 
> Testing:
>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: use output.shouldHaveExitValue(0) in the test

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14244/files
  - new: https://git.openjdk.org/jdk/pull/14244/files/062b3f85..6712f24a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14244=04
 - incr: https://webrevs.openjdk.org/?repo=jdk=14244=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14244.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14244/head:pull/14244

PR: https://git.openjdk.org/jdk/pull/14244

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v4]

[Serguei Spitsyn]

On Thu, 1 Jun 2023 23:14:23 GMT, Chris Plummer  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: use output.shouldContain()
>
> test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 57:
> 
>> 55: OutputAnalyzer output = JcmdBase.jcmd(jcmdArgs);
>> 56: 
>> 57: assertEquals(output.getExitValue(), 0);
> 
> I think tests normally just use `output.shouldHaveExitValue(0);`

Thank you for the suggestion. I've derived this code from one of the existing 
jcmd tests.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1213871217

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v4]

[Kelvin Nilsen]

On Thu, 1 Jun 2023 14:27:12 GMT, Thomas Stuefe  wrote:

>> Kelvin Nilsen has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Force PLAB sizes to align on card-table size
>
> test/hotspot/jtreg/gc/shenandoah/oom/TestClassLoaderLeak.java line 132:
> 
>> 130:  {{"iu"},   {"adaptive", "aggressive"}},
>> 131:  {{"passive"},  {"passive"}},
>> 132:  {{"generational"}, {"adaptive"}}
> 
> Curious, here and in similar places, why only test adaptive heuristic for 
> generational, if we test satb with all variants?

Generational mode only works with adaptive heuristic.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213863860

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v4]

[Kelvin Nilsen]

On Thu, 1 Jun 2023 18:39:05 GMT, William Kemper  wrote:

>> src/hotspot/share/gc/shared/gcConfiguration.cpp line 88:
>> 
>>> 86: }
>>> 87: #endif
>>> 88: return NA;
>> 
>> You moved the order between Shenandoah and ZGC in `young_collector()`, so 
>> you should probably do the same here.
>
> Fixed. Thank you for the review.

Thanks.  We've made your suggested change.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213862862

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v4]

[Kelvin Nilsen]

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

Kelvin Nilsen has updated the pull request incrementally with one additional 
commit since the last revision:

  Force PLAB sizes to align on card-table size

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14185/files
  - new: https://git.openjdk.org/jdk/pull/14185/files/5bf6e7e0..d4d2f1cf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14185=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=14185=02-03

  Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/14185.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14185/head:pull/14185

PR: https://git.openjdk.org/jdk/pull/14185

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v3]

[Kelvin Nilsen]

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

Kelvin Nilsen has updated the pull request incrementally with one additional 
commit since the last revision:

  Assert bounds only when allocations succeed, increase test timeouts (#2)

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14185/files
  - new: https://git.openjdk.org/jdk/pull/14185/files/eb656ec2..5bf6e7e0

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14185=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=14185=01-02

  Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/14185.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14185/head:pull/14185

PR: https://git.openjdk.org/jdk/pull/14185

Re: RFR: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit

[Alex Menkov]

On Thu, 1 Jun 2023 23:03:47 GMT, Chris Plummer  wrote:

> Virtual threads are always daemon threads, so tests that previously did not 
> explicitly wait for test threads to exit sometimes fail with virtual threads 
> due to the test exiting before the test threads have exited. A join() for 
> each test thread is needed to fix this issue.
> 
> com/sun/jdi/DeferredStepTest.java is one such tests. I looked at the other 
> com/sun/jdi failures listed in 
> [JDK-8285422](https://bugs.openjdk.org/browse/JDK-8285422) and didn't see any 
> others that might be failing for this same reason.
> 
> I tested locally with `JTREG_TEST_THREAD_FACTORY=Virtual`. I'll also run the 
> appropriate mach5 tier that tests com/sun/jdi with virtual threads.

test/jdk/com/sun/jdi/DeferredStepTest.java line 80:

> 78: Thread jj2 = TestScaffold.newThread(obj2, "jj2");
> 79: jj1.start();
> 80: jj2.start();

It looks strange that there are no errors about conflict between variable names 
and class names.
Anyway I think it would be better to rename the variables

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14275#discussion_r1213789142

Re: RFR: 8308978: regression with a deadlock involving FollowReferences [v3]

[Alex Menkov]

On Wed, 31 May 2023 21:33:13 GMT, Alex Menkov  wrote:

>> The change fixes regression from JDK-8299414.
>> There is a deadlock between JvmtiVTMSTransitionDisabler and EscapeBarrier 
>> when virtual threads are in mount/unmount transition:
>> EscapeBarrier requests deoptimization which requires thread suspension.
>> JvmtiVTMSTransitionDisabler ctor waits until all in progress VTMS 
>> transitions complete, but they cannot be completed as thread is suspended.
>> To avoid the deadlock mount/unmount transitions should be completed before 
>> EscapeBarrier stuff.
>
> Alex Menkov 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 eight additional 
> commits since the last revision:
> 
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - Merge branch 'openjdk:master' into follow_ref_deadlock
>  - Merge branch 'follow_ref_deadlock' of github.com:alexmenkov/jdk into 
> follow_ref_deadlock
>  - fix
>  - unproblem-list tests
>  - fix
>  - unproblem-list tests
>  - fix

tier 1-5 passed (there are 3 failed tests, but they are not related to 
FollowReferences)

-

PR Comment: https://git.openjdk.org/jdk/pull/14233#issuecomment-1572934722

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v4]

[Chris Plummer]

On Thu, 1 Jun 2023 22:51:23 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> The CSR is:
>> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
>> JVMTI.agent_load should obey EnableDynamicAgentLoading
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: use output.shouldContain()

test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 57:

> 55: OutputAnalyzer output = JcmdBase.jcmd(jcmdArgs);
> 56: 
> 57: assertEquals(output.getExitValue(), 0);

I think tests normally just use `output.shouldHaveExitValue(0);`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1213766546

RFR: 8309329: com/sun/jdi/DeferredStepTest.java fails with virtual threads due to not waiting for threads to exit

[Chris Plummer]

Virtual threads are always daemon threads, so tests that previously did not 
explicitly wait for test threads to exit sometimes fail with virtual threads 
due to the test exiting before the test threads have exited. A join() for each 
test thread is needed to fix this issue.

com/sun/jdi/DeferredStepTest.java is one such tests. I looked at the other 
com/sun/jdi failures listed in 
[JDK-8285422](https://bugs.openjdk.org/browse/JDK-8285422) and didn't see any 
others that might be failing for this same reason.

I tested locally with `JTREG_TEST_THREAD_FACTORY=Virtual`. I'll also run the 
appropriate mach5 tier that tests com/sun/jdi with virtual threads.

-

Commit messages:
 - Use join() to make sure test threads have exited

Changes: https://git.openjdk.org/jdk/pull/14275/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=14275=00
  Issue: https://bugs.openjdk.org/browse/JDK-8309329
  Stats: 12 lines in 2 files changed: 9 ins; 1 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/14275.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14275/head:pull/14275

PR: https://git.openjdk.org/jdk/pull/14275

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v4]

[Serguei Spitsyn]

> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
> allow deployment to choose whether to allow agents to be loaded/started in 
> the VM. The VM option does the right thing for tools using the Attach API but 
> jcmd JVMTI.agent_load was missed. This should be fixed to disallow loading 
> JVMTI agents when the EnableDynamicAgentLoading is false.
> 
> The CSR is:
> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
> JVMTI.agent_load should obey EnableDynamicAgentLoading
> 
> Testing:
>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: use output.shouldContain()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14244/files
  - new: https://git.openjdk.org/jdk/pull/14244/files/be0ec0c9..062b3f85

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14244=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=14244=02-03

  Stats: 15 lines in 1 file changed: 0 ins; 14 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14244.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14244/head:pull/14244

PR: https://git.openjdk.org/jdk/pull/14244

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v2]

[Serguei Spitsyn]

On Wed, 31 May 2023 22:08:43 GMT, Chris Plummer  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   minor renaming in new test TestJcmdNoAgentLoad.java
>
> test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 68:
> 
>> 66: System.out.println("Found output line with the expected 
>> error message:\n" + line);
>> 67: }
>> 68: }
> 
> Why not just use  `OutputAnalyzer.shouldContain()`?

Good suggestion, thanks. I've not found this method. :)

> test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 70:
> 
>> 68: }
>> 69: if (!seenPattern) {
>> 70: throw new RuntimeException("Not found expected error message 
>> in output");
> 
> "Not found" -> "Did not find"
> 
> Also, you should include the expected error message in the output.

Okay. But I've replaced this code with `OutputAnalyzer.shouldContain()` as you 
suggested.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1213750622
PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1213751619

Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]

[Mikhailo Seledtsov]

On Tue, 23 May 2023 09:04:11 GMT, Severin Gehwolf  wrote:

>> Please review these test changes which implement automatic testing of 
>> container resource updates without JVM restart. Note that this merely tests 
>> container detection code handling this case. It doesn't do anything special 
>> for the JVM itself, though it might make sense to add some sanity checks 
>> should we detect certain limits changing. In another PR, though.
>> 
>> As to the test design, it works similar to the shared temp tests: Interact 
>> between the two containers by virtue of a shared filesystem `/tmp` and 
>> creating marker files there in order to make them cooperate. Note that the 
>> new test needs `podman` version `4.3.0` and better (`4.5` is current).
>> 
>> Testing:
>>  - [x] GHA
>>  - [x] Linux x86_64 container tests on cg v1 and cg v2 system
>>  - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and 
>> `docker`)
>
> Severin Gehwolf 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8308090-tests-container-on-fly-updates
>  - Fix whitespace
>  - 8308090: Add container tests for on-the-fly resource quota updates

Changes look good to me. Thank you for adding these tests.

-

Marked as reviewed by mseledtsov (Committer).

PR Review: https://git.openjdk.org/jdk/pull/14090#pullrequestreview-1456374316

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v9]

[Chris Plummer]

On Thu, 1 Jun 2023 16:56:17 GMT, Alan Bateman  wrote:

>> This is the implementation for JEP 451. There are two parts to this:
>> 
>> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded 
>> into a running VM. For JVM TI, the message is printed to stderr from 
>> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
>> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
>> includes an update to the JVM TI spec and API docs to require the warning.
>> 
>> 2. If running with -Djdk.instrument.traceUsage or 
>> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
>> a trace message and stack trace.
>> 
>> Testing: tier1-6
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add comment to invoke_Agent_OnAttach

Marked as reviewed by cjplummer (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/13899#pullrequestreview-1456185001

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v2]

[William Kemper]

On Thu, 1 Jun 2023 10:12:02 GMT, Stefan Karlsson  wrote:

>> Kelvin Nilsen has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Make the order of young/old collector checks consistent (#1)
>
> src/hotspot/share/gc/shared/gcConfiguration.cpp line 88:
> 
>> 86: }
>> 87: #endif
>> 88: return NA;
> 
> You moved the order between Shenandoah and ZGC in `young_collector()`, so you 
> should probably do the same here.

Fixed. Thank you for the review.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213538282

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental) [v2]

[Kelvin Nilsen]

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

Kelvin Nilsen has updated the pull request incrementally with one additional 
commit since the last revision:

  Make the order of young/old collector checks consistent (#1)

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14185/files
  - new: https://git.openjdk.org/jdk/pull/14185/files/aa85a907..eb656ec2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14185=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=14185=00-01

  Stats: 16 lines in 1 file changed: 7 ins; 8 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/14185.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14185/head:pull/14185

PR: https://git.openjdk.org/jdk/pull/14185

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)

[Y . Srinivas Ramakrishna]

On Thu, 1 Jun 2023 12:15:37 GMT, Martin Doerr  wrote:

> Issues already reported to GenShen engineers:
> 
> gc/shenandoah/TestElasticTLAB.java#generational #  Internal Error 
> (src\hotspot\share\gc\shenandoah\shenandoahFreeSet.cpp:695), pid=23288, 
> tid=23784 #  assert(size % CardTable::card_size_in_words() == 0) failed: size 
> must be multiple of card table size, was 258
> 
> gc/stress/gcold/TestGCOldWithShenandoah.java#generational #  Internal Error 
> (src\hotspot\share\gc\shenandoah\heuristics\shenandoahOldHeuristics.cpp:82), 
> pid=20828, tid=5836 #  assert(_old_generation->available() > 
> old_evacuation_budget) failed: Cannot budget more than is available
> 
> gc/shenandoah/oom/TestAllocOutOfMemory.java#large Execution failed: `main' 
> threw exception: java.lang.RuntimeException: 'java.lang.OutOfMemoryError: 
> Java heap space' missing from stdout/stderr (Issue with 64k Pages)
> 
> gc/shenandoah/TestRetainObjects.java#no-tlab 
> gc/shenandoah/TestSieveObjects.java#no-tlab Timeouts.
> 
> gc/shenandoah/TestAllocObjects.java#generational 
> gc/shenandoah/TestDynamicSoftMaxHeapSize.java#generational #  Internal Error 
> src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp:664), pid=18434, 
> tid=29955 #  assert(is_global() || 
> ShenandoahHeap::heap()->is_full_gc_in_progress() || (_used + _humongous_waste 
> <= _affiliated_region_count * ShenandoahHeapRegion::region_size_bytes())) 
> failed: used cannot exceed regions

Thanks @TheRealMDoerr ; could you specify the platforms on which you see these 
failures, so we have a better chance at reproducing them? Thanks!

-

PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1572528277

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v9]

[Alan Bateman]

> This is the implementation for JEP 451. There are two parts to this:
> 
> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded into 
> a running VM. For JVM TI, the message is printed to stderr from 
> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
> includes an update to the JVM TI spec and API docs to require the warning.
> 
> 2. If running with -Djdk.instrument.traceUsage or 
> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
> a trace message and stack trace.
> 
> Testing: tier1-6

Alan Bateman has updated the pull request incrementally with one additional 
commit since the last revision:

  Add comment to invoke_Agent_OnAttach

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13899/files
  - new: https://git.openjdk.org/jdk/pull/13899/files/cbd9bb1d..71f9886a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=13899=08
 - incr: https://webrevs.openjdk.org/?repo=jdk=13899=07-08

  Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/13899.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13899/head:pull/13899

PR: https://git.openjdk.org/jdk/pull/13899

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v8]

[Chris Plummer]

On Thu, 1 Jun 2023 12:37:20 GMT, Alan Bateman  wrote:

>> This is the implementation for JEP 451. There are two parts to this:
>> 
>> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded 
>> into a running VM. For JVM TI, the message is printed to stderr from 
>> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
>> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
>> includes an update to the JVM TI spec and API docs to require the warning.
>> 
>> 2. If running with -Djdk.instrument.traceUsage or 
>> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
>> a trace message and stack trace.
>> 
>> Testing: tier1-6
>
> 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 19 additional 
> commits since the last revision:
> 
>  - Split is_loaded, typos in comments
>  - Merge
>  - Add impl note to document the XX option
>  - Cleanup
>  - Merge
>  - Allow for warning to be skipped when same agent loaded a second/subsequent 
> time
>  - Merge
>  - Tweak javadoc, update test to use more test infra
>  - Merge
>  - Merge
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/25f0fc82...cbd9bb1d

Except for the possible addition of some comments regarding what is meant by 
"loaded", the changes look good.

-

Marked as reviewed by cjplummer (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13899#pullrequestreview-1455815464

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v3]

[Chris Plummer]

On Wed, 31 May 2023 23:39:19 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> The CSR is:
>> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
>> JVMTI.agent_load should obey EnableDynamicAgentLoading
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   move check for EnableDynamicAgentLoading to invoke_Agent_OnAttach

Sorry, previous comment was for another review.

-

Changes requested by cjplummer (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14244#pullrequestreview-1455813557

Integrated: 8309146: extend JDI StackFrame.setValue() and JDWP StackFrame.setValues minimal support for virtual threads

[Chris Plummer]

On Tue, 30 May 2023 23:44:28 GMT, Chris Plummer  wrote:

> The JDWP spec for StackFrame.SetValue currently states:
> 
> "If the thread is a virtual thread then this command can be used to 
> set "
> "the value of local variables in the top-most frame when the thread 
> is "
> "suspended at a breakpoint or single step event. The target VM may 
> support "
> "setting local variables in other cases."
> 
> The JDI spec for StackFrame.setValue() has similar wording. In 
> [JDK-8308814](https://bugs.openjdk.org/browse/JDK-8308814) the JVMTI spec 
> clarified support to be for a thread suspended at any event, not just a 
> breakpoint or single step. That same clarification is needed in the JDWP and 
> JDI specs. No implementation changes are needed.

This pull request has now been integrated.

Changeset: e8271649
Author:Chris Plummer 
URL:   
https://git.openjdk.org/jdk/commit/e8271649e00771a8bbee240aa1bbbc27a672b22a
Stats: 5 lines in 2 files changed: 0 ins; 0 del; 5 mod

8309146: extend JDI StackFrame.setValue() and JDWP StackFrame.setValues minimal 
support for virtual threads

Reviewed-by: sspitsyn, alanb

-

PR: https://git.openjdk.org/jdk/pull/14235

Integrated: 8308232: nsk/jdb tests don't pass -verbose flag to the debuggee

[Chris Plummer]

On Wed, 31 May 2023 00:27:16 GMT, Chris Plummer  wrote:

> The nsk/jdb tests are not passing the test args on to the debuggee. I found a 
> way to pass all of them (see the CR for details), but Windows had trouble 
> with the parsing. It turns out the only args that the debuggee really cares 
> about are -verbose and -waittime, so I settled on just passing these two, and 
> Windows is happy.
> 
> Note only a handful of tests run with -verbose as the default. However, with 
> this change at least now if you add -verbose when debugging an issue, you 
> will see verbose log output from the debuggee. Previously you would not.

This pull request has now been integrated.

Changeset: c6f20db9
Author:Chris Plummer 
URL:   
https://git.openjdk.org/jdk/commit/c6f20db945c6217aea84cebd6c97dbf8b93c48a4
Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod

8308232: nsk/jdb tests don't pass -verbose flag to the debuggee

Reviewed-by: sspitsyn, lmesnik

-

PR: https://git.openjdk.org/jdk/pull/14236

Re: RFR: 8308232: nsk/jdb tests don't pass -verbose flag to the debuggee

[Chris Plummer]

On Wed, 31 May 2023 00:27:16 GMT, Chris Plummer  wrote:

> The nsk/jdb tests are not passing the test args on to the debuggee. I found a 
> way to pass all of them (see the CR for details), but Windows had trouble 
> with the parsing. It turns out the only args that the debuggee really cares 
> about are -verbose and -waittime, so I settled on just passing these two, and 
> Windows is happy.
> 
> Note only a handful of tests run with -verbose as the default. However, with 
> this change at least now if you add -verbose when debugging an issue, you 
> will see verbose log output from the debuggee. Previously you would not.

Thanks for the reviews Serguei and Leonid!

-

PR Comment: https://git.openjdk.org/jdk/pull/14236#issuecomment-1572270309

Re: RFR: 8309146: extend JDI StackFrame.setValue() and JDWP StackFrame.setValues minimal support for virtual threads

[Alan Bateman]

On Tue, 30 May 2023 23:44:28 GMT, Chris Plummer  wrote:

> The JDWP spec for StackFrame.SetValue currently states:
> 
> "If the thread is a virtual thread then this command can be used to 
> set "
> "the value of local variables in the top-most frame when the thread 
> is "
> "suspended at a breakpoint or single step event. The target VM may 
> support "
> "setting local variables in other cases."
> 
> The JDI spec for StackFrame.setValue() has similar wording. In 
> [JDK-8308814](https://bugs.openjdk.org/browse/JDK-8308814) the JVMTI spec 
> clarified support to be for a thread suspended at any event, not just a 
> breakpoint or single step. That same clarification is needed in the JDWP and 
> JDI specs. No implementation changes are needed.

Marked as reviewed by alanb (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/14235#pullrequestreview-1455759449

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v3]

[Chris Plummer]

On Wed, 31 May 2023 23:39:19 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> The CSR is:
>> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
>> JVMTI.agent_load should obey EnableDynamicAgentLoading
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   move check for EnableDynamicAgentLoading to invoke_Agent_OnAttach

Except for the possible addition of some comments regarding what is meant by 
"loaded", the changes look good.

-

Marked as reviewed by cjplummer (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14244#pullrequestreview-1455750113

Re: RFR: 8304438: jcmd JVMTI.agent_load should obey EnableDynamicAgentLoading [v2]

[Chris Plummer]

On Wed, 31 May 2023 10:30:22 GMT, Serguei Spitsyn  wrote:

>> The VM option EnableDynamicAgentLoading was added in JDK 9, default true, to 
>> allow deployment to choose whether to allow agents to be loaded/started in 
>> the VM. The VM option does the right thing for tools using the Attach API 
>> but jcmd JVMTI.agent_load was missed. This should be fixed to disallow 
>> loading JVMTI agents when the EnableDynamicAgentLoading is false.
>> 
>> The CSR is:
>> [JDK-8309250](https://bugs.openjdk.org/browse/JDK-8309250): jcmd 
>> JVMTI.agent_load should obey EnableDynamicAgentLoading
>> 
>> Testing:
>>  - run new test `test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java`
>>  - TBD: submit mach5 tiers 1-5 to make sure no new regressions are introduced
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   minor renaming in new test TestJcmdNoAgentLoad.java

test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 68:

> 66: System.out.println("Found output line with the expected 
> error message:\n" + line);
> 67: }
> 68: }

Why not just use  `OutputAnalyzer.shouldContain()`?

test/jdk/sun/tools/jcmd/TestJcmdNoAgentLoad.java line 70:

> 68: }
> 69: if (!seenPattern) {
> 70: throw new RuntimeException("Not found expected error message 
> in output");

"Not found" -> "Did not find"

Also, you should include the expected error message in the output.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1212367038
PR Review Comment: https://git.openjdk.org/jdk/pull/14244#discussion_r1212367594

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Chris Plummer]

On Thu, 1 Jun 2023 06:46:41 GMT, Serguei Spitsyn  wrote:

>>> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. 
>>> Isn't the library always already loaded by the time we get here (the assert 
>>> below seems to imply that)? 
>> 
>> JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and 
>> adds it to the agent list if is succeeds. The test that you are looking is 
>> checking the list of already loaded agents. Maybe the function name 
>> "is_loaded" is the confusion here, maybe a better name is needed.
>
> I see the source of my confusion.
> The function `invoke_Agent_OnAttach` is called from the `JvmtiAgent::load`.
> But at the time of `invoke_Agent_OnAttach` the agent has not been added to 
> the list yet.
> It is added after the `JvmtiAgent::load` with the function 
> `JvmtiAgentList::add`:
> 
> jint JvmtiAgentList::load_agent(const char* agent_name, const char* absParam,
>const char* options, outputStream* st) {
>   // The abs parameter should be "true" or "false"
>   const bool is_absolute_path = (absParam != nullptr) && (strcmp(absParam, 
> "true") == 0);
>   JvmtiAgent* const agent = new JvmtiAgent(agent_name, options, 
> is_absolute_path, /* dynamic agent */ true);
>   if (agent->load(st)) {
> add(agent);
>   . . .
> 
> Now I see the code is correct.
> I'd suggest to add a small comment before the line 512 saying
> that the agent has not been included into the agents list yet.

> JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and 
> adds it to the agent list if is succeeds. The test that you are looking is 
> checking the list of already loaded agents. Maybe the function name 
> "is_loaded" is the confusion here, maybe a better name is needed.

Yes, I did notice that there were two different `is_loaded` checks going on. 
What's not clear is the relation between agent->is_loaded() and 
JvmtiAgentList::is_loaded(library). I assume that at this point in time the 
library is not on the list but the agent is loaded, so that's why it works, but 
it's not clear to me why this is the case. I think a comment here explaining 
why would be helpful so the reader doesn't need to track down when each of 
these "loaded" conditions becomes true.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1213314470

Re: RFR: 8308286 Fix clang warnings in linux code [v3]

[Weijun Wang]

On Thu, 1 Jun 2023 15:02:16 GMT, Artem Semenov  wrote:

>> src/java.security.jgss/share/native/libj2gss/gssapi.h line 47:
>> 
>>> 45: 
>>> 46: // Condition was copied from
>>> 47: // 
>>> Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/gssapi/gssapi.h
>> 
>> In the current version of the file above, it's written as
>> 
>> #if defined(__APPLE__) && (defined(__ppc__) ||...
>> 
>> Is it better?
>
> done

I didn't ask to revert the change. It's `s/TARGET_OS_MAC/defined(__APPLE__)/`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1213300305

Re: RFR: 8308286 Fix clang warnings in linux code [v3]

[Artem Semenov]

On Wed, 31 May 2023 13:52:39 GMT, Weijun Wang  wrote:

>> Artem Semenov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update
>
> src/java.security.jgss/share/native/libj2gss/gssapi.h line 47:
> 
>> 45: 
>> 46: // Condition was copied from
>> 47: // 
>> Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/gssapi/gssapi.h
> 
> In the current version of the file above, it's written as
> 
> #if defined(__APPLE__) && (defined(__ppc__) ||...
> 
> Is it better?

done

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1213297788

Re: RFR: 8308286 Fix clang warnings in linux code [v4]

[Artem Semenov]

> When using the clang compiler to build OpenJDk on Linux, we encounter various 
> "warnings as errors".
> They can be fixed with small changes.

Artem Semenov has updated the pull request incrementally with one additional 
commit since the last revision:

  update

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/14033/files
  - new: https://git.openjdk.org/jdk/pull/14033/files/d5b70cb2..b423bd4c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=14033=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=14033=02-03

  Stats: 7 lines in 2 files changed: 1 ins; 4 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/14033.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14033/head:pull/14033

PR: https://git.openjdk.org/jdk/pull/14033

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v8]

[Serguei Spitsyn]

On Thu, 1 Jun 2023 12:37:20 GMT, Alan Bateman  wrote:

>> This is the implementation for JEP 451. There are two parts to this:
>> 
>> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded 
>> into a running VM. For JVM TI, the message is printed to stderr from 
>> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
>> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
>> includes an update to the JVM TI spec and API docs to require the warning.
>> 
>> 2. If running with -Djdk.instrument.traceUsage or 
>> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
>> a trace message and stack trace.
>> 
>> Testing: tier1-6
>
> 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 19 additional 
> commits since the last revision:
> 
>  - Split is_loaded, typos in comments
>  - Merge
>  - Add impl note to document the XX option
>  - Cleanup
>  - Merge
>  - Allow for warning to be skipped when same agent loaded a second/subsequent 
> time
>  - Merge
>  - Tweak javadoc, update test to use more test infra
>  - Merge
>  - Merge
>  - ... and 9 more: https://git.openjdk.org/jdk/compare/6cea493a...cbd9bb1d

The update looks good.
Thanks,
Serguei

-

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13899#pullrequestreview-1455647274

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)

[Thomas Stuefe]

On Fri, 26 May 2023 20:46:29 GMT, Kelvin Nilsen  wrote:

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

I did a first read through the tests to check if any test changes affect 
traditional Shenandoah. To see if regression tests for non-generational are 
unchanged.

All good, I did not find anything noteworthy.

test/hotspot/jtreg/gc/shenandoah/TestEvilSyncBug.java line 33:

> 31:  * @modules java.base/jdk.internal.misc
> 32:  *  java.management
> 33:  * @run driver/timeout=480 TestEvilSyncBug 
> -XX:ShenandoahGCHeuristics=aggressive

Probably fine, but why this change to non-generational testing? Will aggressive 
heuristic sharpen the test?

test/hotspot/jtreg/gc/shenandoah/mxbeans/TestChurnNotifications.java line 169:

> 167: 
> 168: MemoryUsage before = getUsage(mapBefore);
> 169: MemoryUsage after = getUsage(mapAfter);

This also changes test logic for traditional Shenandoah, but its harmless. 
Nit: more precise would be to require "Young Gen" pool to only exist for 
-XX:ShenandoahGCMode=generational.

test/hotspot/jtreg/gc/shenandoah/oom/TestAllocOutOfMemory.java line 23:

> 21:  * questions.
> 22:  *
> 23:  */

Three tests folded into one, but it does not look like functionality changed 
for testing traditional Shenandoah. Okay.

test/hotspot/jtreg/gc/shenandoah/oom/TestAllocOutOfMemory.java line 92:

> 90: expectFailure("-Xmx16m",
> 91:   "-XX:+UnlockExperimentalVMOptions",
> 92:   "-XX:+UseShenandoahGC",

Nit: should not need UnlockExperimentalVMOptions anymore.

test/hotspot/jtreg/gc/shenandoah/oom/TestClassLoaderLeak.java line 132:

> 130:  {{"iu"},   {"adaptive", "aggressive"}},
> 131:  {{"passive"},  {"passive"}},
> 132:  {{"generational"}, {"adaptive"}}

Curious, here and in similar places, why only test adaptive heuristic for 
generational, if we test satb with all variants?

-

PR Review: https://git.openjdk.org/jdk/pull/14185#pullrequestreview-1455490699
PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213164085
PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213232252
PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213243280
PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213242053
PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1213244749

Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]

[JoKern65]

On Fri, 26 May 2023 08:31:46 GMT, JoKern65  wrote:

>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk 
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small 
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the 
>> OpenJDK C/C++ code.
>
> JoKern65 has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   forgotton _

Hi,

As this PR is big and spans several components I split
off the java.base, java.desktop and the sercivability/security
issues  into extra JBS issues. 
https://bugs.openjdk.org/browse/JDK-8309219 Fix xlc17 clang 1.5 warnings in 
java.base
https://bugs.openjdk.org/browse/JDK-8309224 Fix xlc17 clang 1.5 warnings in 
java.desktop
https://bugs.openjdk.org/browse/JDK-8309225 Fix xlc17 clang 1.5 warnings in 
security and servicability
I’ll move the changes from this pull request into new pull requests.
I will incorporate the requested changes right in the new PRs.
I will reuse this issue 8308388 for the hotspot changes but come up
with a new, smaller PR.
@colleenp, I will move alloca.h to the globalDefinitions_xlc.hpp.
@prrace, I will come up with an identical PR for the client
files (java.desktop), but improve the comment as @kimbarrett proposed
@mbaesken, I will use AIX and take up some of the other fixes you proposed.

I guess we need to find a way to fix the issue with the malloc 
in globalDefinitions_xlc.hpp in the upcoming PR for hotspot.  

Thanks for your help so far!

Hi,

As this PR is big and spans several components I split
off the java.base, java.desktop and the sercivability/security
issues into extra JBS issues.
https://bugs.openjdk.org/browse/JDK-8309219 Fix xlc17 clang 1.5 warnings in 
java.base
https://bugs.openjdk.org/browse/JDK-8309224 Fix xlc17 clang 1.5 warnings in 
java.desktop
https://bugs.openjdk.org/browse/JDK-8309225 Fix xlc17 clang 1.5 warnings in 
security and servicability
I’ll move the changes from this pull request into new pull requests.
I will incorporate the requested changes right in the new PRs.
I will reuse this issue 8308388 for the hotspot changes but come up
with a new, smaller PR.
@colleenp, I will move alloca.h to the globalDefinitions_xlc.hpp.
@prrace, I will come up with an identical PR for the client
files (java.desktop), but improve the comment as @kimbarrett proposed
@MBaesken, I will use AIX and take up some of the other fixes you proposed.

I guess we need to find a way to fix the issue with the malloc
in globalDefinitions_xlc.hpp in the upcoming PR for hotspot.

Thanks for your help so far!

-

PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1572023812
PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1572024628

Withdrawn: JDK-8308288: Fix xlc17 clang warnings in shared code

[JoKern65]

On Thu, 25 May 2023 09:14:14 GMT, JoKern65  wrote:

> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk 
> on AIX , we run into various "warnings as errors".
> Some of those are in shared codebase and could be addressed by small 
> adjustments.
> A lot of those changes are in hotspot, some might be somewhere else in the 
> OpenJDK C/C++ code.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.org/jdk/pull/14146

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)

[Thomas Stuefe]

On Fri, 26 May 2023 20:46:29 GMT, Kelvin Nilsen  wrote:

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

Hi Kevin,

First off, kudos. This is impressive work by you and your Amazon colleagues!

I have one particular worry, though, how to verify that this experimental 
feature does not cause regressions with traditional Shenandoah? 

The PR is massive (+18kloc) and targeted for JDK 21. Rampdown P1 is in a week. 
By all accounts, JDK 21 will be a massive release, so we will all have our 
hands full, fixing stuff and plugging holes.

Oracle did put the sources for the Generational ZGC beside the old sources, 
thereby somewhat guaranteeing traditional ZGC does not regress. Could we follow 
the same cautionary process here?

I am not a Shenandoah expert, but to me the new feature seems intertwined with 
normal code paths. It's difficult to ensure, via review, that traditional 
Shenandoah will not suffer regressions. So close to rampdown this is a bit 
scary.

The JEP mentions several "Risks and Assumptions", but it is unclear whether 
these risks also affect traditional Shenandoah.

Cheers, Thomas

-

PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1572008074

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v6]

[Alan Bateman]

On Thu, 1 Jun 2023 05:55:43 GMT, Alan Bateman  wrote:

>> src/hotspot/share/prims/jvmtiAgentList.cpp line 231:
>> 
>>> 229:if (agent->is_static_lib() && agent->is_loaded()) {
>>> 230:  return true;
>>> 231:}
>> 
>> This doesn't make sense to me. If  you pass in `null` for `os_lib`, then we 
>> return true if any loaded static lib is found. Is this an attempt to limit 
>> the warning to just the first static lib that is loaded? Also, why would 
>> `null` ever be passed in if there wasn't at least one static lib. Some 
>> clarify comments would be useful.
>
> load_agent_from_executable has a comment to explain how statically linked 
> agents are started, that's why it needs to use agent->is_static_lib().&& 
> agent->is_loaded() here. There isn't currently a way to test this but there 
> is other work going to support static builds so it might be possible to write 
> some automated tests at that point.

I've updated this so that is_loaded is split into one function to test if a 
statically linked agent is already loaded and another for agent libraries. That 
might be clearer to understand.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1213080073

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v8]

[Alan Bateman]

> This is the implementation for JEP 451. There are two parts to this:
> 
> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded into 
> a running VM. For JVM TI, the message is printed to stderr from 
> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
> includes an update to the JVM TI spec and API docs to require the warning.
> 
> 2. If running with -Djdk.instrument.traceUsage or 
> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
> a trace message and stack trace.
> 
> Testing: tier1-6

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 19 additional commits since the 
last revision:

 - Split is_loaded, typos in comments
 - Merge
 - Add impl note to document the XX option
 - Cleanup
 - Merge
 - Allow for warning to be skipped when same agent loaded a second/subsequent 
time
 - Merge
 - Tweak javadoc, update test to use more test infra
 - Merge
 - Merge
 - ... and 9 more: https://git.openjdk.org/jdk/compare/eb72f87a...cbd9bb1d

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13899/files
  - new: https://git.openjdk.org/jdk/pull/13899/files/2d9d5922..cbd9bb1d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=13899=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=13899=06-07

  Stats: 2137 lines in 162 files changed: 788 ins; 840 del; 509 mod
  Patch: https://git.openjdk.org/jdk/pull/13899.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13899/head:pull/13899

PR: https://git.openjdk.org/jdk/pull/13899

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)

[Martin Doerr]

On Fri, 26 May 2023 20:46:29 GMT, Kelvin Nilsen  wrote:

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

Issues already reported to GenShen engineers:

gc/shenandoah/TestElasticTLAB.java#generational
#  Internal Error (src\hotspot\share\gc\shenandoah\shenandoahFreeSet.cpp:695), 
pid=23288, tid=23784
#  assert(size % CardTable::card_size_in_words() == 0) failed: size must be 
multiple of card table size, was 258

gc/stress/gcold/TestGCOldWithShenandoah.java#generational
#  Internal Error 
(src\hotspot\share\gc\shenandoah\heuristics\shenandoahOldHeuristics.cpp:82), 
pid=20828, tid=5836
#  assert(_old_generation->available() > old_evacuation_budget) failed: Cannot 
budget more than is available

gc/shenandoah/oom/TestAllocOutOfMemory.java#large
Execution failed: `main' threw exception: java.lang.RuntimeException: 
'java.lang.OutOfMemoryError: Java heap space' missing from stdout/stderr
(Issue with 64k Pages)

gc/shenandoah/TestRetainObjects.java#no-tlab
gc/shenandoah/TestSieveObjects.java#no-tlab
Timeouts.

gc/shenandoah/TestAllocObjects.java#generational
gc/shenandoah/TestDynamicSoftMaxHeapSize.java#generational
#  Internal Error 
src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp:664), pid=18434, 
tid=29955
#  assert(is_global() || ShenandoahHeap::heap()->is_full_gc_in_progress() || 
(_used + _humongous_waste <= _affiliated_region_count * 
ShenandoahHeapRegion::region_size_bytes())) failed: used cannot exceed regions

-

PR Comment: https://git.openjdk.org/jdk/pull/14185#issuecomment-1571947174

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)

[Stefan Karlsson]

On Fri, 26 May 2023 20:46:29 GMT, Kelvin Nilsen  wrote:

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

I've looked at the shared code and it's really nice that you've managed to keep 
them to a minimum. I have one tiny nit that would be nice to fix.

src/hotspot/share/gc/shared/gcConfiguration.cpp line 88:

> 86: }
> 87: #endif
> 88: return NA;

You moved the order between Shenandoah and ZGC in `young_collector()`, so you 
should probably do the same here.

-

PR Review: https://git.openjdk.org/jdk/pull/14185#pullrequestreview-1455087651
PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1212919666

Re: RFR: JDK-8307314: Implementation: Generational Shenandoah (Experimental)

[Andrew Haley]

On Fri, 26 May 2023 20:46:29 GMT, Kelvin Nilsen  wrote:

> OpenJDK Colleagues:
> 
> Please review this proposed integration of Generational mode for Shenandoah 
> GC under https://bugs.openjdk.org/browse/JDK-8307314.
> 
> Generational mode of Shenandoah is enabled by adding 
> `-XX:+UnlockExperimentalVMOptions -XX:ShenandoahGCMode=generational` to a 
> command line that already specifies ` -XX:+UseShenandoahGC`.  The 
> implementation automatically adjusts the sizes of old generation and young 
> generation to efficiently utilize the entire heap capacity.  Generational 
> mode of Shenandoah resembles G1 in the following regards:
> 
> 1. Old-generation marking runs concurrently during the time that multiple 
> young generation collections run to completion.
> 2. After old-generation marking completes, we perform a sequence of mixed 
> collections.  Each mixed collection combines collection of young generation 
> with evacuation of a portion of the old-generation regions identified for 
> collection based on old-generation marking information.
> 3. Unlike G1, young-generation collections and evacuations are entirely 
> concurrent, as with single-generation Shenandoah.
> 4. As with single-generation Shenandoah, there is no explicit notion of eden 
> and survivor space within the young generation.  In practice, regions that 
> were most recently allocated tend to have large amounts of garbage and these 
> regions tend to be collected with very little effort.  Young-generation 
> objects that survive garbage collection tend to accumulate in regions that 
> hold survivor objects.  These regions tend to have smaller amounts of 
> garbage, and are less likely to be collected.  If they survive a sufficient 
> number of young-generation collections, the “survivor” regions are promoted 
> into the old generation.
> 
> We expect to refine heuristics as we gain experience with more production 
> workloads.  In the future, we plan to remove the “experimental” qualifier 
> from generational mode, at which time we expect that generational mode will 
> become the default mode for Shenandoah.
> 
> **Testing**: We continuously run jtreg tiers 1-4 + hotspot_gc_shenandoah, 
> gcstress, jck compiler, jck runtime, Dacapo, SpecJBB, SpecVM, Extremem, 
> HyperAlloc, and multiple AWS production workload simulators. We test on Linux 
> x64 and aarch64, Alpine x64 and aarch64, macOS x64 and aarch64, and Windows 
> x64.

src/hotspot/cpu/x86/gc/shenandoah/shenandoahBarrierSetAssembler_x86.cpp line 
147:

> 145:   }
> 146: #endif
> 147: }

This logic is so gnarly that it's very hard to review and maintain, and IMO 
it's dangerous.

The problem is that its correctness depends on exactly how registers are 
allocated in its caller. This needs restructuring so that the register 
allocation is defined in a single place then passed down to everyone who needs 
it.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14185#discussion_r1212922771

Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]

[Doug Simon]

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: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]

[Johan Sjölen]

On Thu, 1 Jun 2023 05:23:25 GMT, Tobias Hartmann  wrote:

> What's the plan now to prevent re-introducing `NULL`?

Hi Tobias. The only plan in place is social, the reviewers have to look out for 
it. I am however researching how to do this through machine. I'm currently 
researching ways of preventing any re-introductions by machine. These include 
poisoning the NULL macro by re-defining it and finding a tool which is capable 
of parsing C++ code which is yet to go through the pre-processor.

-

PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571722147

Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]

[Andrew Haley]

On Tue, 30 May 2023 00:57:40 GMT, David Holmes  wrote:

> Can we now poison NULL so it can't get reintroduced? Or would that 
> potentially break standard headers?

I'm sure it would. Maybe some changes to Skara?

-

PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1571706648

RFR: 8309271: A way to align already compiled methods with compiler directives

[Dmitry Chuyko]

Compiler Control (https://openjdk.org/jeps/165) provides method-context 
dependent control of the JVM compilers (C1 and C2). The active directive stack 
is built from the directive files passed with the `-XX:CompilerDirectivesFile` 
diagnostic command-line option and the Compiler.add_directives diagnostic 
command. It is also possible to clear all directives or remove the top from the 
stack.

A matching directive will be applied at method compilation time when such 
compilation is started. If directives are added or changed, but compilation 
does not start, then the state of compiled methods doesn't correspond to the 
rules. This is not an error, and it happens in long running applications when 
directives are added or removed after compilation of methods that could be 
matched. For example, the user decides that C2 compilation needs to be disabled 
for some method due to a compiler bug, issues such a directive but this does 
not affect the application behavior. In such case, the target application needs 
to be restarted, and such an operation can have high costs and risks. Another 
goal is testing/debugging compilers.

It would be convenient to optionally reconcile at least existing matching 
nmethods to the current stack of compiler directives. Methods in general are 
often inlined, and this information is hard to track down.

Natural way to eliminate the discrepancy between the result of compilation and 
the broken rule is to discard the compilation result, i.e. deoptimization. 
Obviously there is a performance penalty, so it should be applied with care. 
Hot code will most likely be recompiled soon, as nothing happens to its hotness.

A new flag '`-d`' has beed introduced for some directives related to compile 
commands: `Compiler.add_directives`, `Compiler.remove_directives`, 
`Compiler.clear_directives`. The default behavior has not changed (no flag). If 
the new flag is present, the command scans already compiled methods and marks 
for deoptimization those methods that have any active non-default matching 
compiler directives. There is currently no distinction which directives are 
found. In particular, this means that if there are rules for inlining into some 
method, it will be deoptimized. On the other hand, if there are rules for a 
method and it was inlined, top-level methods won't be deoptimized, but this can 
be achieved by having rules for them.

In addition, a new diagnistic command `Compiler.replace_directives`, has been 
added for convenience. It's like a combination of `Compiler.clear_directives` 
and `Compiler.add_directives`. It supports the same new optional '-d' flag that 
marks both cleared and added methods for deoptimization.

The behavior of the '-d' flag is implemented in the new 
`CodeCache::mark_for_deoptimization_directives_matches` and 
`DirectivesStack::hasMatchingDirectives` methods.

`CompilerDirectivesDCMDTest` now checks add, remove and replace commands in two 
modes (default and '-d') and checks that '-d' flag causes deoptimization.

An alternative approach to the '-d' flag could be to have a special diagnostic 
command for deoptimization. It will get a list of method patterns and reuse the 
matcher, however this is not so trivial. Overall usage and effects will be 
similar but this is one more file format. The user will also need to monitor or 
query active directives in advance, e.g. to deoptimize all mentioned methods 
after clearing all directives.

An alternative approach for selection of deoptimized methods could be to track 
down all inlining dependencies. This may be similar to searching references to 
old methods, but it requires scanning all code blobs, which looks too expensive.

An alternative naming for the flag is welcome. The obvious '-f' ('force') 
unfortunately has a conflict. Other verbs can be 'update', 'refresh' or 
'apply'. Deoptimization is just what's done to reconcile the state. It could be 
something else, like first compiling with a different compiler and then 
switching to that version. Although in the latter case, triggered compilation 
would be an essential detail.

-

Commit messages:
 - Merge branch 'openjdk:master' into compiler-directives-force-update
 - Formatting
 - Formatting
 - Merge branch 'openjdk:master' into compiler-directives-force-update
 - Merge branch 'openjdk:master' into compiler-directives-force-update
 - Correct arguments info for new commands
 - Update through de-optimization

Changes: https://git.openjdk.org/jdk/pull/14111/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=14111=00
  Issue: https://bugs.openjdk.org/browse/JDK-8309271
  Stats: 214 lines in 9 files changed: 194 ins; 0 del; 20 mod
  Patch: https://git.openjdk.org/jdk/pull/14111.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111

PR: https://git.openjdk.org/jdk/pull/14111

Re: Clarifying jmethodID Usage and Potential JVM Crashes

[David Holmes]

On 1/06/2023 5:16 pm, Jaroslav Bachorík wrote:

Hi David,

On Thu, Jun 1, 2023 at 3:56 AM David Holmes > wrote:


Hi Jaroslav,

On 31/05/2023 9:12 pm, Jaroslav Bachorík wrote:
 > Dear Team,
 >
 > I've been investigating the unusual JVM crashes occurring in
JVMTI calls
 > on a J9 JVM. During my investigation, I scrutinized the `jmethodID`
 > definition closely, available here: [jmethodID
 >
definition](https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID 
 
>).
 >
 > To paraphrase, the definition suggests that `jmethodID` identifies a
 > Java method, initializer, or constructor. These identifiers, once
 > returned by JVM TI functions and events, can be safely stored.
However,
 > when the class is unloaded, they become invalid, rendering them
 > unsuitable for use.
 >
 > My interpretation is that the JVMTI user should verify the
validity of a
 > `jmethodID` value before using it to prevent potential crashes.
Would
 > you agree with this interpretation?

Not quite - as you note you can't verify the jmethodID validity. What
the user needs to do, in line with what Dan was saying, is ensure that
they keep track of the classes to which the methods belong and keep
them
alive if necessary. Now that may be easier said than done, but that is
the gist of it. This comes from the JNI spec:

"A field or method ID does not prevent the VM from unloading the class
from which the ID has been derived. After the class is unloaded, the
method or field ID becomes invalid and may not be passed to any
function
taking such an ID. The native code, therefore, must make sure to:

      keep a live reference to the underlying class, or
      recompute the method or field ID

if it intends to use a method or field ID for an extended period of
time."

 > This sounds like a sensible requirement, but its practical
application
 > remains unclear. As far as I know, methods can be unloaded
concurrently
 > to the native code executing JVMTI functions. This introduces a
 > potential race condition where the JVM unloads the methods during
the
 > check->use flow, making it only a partial solution. To complicate
 > matters further, no method exists to confirm whether a
`jmethodID` is valid.
 >
 > Theoretically, we could monitor the `CompiledMethodUnload` event to
 > track the validity state, creating a constantly expanding set of
 > unloaded `jmethodID` values or a bloom filter, if one does not care
 > about few potential false positives. This strategy, however, doesn't
 > address the potential race condition, and it could even
exacerbate it
 > due to possible event delays. This delay might mistakenly validate a
 > `jmethodID` value that has already been unloaded, but for which the
 > event hasn't been delivered yet.
 >
 > Honestly, I don't see a way to use `jmethodID` safely unless the
code
 > using it suspends the entire JVM and doesn't resume until it's
finished
 > with that `jmethodID`. Any other approach might lead to JVM
crashes, as
 > we've observed with J9.
 >
 > Lastly, it's noteworthy that Hotspot takes meticulous measures to
ensure
 > that using jmethodIDs for unloaded methods doesn't crash the JVM and
 > even provides useful information. This observation has led me to
 > question whether the documentation aligns with the Hotspot
 > implementation, especially given that following closely the
 > documentation appears to increase the risk associated with the
use of
 > `jmethodID` values.

There have been folk who wanted to make this area more user-friendly
but
that shouldn't be mistaken for moving towards a world where jmethodIDs
are always safe to use.


Yes, I see your point. Unfortunately, this confirms my worries that 
using AsyncGetCallTrace (ASGCT) on a system strictly adhering to the 
JVMTI spec of jmethoID is not really possible without risking random and 
quite frequent crashes on systems with concurrent class unloading enabled.
FTR, ASGCT will record the stack trace as a list of frames, each one 
containing the corresponding jmethodID value. Considering that the most 
common usage of ASGCT is in a signal handler it makes it impossible to 
use JVMTI calls to resolve the holder class and create a strong 
reference to prevent it from being unloaded.
And even if this would be possible we would need to figure out when to 
release the class reference when it is no more needed - and it is not 
really clear how we could do that reliably, leaving 

Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]

[Severin Gehwolf]

On Thu, 1 Jun 2023 02:16:12 GMT, David Holmes  wrote:

>> Anyone willing to review this?
>
> @jerboaa I can't really review the tests themselves but will run through our 
> CI to see if they cause any problems. If not then they should be okay to add.

Thanks @dholmes-ora for running them through your CI.

-

PR Comment: https://git.openjdk.org/jdk/pull/14090#issuecomment-1571584032

Re: Clarifying jmethodID Usage and Potential JVM Crashes

[Thomas Stüfe]

On Thu, Jun 1, 2023 at 9:17 AM Jaroslav Bachorík <
jaroslav.bacho...@datadoghq.com> wrote:

> Hi David,
>
> On Thu, Jun 1, 2023 at 3:56 AM David Holmes 
> wrote:
>
>> Hi Jaroslav,
>>
>> On 31/05/2023 9:12 pm, Jaroslav Bachorík wrote:
>> > Dear Team,
>> >
>> > I've been investigating the unusual JVM crashes occurring in JVMTI
>> calls
>> > on a J9 JVM. During my investigation, I scrutinized the `jmethodID`
>> > definition closely, available here: [jmethodID
>> > definition](
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID
>> <
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID
>> >).
>> >
>> > To paraphrase, the definition suggests that `jmethodID` identifies a
>> > Java method, initializer, or constructor. These identifiers, once
>> > returned by JVM TI functions and events, can be safely stored. However,
>> > when the class is unloaded, they become invalid, rendering them
>> > unsuitable for use.
>> >
>> > My interpretation is that the JVMTI user should verify the validity of
>> a
>> > `jmethodID` value before using it to prevent potential crashes. Would
>> > you agree with this interpretation?
>>
>> Not quite - as you note you can't verify the jmethodID validity. What
>> the user needs to do, in line with what Dan was saying, is ensure that
>> they keep track of the classes to which the methods belong and keep them
>> alive if necessary. Now that may be easier said than done, but that is
>> the gist of it. This comes from the JNI spec:
>>
>> "A field or method ID does not prevent the VM from unloading the class
>> from which the ID has been derived. After the class is unloaded, the
>> method or field ID becomes invalid and may not be passed to any function
>> taking such an ID. The native code, therefore, must make sure to:
>>
>>  keep a live reference to the underlying class, or
>>  recompute the method or field ID
>>
>> if it intends to use a method or field ID for an extended period of time."
>>
>> > This sounds like a sensible requirement, but its practical application
>> > remains unclear. As far as I know, methods can be unloaded concurrently
>> > to the native code executing JVMTI functions. This introduces a
>> > potential race condition where the JVM unloads the methods during the
>> > check->use flow, making it only a partial solution. To complicate
>> > matters further, no method exists to confirm whether a `jmethodID` is
>> valid.
>> >
>> > Theoretically, we could monitor the `CompiledMethodUnload` event to
>> > track the validity state, creating a constantly expanding set of
>> > unloaded `jmethodID` values or a bloom filter, if one does not care
>> > about few potential false positives. This strategy, however, doesn't
>> > address the potential race condition, and it could even exacerbate it
>> > due to possible event delays. This delay might mistakenly validate a
>> > `jmethodID` value that has already been unloaded, but for which the
>> > event hasn't been delivered yet.
>> >
>> > Honestly, I don't see a way to use `jmethodID` safely unless the code
>> > using it suspends the entire JVM and doesn't resume until it's finished
>> > with that `jmethodID`. Any other approach might lead to JVM crashes, as
>> > we've observed with J9.
>> >
>> > Lastly, it's noteworthy that Hotspot takes meticulous measures to
>> ensure
>> > that using jmethodIDs for unloaded methods doesn't crash the JVM and
>> > even provides useful information. This observation has led me to
>> > question whether the documentation aligns with the Hotspot
>> > implementation, especially given that following closely the
>> > documentation appears to increase the risk associated with the use of
>> > `jmethodID` values.
>>
>> There have been folk who wanted to make this area more user-friendly but
>> that shouldn't be mistaken for moving towards a world where jmethodIDs
>> are always safe to use.
>>
>
> Yes, I see your point. Unfortunately, this confirms my worries that using
> AsyncGetCallTrace (ASGCT) on a system strictly adhering to the JVMTI spec
> of jmethoID is not really possible without risking random and quite
> frequent crashes on systems with concurrent class unloading enabled.
> FTR, ASGCT will record the stack trace as a list of frames, each one
> containing the corresponding jmethodID value. Considering that the most
> common usage of ASGCT is in a signal handler it makes it impossible to use
> JVMTI calls to resolve the holder class and create a strong reference to
> prevent it from being unloaded.
> And even if this would be possible we would need to figure out when to
> release the class reference when it is no more needed - and it is not
> really clear how we could do that reliably, leaving us with the option of
> holding the class references indefinitely or risking crashing JVM.
>
> I want to emphasize that not being able to resolve additional details for
> a jmethodID pointing to a method of an unloaded class is not an issue, as
> long as the 

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Serguei Spitsyn]

On Wed, 31 May 2023 15:01:37 GMT, Alan Bateman  wrote:

>> This is the implementation for JEP 451. There are two parts to this:
>> 
>> 1. A multi-line warning is printed when a JVM TI or Java agent is loaded 
>> into a running VM. For JVM TI, the message is printed to stderr from 
>> JvmtiAgent::load. For Java agents, it is printed to System.err (as that may 
>> be redirected) in the JPLIS (j.l.instrumentation) implementation. This part 
>> includes an update to the JVM TI spec and API docs to require the warning.
>> 
>> 2. If running with -Djdk.instrument.traceUsage or 
>> -Djdk.instrument.traceUsage=true, the calls to the Instrumentation API print 
>> a trace message and stack trace.
>> 
>> Testing: tier1-6
>
> 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 17 additional 
> commits since the last revision:
> 
>  - Add impl note to document the XX option
>  - Cleanup
>  - Merge
>  - Allow for warning to be skipped when same agent loaded a second/subsequent 
> time
>  - Merge
>  - Tweak javadoc, update test to use more test infra
>  - Merge
>  - Merge
>  - Refresh package description
>  - Merge
>  - ... and 7 more: https://git.openjdk.org/jdk/compare/381973e4...2d9d5922

The update looks good to me.
Posted one nit though.
Thanks,
Serguei

test/jdk/com/sun/tools/attach/warnings/DynamicLoadWarningTest.java line 67:

> 65: class DynamicLoadWarningTest {
> 66: private static final String JVMTI_AGENT_WARNING = "WARNING: A JVM TI 
> agent has been dynamically loaded";
> 67: private static final String JAVA_AGENT_WARNING = "WARNING: A Java 
> agent has been loaded dynamically";

Nit: The warnings can be unified to say "loaded dynamically" or "dynamically 
loaded" in the same order.
Of course, it also impacts the files: `src/hotspot/share/prims/jvmtiAgent.cpp` 
and `src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java`.

-

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13899#pullrequestreview-1454785048
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212717874

Re: Clarifying jmethodID Usage and Potential JVM Crashes

[Jaroslav Bachorík]

Hi David,

On Thu, Jun 1, 2023 at 3:56 AM David Holmes  wrote:

> Hi Jaroslav,
>
> On 31/05/2023 9:12 pm, Jaroslav Bachorík wrote:
> > Dear Team,
> >
> > I've been investigating the unusual JVM crashes occurring in JVMTI calls
> > on a J9 JVM. During my investigation, I scrutinized the `jmethodID`
> > definition closely, available here: [jmethodID
> > definition](
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#jmethodID
>  >).
> >
> > To paraphrase, the definition suggests that `jmethodID` identifies a
> > Java method, initializer, or constructor. These identifiers, once
> > returned by JVM TI functions and events, can be safely stored. However,
> > when the class is unloaded, they become invalid, rendering them
> > unsuitable for use.
> >
> > My interpretation is that the JVMTI user should verify the validity of a
> > `jmethodID` value before using it to prevent potential crashes. Would
> > you agree with this interpretation?
>
> Not quite - as you note you can't verify the jmethodID validity. What
> the user needs to do, in line with what Dan was saying, is ensure that
> they keep track of the classes to which the methods belong and keep them
> alive if necessary. Now that may be easier said than done, but that is
> the gist of it. This comes from the JNI spec:
>
> "A field or method ID does not prevent the VM from unloading the class
> from which the ID has been derived. After the class is unloaded, the
> method or field ID becomes invalid and may not be passed to any function
> taking such an ID. The native code, therefore, must make sure to:
>
>  keep a live reference to the underlying class, or
>  recompute the method or field ID
>
> if it intends to use a method or field ID for an extended period of time."
>
> > This sounds like a sensible requirement, but its practical application
> > remains unclear. As far as I know, methods can be unloaded concurrently
> > to the native code executing JVMTI functions. This introduces a
> > potential race condition where the JVM unloads the methods during the
> > check->use flow, making it only a partial solution. To complicate
> > matters further, no method exists to confirm whether a `jmethodID` is
> valid.
> >
> > Theoretically, we could monitor the `CompiledMethodUnload` event to
> > track the validity state, creating a constantly expanding set of
> > unloaded `jmethodID` values or a bloom filter, if one does not care
> > about few potential false positives. This strategy, however, doesn't
> > address the potential race condition, and it could even exacerbate it
> > due to possible event delays. This delay might mistakenly validate a
> > `jmethodID` value that has already been unloaded, but for which the
> > event hasn't been delivered yet.
> >
> > Honestly, I don't see a way to use `jmethodID` safely unless the code
> > using it suspends the entire JVM and doesn't resume until it's finished
> > with that `jmethodID`. Any other approach might lead to JVM crashes, as
> > we've observed with J9.
> >
> > Lastly, it's noteworthy that Hotspot takes meticulous measures to ensure
> > that using jmethodIDs for unloaded methods doesn't crash the JVM and
> > even provides useful information. This observation has led me to
> > question whether the documentation aligns with the Hotspot
> > implementation, especially given that following closely the
> > documentation appears to increase the risk associated with the use of
> > `jmethodID` values.
>
> There have been folk who wanted to make this area more user-friendly but
> that shouldn't be mistaken for moving towards a world where jmethodIDs
> are always safe to use.
>

Yes, I see your point. Unfortunately, this confirms my worries that using
AsyncGetCallTrace (ASGCT) on a system strictly adhering to the JVMTI spec
of jmethoID is not really possible without risking random and quite
frequent crashes on systems with concurrent class unloading enabled.
FTR, ASGCT will record the stack trace as a list of frames, each one
containing the corresponding jmethodID value. Considering that the most
common usage of ASGCT is in a signal handler it makes it impossible to use
JVMTI calls to resolve the holder class and create a strong reference to
prevent it from being unloaded.
And even if this would be possible we would need to figure out when to
release the class reference when it is no more needed - and it is not
really clear how we could do that reliably, leaving us with the option of
holding the class references indefinitely or risking crashing JVM.

I want to emphasize that not being able to resolve additional details for a
jmethodID pointing to a method of an unloaded class is not an issue, as
long as the JVMTI call does not crash. I think that
https://bugs.openjdk.org/browse/JDK-8268364 did address exactly the problem
of concurrent class unloading causing races in the code that is checking
for validity of jmethodID and then 

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Serguei Spitsyn]

On Thu, 1 Jun 2023 05:51:51 GMT, Alan Bateman  wrote:

>> It looks like you are right.
>> There is also and assert at line 519:
>> `519   assert(agent->is_loaded(), "invariant");`
>> 
>> So, the agent has to be loaded if we got to the line 512.
>> Also, there is a statement at line 507 (before line 512):
>> `507agent->set_os_lib(library);`
>
>> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. 
>> Isn't the library always already loaded by the time we get here (the assert 
>> below seems to imply that)? 
> 
> JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and 
> adds it to the agent list if is succeeds. The test that you are looking is 
> checking the list of already loaded agents. Maybe the function name 
> "is_loaded" is the confusion here, maybe a better name is needed.

I see the source of my confusion.
The function `invoke_Agent_OnAttach` is called from the `JvmtiAgent::load`.
But at the time of `invoke_Agent_OnAttach` the agent has not been added to the 
list yet.
It is added after the `JvmtiAgent::load` with the function 
`JvmtiAgentList::add`:

jint JvmtiAgentList::load_agent(const char* agent_name, const char* absParam,
   const char* options, outputStream* st) {
  // The abs parameter should be "true" or "false"
  const bool is_absolute_path = (absParam != nullptr) && (strcmp(absParam, 
"true") == 0);
  JvmtiAgent* const agent = new JvmtiAgent(agent_name, options, 
is_absolute_path, /* dynamic agent */ true);
  if (agent->load(st)) {
add(agent);
  . . .

Now I see the code is correct.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212665902

Re: RFR: 8308090: Add container tests for on-the-fly resource quota updates [v2]

[David Holmes]

On Tue, 23 May 2023 09:04:11 GMT, Severin Gehwolf  wrote:

>> Please review these test changes which implement automatic testing of 
>> container resource updates without JVM restart. Note that this merely tests 
>> container detection code handling this case. It doesn't do anything special 
>> for the JVM itself, though it might make sense to add some sanity checks 
>> should we detect certain limits changing. In another PR, though.
>> 
>> As to the test design, it works similar to the shared temp tests: Interact 
>> between the two containers by virtue of a shared filesystem `/tmp` and 
>> creating marker files there in order to make them cooperate. Note that the 
>> new test needs `podman` version `4.3.0` and better (`4.5` is current).
>> 
>> Testing:
>>  - [x] GHA
>>  - [x] Linux x86_64 container tests on cg v1 and cg v2 system
>>  - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and 
>> `docker`)
>
> Severin Gehwolf 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 three additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into jdk-8308090-tests-container-on-fly-updates
>  - Fix whitespace
>  - 8308090: Add container tests for on-the-fly resource quota updates

These tests didn't fail but I couldn't properly validate the output as we don't 
seem to save the process stdout file.

But I guess these are okay.

Thanks.

-

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14090#pullrequestreview-1454663420

Re: RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v4]

[Alan Bateman]

On Wed, 31 May 2023 22:13:08 GMT, Chris Plummer  wrote:

>> We can't currently test `jcmd JVMTI.agent_load` with 
>> `-XX:-EnableDynamicAgentLoading` due to JDK-8304438. But this test is for 
>> warnings, not that dynamic loading is disallowed.
>
> @AlanBateman  @sspitsyn Can this be revisited now that JDK-8304438 is being 
> worked on? Perhaps the changes to JDK-8304438 can include an update to this 
> test, although it all depends on which is being pushed first.

The tests have different concerns. The test here is checking that warnings are 
printed or can be suppressed. Tests for -EnableDynamicAgentLoading are a 
different concern. I see Serguei is expanding test coverage as part of 
JDK-8304438 so I think we are good.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212624593