Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared

[Serguei Spitsyn]

On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson  wrote:

> Hi all, 
> 
> Please review this change to avoid CleanClassLoaderDataMetaspaces safepoint 
> when there is nothing that can be cleaned up.
> 
> **Summary**
> When transforming/redefining classes a previous version list is linked 
> together in the InstanceKlass. The original class is added to this list if it 
> is still used or shared. The difference between shared and used is not 
> currently noted. This leads to a problem when doing concurrent class 
> unloading, because during that we postpone some potential work to a safepoint 
> (since we are not in one). This is the CleanClassLoaderDataMetaspaces and it 
> is triggered by the ServiceThread if there is work to be done, for example if 
> InstanceKlass::_has_previous_versions is true.
> 
> Since we currently does not differentiate between shared and "in use" we 
> always set _has_previous_versions if anything is on this list. This together 
> with the fact that shared previous versions should never be cleaned out leads 
> to this safepoint being triggered after every concurrent class unloading even 
> though there is nothing that can be cleaned out.
> 
> This can be avoided by making sure the _previous_versions list is only 
> cleaned when there are non-shared classes on it. This change renames 
> `_has_previous_versions` to `_clean_previous_versions` and only updates it if 
> we have non-shared classes on the list.
> 
> **Testing**
> * A lot of manual testing verifying that we do get the safepoint when we 
> should. 
> * Added new test to verify expected behavior by parsing the logs. The test 
> uses JFR to trigger redefinition of some shared classes (when -Xshare:on).
> * Mach5 run of new test and tier 1-3

Thank you for taking care about it.
I've posted a couple of comments but it it looks good anyway.
Thanks,
Serguei

test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineSharedClassJFR.java
 line 94:

> 92: .shouldNotContain("scratch class added; one of its 
> methods is on_stack.")
> 93: .shouldHaveExitValue(0);
> 94: return;

The fragments 61-74 and 79-93 have a big common part which can be a good base 
for a refactoring.
But it can be not worth it. So, I leave it up to you.

-

PR Review: https://git.openjdk.org/jdk/pull/13716#pullrequestreview-1408498631
PR Review Comment: https://git.openjdk.org/jdk/pull/13716#discussion_r1182156381

Re: RFR: 8306929: Avoid CleanClassLoaderDataMetaspaces safepoints when previous versions are shared

[Serguei Spitsyn]

On Fri, 28 Apr 2023 12:48:44 GMT, Stefan Johansson  wrote:

> Hi all, 
> 
> Please review this change to avoid CleanClassLoaderDataMetaspaces safepoint 
> when there is nothing that can be cleaned up.
> 
> **Summary**
> When transforming/redefining classes a previous version list is linked 
> together in the InstanceKlass. The original class is added to this list if it 
> is still used or shared. The difference between shared and used is not 
> currently noted. This leads to a problem when doing concurrent class 
> unloading, because during that we postpone some potential work to a safepoint 
> (since we are not in one). This is the CleanClassLoaderDataMetaspaces and it 
> is triggered by the ServiceThread if there is work to be done, for example if 
> InstanceKlass::_has_previous_versions is true.
> 
> Since we currently does not differentiate between shared and "in use" we 
> always set _has_previous_versions if anything is on this list. This together 
> with the fact that shared previous versions should never be cleaned out leads 
> to this safepoint being triggered after every concurrent class unloading even 
> though there is nothing that can be cleaned out.
> 
> This can be avoided by making sure the _previous_versions list is only 
> cleaned when there are non-shared classes on it. This change renames 
> `_has_previous_versions` to `_clean_previous_versions` and only updates it if 
> we have non-shared classes on the list.
> 
> **Testing**
> * A lot of manual testing verifying that we do get the safepoint when we 
> should. 
> * Added new test to verify expected behavior by parsing the logs. The test 
> uses JFR to trigger redefinition of some shared classes (when -Xshare:on).
> * Mach5 run of new test and tier 1-3

src/hotspot/share/oops/instanceKlass.hpp line 718:

> 716: 
> 717:  private:
> 718:   static bool  _clean_previous_versions;

Nit: I'd suggest to name it as `_should_clean_previous_versions`.
   Then the corresponding function needs to be named as 
`should_clean_previous_versions()`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13716#discussion_r1182136483

Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]

[Alan Bateman]

On Tue, 2 May 2023 03:17:42 GMT, Serguei Spitsyn  wrote:

>> We need to account for the `SetLocalXXX` functions with the `depth` 
>> parameter which also return `OPAQUE_FRAME` error code for virtual frames. My 
>> concern is if the "current frame" part is fully correct.
>
> I've pushed variant from Chris which is a rephrase of what Alan suggested.

I can't help thinking we can do better than "on the thread's current frame" but 
in the absence of a better suggestion then I think what you have is okay. I 
think the CSR will need to be edited to sync it up with the wording that has 
been agreed here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1182132180

Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v6]

[Serguei Spitsyn]

On Fri, 28 Apr 2023 00:50:54 GMT, Serguei Spitsyn  wrote:

>> We have two suggestions:
>>> - "or a function on a thread cannot be performed at the thread's current 
>>> frame".
>>> - "the function cannot be performed on the thread's current frame."
>> 
>> So, we need to pick one. The second one looks simpler to me but
>> I'm not completely sure that it reflects the full meaning correctly.
>> I wonder about a mix of the two suggestions above:
>> 
>>> "the function cannot be performed at the thread's current frame."
>
> We need to account for the `SetLocalXXX` functions with the `depth` parameter 
> which also return `OPAQUE_FRAME` error code for virtual frames. My concern is 
> if the "current frame" part is fully correct.

I've pushed variant from Chris which is a rephrase of what Alan suggested.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13546#discussion_r1182047387

Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v8]

[Serguei Spitsyn]

> This enhancement adds support of virtual threads to the JVMTI `StopThread` 
> function.
> In preview releases before this enhancement the StopThread returned the 
> JVMTI_ERROR_UNSUPPORTED_OPERATION error code for virtual threads.
> 
> The `StopThread` supports sending an asynchronous exception to a virtual 
> thread only if it is current or suspended at mounted state. For instance, a 
> virtual thread can be suspended at a JVMTI event. If the virtual thread is 
> not suspended and is not current then the `JVMTI_ERROR_THREAD_NOT_SUSPENDED` 
> error code is returned. If the virtual thread was suspended at unmounted 
> state then the `JVMTI_ERROR_OPAQUE_FRAME` error code is returned.
> 
> The `StopThread` has the following description for `JVMTI_ERROR_OPAQUE_FRAME` 
> error code:
>> The thread is a suspended virtual thread and the implementation 
>> was unable to throw an asynchronous exception from this frame.
> 
> A couple of the `serviceability/jvmti/vthread` tests has been updated to 
> adopt to new `StopThread` behavior.
> 
> The CSR is: https://bugs.openjdk.org/browse/JDK-8306434
> 
> Testing:
> The mach5 tears 1-6 are in progress.
> Preliminary test runs were good in general.
> The JDB test `vmTestbase/nsk/jdb/kill/kill001/kill001.java` has been 
> problem-listed and will be fixed by the corresponding debugger enhancement 
> which is going to adopt JDWP/JDI specs to new behavior of the JVMTI 
> `StopThread` related to virtual threads.
> 
> Also, two JCK JVMTI tests are failing in the tier-6 :
>> vm/jvmti/StopThread/stop001/stop00103/stop00103.html
>> vm/jvmti/StopThread/stop001/stop00103/stop00103a.html
> 
> These two tests will be excluded from the test runs by the JCK team and then 
> adjusted to new `StopThread` behavior.

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

  minor tweak of JVMTI_ERROR_OPAQUE_FRAME description

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13546/files
  - new: https://git.openjdk.org/jdk/pull/13546/files/50e615eb..0ad9a6cc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=06-07

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

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

Integrated: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions

[Serguei Spitsyn]

On Fri, 14 Apr 2023 22:01:23 GMT, Serguei Spitsyn  wrote:

> This refactoring to separate ThreadStart/ThreadEnd events posting code in the 
> JVMTI VTMS transitions is needed for future work on JVMTI scalability and 
> performance improvements. It is to easier put this code on slow path.
> 
> Testing: mach5 tiers 1-6 were successful.

This pull request has now been integrated.

Changeset: 1227a275
Author:Serguei Spitsyn 
URL:   
https://git.openjdk.org/jdk/commit/1227a275a1c1e82b9a6410843f32534d7e841f54
Stats: 335 lines in 16 files changed: 184 ins; 71 del; 80 mod

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS 
transitions
830: Reappearance of NULL in jvmtiThreadState.cpp

Reviewed-by: pchilanomate, lmesnik

-

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

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v6]

[Serguei Spitsyn]

On Tue, 2 May 2023 00:54:07 GMT, Leonid Mesnik  wrote:

> Please update copyrights, at leas in symbols-unix.

Done.

-

PR Comment: https://git.openjdk.org/jdk/pull/13484#issuecomment-1530762860

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v8]

[Serguei Spitsyn]

> This refactoring to separate ThreadStart/ThreadEnd events posting code in the 
> JVMTI VTMS transitions is needed for future work on JVMTI scalability and 
> performance improvements. It is to easier put this code on slow path.
> 
> Testing: mach5 tiers 1-6 were successful.

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

  update copyright comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13484/files
  - new: https://git.openjdk.org/jdk/pull/13484/files/02b27601..f4227c7a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13484&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13484&range=06-07

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

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

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v7]

[Serguei Spitsyn]

> This refactoring to separate ThreadStart/ThreadEnd events posting code in the 
> JVMTI VTMS transitions is needed for future work on JVMTI scalability and 
> performance improvements. It is to easier put this code on slow path.
> 
> Testing: mach5 tiers 1-6 were successful.

Serguei Spitsyn has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains 10 commits:

 - Merge
 - minor correction in sharedRuntime.cpp
 - addressed review comment: add a couple of asserts
 - Merge branch 'br29' of https://github.com/sspitsyn/jdk into br29
   merge with branch29
 - Merge branch 'master' into br29
 - move code a little bit
 - do more refactoring including VirtualThread class
 - Merge
 - 830: Reappearance of NULL in jvmtiThreadState.cpp
 - 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS 
transitions

-

Changes: https://git.openjdk.org/jdk/pull/13484/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13484&range=06
  Stats: 333 lines in 16 files changed: 184 ins; 71 del; 78 mod
  Patch: https://git.openjdk.org/jdk/pull/13484.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13484/head:pull/13484

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

Re: RFR: 8306034: add support of virtual threads to JVMTI StopThread [v7]

[Serguei Spitsyn]

> This enhancement adds support of virtual threads to the JVMTI `StopThread` 
> function.
> In preview releases before this enhancement the StopThread returned the 
> JVMTI_ERROR_UNSUPPORTED_OPERATION error code for virtual threads.
> 
> The `StopThread` supports sending an asynchronous exception to a virtual 
> thread only if it is current or suspended at mounted state. For instance, a 
> virtual thread can be suspended at a JVMTI event. If the virtual thread is 
> not suspended and is not current then the `JVMTI_ERROR_THREAD_NOT_SUSPENDED` 
> error code is returned. If the virtual thread was suspended at unmounted 
> state then the `JVMTI_ERROR_OPAQUE_FRAME` error code is returned.
> 
> The `StopThread` has the following description for `JVMTI_ERROR_OPAQUE_FRAME` 
> error code:
>> The thread is a suspended virtual thread and the implementation 
>> was unable to throw an asynchronous exception from this frame.
> 
> A couple of the `serviceability/jvmti/vthread` tests has been updated to 
> adopt to new `StopThread` behavior.
> 
> The CSR is: https://bugs.openjdk.org/browse/JDK-8306434
> 
> Testing:
> The mach5 tears 1-6 are in progress.
> Preliminary test runs were good in general.
> The JDB test `vmTestbase/nsk/jdb/kill/kill001/kill001.java` has been 
> problem-listed and will be fixed by the corresponding debugger enhancement 
> which is going to adopt JDWP/JDI specs to new behavior of the JVMTI 
> `StopThread` related to virtual threads.
> 
> Also, two JCK JVMTI tests are failing in the tier-6 :
>> vm/jvmti/StopThread/stop001/stop00103/stop00103.html
>> vm/jvmti/StopThread/stop001/stop00103/stop00103a.html
> 
> These two tests will be excluded from the test runs by the JCK team and then 
> adjusted to new `StopThread` behavior.

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

 - Merge
 - install_async_exception: set interrupt status for platform threads only
 - minor tweak in new test
 - 1. Address review comments 2. Clear interrupt bit in the TestTaskThread
 - corrections for BoundVirtualThread and test typos
 - addressed review comments on new test
 - fixed trailing spaces
 - 8306034: add support of virtual threads to JVMTI StopThread

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13546/files
  - new: https://git.openjdk.org/jdk/pull/13546/files/0113f034..50e615eb

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13546&range=05-06

  Stats: 58946 lines in 964 files changed: 40128 ins; 12285 del; 6533 mod
  Patch: https://git.openjdk.org/jdk/pull/13546.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13546/head:pull/13546

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

Withdrawn: 8297286: runtime/vthread tests crashing after JDK-8296324

[Serguei Spitsyn]

On Wed, 23 Nov 2022 00:24:28 GMT, Serguei Spitsyn  wrote:

> This problem has two sides.
> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` 
> value.
> It caused the native method `notifyJvmtiUnmountBegin()` not called after the 
> field `notifyJvmtiEvents`
> value has been set to `true` when an agent library is loaded into running VM.
> The fix is to get rid of this cashing.
> Another is that enabling `notifyJvmtiEvents` notifications needs a 
> synchronization.
> Otherwise, a VTMS transition start can be missed which will cause some 
> asserts to fire.
> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync.
> 
> Testing:
> The originally failed tests are passed now:
> 
> runtime/vthread/RedefineClass.java
> runtime/vthread/TestObjectAllocationSampleEvent.java 
> 
> In progress:
> Run the tiers 1-6 to make sure there are no regression.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v6]

[Serguei Spitsyn]

On Mon, 1 May 2023 23:42:28 GMT, Serguei Spitsyn  wrote:

>> This refactoring to separate ThreadStart/ThreadEnd events posting code in 
>> the JVMTI VTMS transitions is needed for future work on JVMTI scalability 
>> and performance improvements. It is to easier put this code on slow path.
>> 
>> Testing: mach5 tiers 1-6 were successful.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   addressed review comment: add a couple of asserts

Leonid, thank you a lot for review!

-

PR Comment: https://git.openjdk.org/jdk/pull/13484#issuecomment-1530734063

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v6]

[Leonid Mesnik]

On Mon, 1 May 2023 23:42:28 GMT, Serguei Spitsyn  wrote:

>> This refactoring to separate ThreadStart/ThreadEnd events posting code in 
>> the JVMTI VTMS transitions is needed for future work on JVMTI scalability 
>> and performance improvements. It is to easier put this code on slow path.
>> 
>> Testing: mach5 tiers 1-6 were successful.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   addressed review comment: add a couple of asserts

Please update copyrights, at leas in symbols-unix.

-

Marked as reviewed by lmesnik (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13484#pullrequestreview-1408264864

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v6]

[Serguei Spitsyn]

> This refactoring to separate ThreadStart/ThreadEnd events posting code in the 
> JVMTI VTMS transitions is needed for future work on JVMTI scalability and 
> performance improvements. It is to easier put this code on slow path.
> 
> Testing: mach5 tiers 1-6 were successful.

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

  addressed review comment: add a couple of asserts

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13484/files
  - new: https://git.openjdk.org/jdk/pull/13484/files/debe49c3..157f33af

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13484&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13484&range=04-05

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

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

Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Michał Kłeczek]

> On 1 May 2023, at 11:57, Ron Pressler  wrote:
> 

[...]

> 
> There’s no need for such code. Modules that need JNI will use JNI. The 
> application will simply give them permission to do so with 
> --enable-native-access=MODULE-NAME, as it would also do to allow FFM to use 
> native libraries.


I wonder if you are planning to define a formal grammar for all these command 
line options defining “integrity policies” as it surely looks to me like…

grant MODULE-NAME {
  AllPermission
}

grant MODULE-NAME {
  OpenModulePermission(“module-to-open-name”)
}

Wouldn’t it be better to reconsider JEP 411 and just make running under 
security manager the default?

—
Michal

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v5]

[Serguei Spitsyn]

On Mon, 1 May 2023 17:02:04 GMT, Patricio Chilano Mateo 
 wrote:

>> Serguei Spitsyn has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - Merge branch 'br29' of https://github.com/sspitsyn/jdk into br29
>>merge with branch29
>>  - move code a little bit
>
> src/hotspot/share/runtime/sharedRuntime.cpp line 639:
> 
>> 637: JRT_END
>> 638: 
>> 639: JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, 
>> jboolean dummy, JavaThread* current))
> 
> Maybe rename dummy to hide and just assert is false in this case and true for 
> the vthread_end case?

Good suggestion. Thank you.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/13484#discussion_r1181893509

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v5]

[Serguei Spitsyn]

On Thu, 27 Apr 2023 04:52:53 GMT, Serguei Spitsyn  wrote:

>> This refactoring to separate ThreadStart/ThreadEnd events posting code in 
>> the JVMTI VTMS transitions is needed for future work on JVMTI scalability 
>> and performance improvements. It is to easier put this code on slow path.
>> 
>> Testing: mach5 tiers 1-6 were successful.
>
> Serguei Spitsyn has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Merge branch 'br29' of https://github.com/sspitsyn/jdk into br29
>merge with branch29
>  - move code a little bit

Patricio, thank you a lot for review!

-

PR Comment: https://git.openjdk.org/jdk/pull/13484#issuecomment-1530272166

Re: [External] : Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Ron Pressler]

> On 1 May 2023, at 18:08, Michał Kłeczek  wrote:
> 
> 
> I wonder if you are planning to define a formal grammar for all these command 
> line options defining “integrity policies” as it surely looks to me like…
> 

We already have! With the exception of --enable-native-access=M1,M2,M3, the 
access policy is declared by modules in their module-info.java files, using a 
grammar that is now part of the Java language. Flags such as --add-opens, 
--add-exports, and --patch-module, when used *in production* (as opposed to in 
whitebox testing, where the configuration should be created automatically by 
tools), are not a policy but an emergency override of the policy that signifies 
some technical debt in the code that needs to be resolved.

— Ron

Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Gregg Wonderly]

> On May 1, 2023, at 10:56 AM, Alan Bateman  wrote:
> 
> On 01/05/2023 10:57, Ron Pressler wrote:
>> :
>>> Do you really plan to make JNI an optional feature which will have to
>>> be manually enabled at startup?
>> Not optional at all, but an important, useful feature that is restricted; 
>> JNI’s replacement, FFM will be restricted, too (in its use of native 
>> libraries). The restriction of FFM is already mentioned in JEP 442. Another 
>> JEP addressing JNI will be published soonish.
> Just to add that "Restricted methods" have been in the Java SE spec since 
> Java 19. So far it has just been the restricted methods in the FFM API but 
> it's hard to see how this would be extended to list Runtime.load/loadLibrary 
> at some point.

In many different places that I am aware of, there are still people using 
serial port connected devices.  Because Sun stopped supporting their JNI based 
access, there have been other versions of such things done.  I have packed .dll 
and .so libraries into jar files, and copied them to “temp” files and loaded 
them from there to provide OS independent jars that could provide applications 
that use serial ports, USB or otherwise connected.  If  
Runtime.load/loadLibrary are limited, a nice way to provide a description of 
the details should be part of any implementation that prompts the user to 
accept the use of JNI code.  Realistically, I don’t know how you do this and 
provide the users any guarantees of what OS functions are actually going to be 
used.  This is the number one reason for me, that I don’t understand why this 
support was removed instead of being made a platform feature.  
Runtime.load/loadLibrary are the way that the community supports the needs of 
their users when the platform doesn’t extend to such functionality.  Using it 
as a safety “gateway” feature is a steep path…

Gregg Wonderly

Integrated: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread

[Chris Plummer]

On Thu, 27 Apr 2023 17:42:41 GMT, Chris Plummer  wrote:

> Convert this ThreadReference.interrupt() test to support virtual threads. I 
> believe this is the only test for ThreadReference.interrupt() that we have.
> 
> Tested by running with and without -Dmain.wrapper=Virtual on all supported 
> platforms.

This pull request has now been integrated.

Changeset: ae5f678f
Author:Chris Plummer 
URL:   
https://git.openjdk.org/jdk/commit/ae5f678fbafcd643a5a74447ed718636a53f9e2b
Stats: 21 lines in 2 files changed: 3 ins; 2 del; 16 mod

8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread

Reviewed-by: lmesnik, sspitsyn

-

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

Re: RFR: 8282384: [LOOM] Need test for ThreadReference.interrupt() on a vthread [v4]

[Chris Plummer]

On Fri, 28 Apr 2023 17:45:59 GMT, Chris Plummer  wrote:

>> Convert this ThreadReference.interrupt() test to support virtual threads. I 
>> believe this is the only test for ThreadReference.interrupt() that we have.
>> 
>> Tested by running with and without -Dmain.wrapper=Virtual on all supported 
>> platforms.
>
> Chris Plummer has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update comment

Thanks for the reviews Serguei and Leonid!

-

PR Comment: https://git.openjdk.org/jdk/pull/13696#issuecomment-1530032710

Re: RFR: 8299414: JVMTI FollowReferences should support references from VirtualThread stack [v9]

[Alex Menkov]

> The fix updates JVMTI FollowReferences implementation to report references 
> from virtual threads:
> - unmounted vthreads are detected, their stack references for 
> JVMTI_HEAP_REFERENCE_STACK_LOCAL/JVMTI_HEAP_REFERENCE_JNI_LOCAL;
> - stacks of mounted vthreads are splitted into 2 parts (virtual thread stack 
> and carrier thread stack), references are reported with correct thread 
> id/class tag/object tags/frame depth;
> - common code to handle stack frames are moved into separate class;
> 
> Threads are reported as:
> - platform threads: JVMTI_HEAP_REFERENCE_THREAD (as before);
> - mounted vthreads (synthetic references, consider them as heap roots because 
> carrier threads are roots): JVMTI_HEAP_REFERENCE_OTHER;
> - unmounted vthreads: not reported as heap roots.

Alex Menkov has updated the pull request incrementally with one additional 
commit since the last revision:

  Added "no continuations" test case

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/13254/files
  - new: https://git.openjdk.org/jdk/pull/13254/files/d149be41..dd3be3b1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=13254&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13254&range=07-08

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

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

Re: [External] : Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Alan Bateman]

On 30/04/2023 23:24, Ron Pressler wrote:

Hi Mike!


On 30 Apr 2023, at 19:59, Mike Hearn  wrote:

we’ve begun to explore means other than the flag to allow a tool to 
load an agent at runtime


How about restricting access to the jcmd socket. For in-VM code it can
be blocked at the filesystem implementation level, and for
sub-processes by using the operating system APIs to determine if the
other side of the socket is part of the same process tree at connect
time. This would avoid the need for new UI to re-enable existing jcmd
functionality, whilst preventing code loaded into the VM from
connecting back to that same VM. Only truly external tools could
trigger agent loading, or modules that had been given permission to do
that.



Determining the process on the other side and/or maintaining the 
integrity of the process tree is not easy on all OSes.




Right, it's feasible to get the peer pid on some platforms but you can't 
rely on the process tree due to re-parenting when a parent terminates.


-Alan

Re: RFR: 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions [v5]

[Patricio Chilano Mateo]

On Thu, 27 Apr 2023 04:52:53 GMT, Serguei Spitsyn  wrote:

>> This refactoring to separate ThreadStart/ThreadEnd events posting code in 
>> the JVMTI VTMS transitions is needed for future work on JVMTI scalability 
>> and performance improvements. It is to easier put this code on slow path.
>> 
>> Testing: mach5 tiers 1-6 were successful.
>
> Serguei Spitsyn has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Merge branch 'br29' of https://github.com/sspitsyn/jdk into br29
>merge with branch29
>  - move code a little bit

Hi Serguei,

Changes look good to me. Thanks for taking care of the refactoring.

Patricio

src/hotspot/share/runtime/sharedRuntime.cpp line 639:

> 637: JRT_END
> 638: 
> 639: JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, 
> jboolean dummy, JavaThread* current))

Maybe rename dummy to hide and just assert is false in this case and true for 
the vthread_end case?

-

Marked as reviewed by pchilanomate (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13484#pullrequestreview-1407836173
PR Review Comment: https://git.openjdk.org/jdk/pull/13484#discussion_r1181722432

Re: [External] : Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Eirik Bjørsnøs]

>
> Keep in mind two things:
>
> 1. Dynamically loaded agents are more limited in their capabilities than
> agents loaded at startup because redefinition/retransformation is limited
> to changing the body of existing methods. Redefinition can only fix issues
> if you’re lucky.
>
> 2. Java offers no general mechanism to make patches applied through
> redefinition persistent. They are reverted at the next startup.
>

Ron,

My concern was more the observation that the Summary of the draft JEP can
be misunderstood, (It seems to have happened in this thread):

Disallow the dynamic loading of agents into a running JVM by default.
> Agents are used by profiling tools to instrument Java applications, but
> agents can also be misused to undermine the integrity of the Java Platform.


I think the second sentence here ("agents can also be misused..") was meant
to refer specifically to dynamically loaded agents, not agents in general.
To help avoid confusion, this sentence could be updated to clarify that
it's the dynamic loading that is misused, not the agent mechanism per se.

Something like: "but *dynamically loaded agents* can also be misused to.."

I know the leading sentence defines the context, but I think a bit of
redunancy here would help reduce misunderstandings and prevent people from
thinking "now they're taking away agents too".

Thanks,
Eirik.

Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Alex Buckley]

On 5/1/2023 8:56 AM, Alan Bateman wrote:
Just to add that "Restricted methods" have been in the Java SE spec 
since Java 19. So far it has just been the restricted methods in the FFM 
API but it's hard to see how this would be extended to list 
Runtime.load/loadLibrary at some point.


Clarification: ... but it's NOT hard to see ...

FYI the section about restricted methods in the Java SE Platform Spec:

- 
https://cr.openjdk.org/~iris/se/19/spec/latest/index.html#Restricted-methods
- 
https://cr.openjdk.org/~iris/se/20/spec/latest/index.html#Restricted-methods


Alex

Re: [External] : Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Dan Heidinga]

On Sun, Apr 30, 2023 at 10:19 AM Ron Pressler 
wrote:

> Hi Dan!
>
> > On 29 Apr 2023, at 03:30, Dan Heidinga  wrote:
> >
> > Hi Ron,
> >
> > Thanks for writing up the JEP draft outlining the proposal to disallow
> dynamic loading of agents by default.  The Red Hat Java team has continued
> to discuss this proposal internally and with our stakeholders.
> >
> > While there is a general agreement (or at least acceptance) with the
> overall direction of this proposal, we still see the concerns raised by
> Andrew [0] as needing to be addressed.
> >
> > So let’s start with the high-order bit: timing.
> >
> > The JEP draft states it “intends to adopt the proposal [to disable
> agents by default] six years later, in 2023, for JDK 21.”  We would like to
> see this targeted to JDK 22 instead as the change has not been widely
> evangelized yet and comes as a surprise to many, both to those actively
> developing OpenJDK and to those monitoring its development.
> >
> > We owe it to the community to give this proposal enough bake time,
> especially given that JDK 21 is an LTS release.  Though the official
> position is that LTS releases are no different than any other release, the
> actions of JDK consumers make them special.  Users will be surprised by
> this change when they migrate from JDK 17 to 21.  If we delay till JDK 22,
> we give the ecosystem time to adapt to the change before the next LTS.
> >
> > Additionally, users who have tested with the
> -XX:-EnableDynamicAgentLoading option will have false expectations if they
> validated their use of jcmd to load the agent as the behaviour was not
> correct prior to JDK 21 [1].
> >
> > The next concern is one you call out in the draft that “Java's excellent
> serviceability has long been a source of pride for the platform.”  We
> agree!
> >
> > Java currently has an excellent, prime position in Observability
> capabilities. For better or for worse, there are many Observability tools
> that have relied on dynamic attach to provide the necessary monitoring for
> workloads
> >
> > It’s important we give Java’s monitoring tools sufficient time to
> migrate comfortably without shocking the ecosystem by changing the default
> in an LTS.  By delaying the change till JDK 22, we give the ecosystem 2
> years to migrate and to prepare users for the change.
> >
> > Additionally, this provides the time for Java’s profiling tools to adapt
> as well.  And for the “ad-hoc troubleshooting” tools - like Byteman - to
> retrain their users.
>
> That’s a fair point. Even though the change was announced some years ago,
> some strong encapsulation features had a transition period where they
> emitted warnings before changing defaults. Since we can reasonably expect
> 21 to see relatively high adoption, we could take that opportunity to
> educate more users and only emit a warning when an agent is loaded
> dynamically (otherwise, since many users unfortunately skip versions, they
> would be equally surprised and unprepared at the next version they adopt as
> they would be if the default change were made in 21). Would you find that
> reasonable?
>

This "print a warning" approach makes a lot of sense - as you say, it
educates users of dynamic agents that action will be required while not
impeding the uptake of JDK 21.  It also follows the precedent set by the
--illegal-access option in JDK 9+.  Users who don't want to see the warning
in their logs can specify -XX:+EnableDynamicAgentLoading and are then well
prepared for JDK 22+.  Seems like a win-win approach.


>
> If so, we may perhaps be able to also emit warnings on JNI use in 21, thus
> bringing agents, JNI, and FFM to the same baseline in 21, i.e. they would
> all issue warnings unless sanctioned by the application.
>

I'm still working through the integrity JEP so I'll hold off on responding
regarding JNI for now.


>
> >
> > Finally, while it’s easy to agree with the principle that “the
> application owner is given the final say on whether to allow dynamic
> loading of agents”, the argument can (and should!) be made that those
> application owners have made that final decision by deploying the libraries
> and tools that use dynamic attach.  A JVM command line argument is not the
> only way to express their consent for code to run on their system.
> >
> > For many production uses, the reality is more complicated than a single
> “application owner”. Take a Kubernetes cluster for example.
> >
> > Who is the application owner when deploying to a Kubernetes cluster?
> The dev team that develops the application?  The operations team that
> manages the cluster?  The monitoring team that monitors the applications?
> The Support team that troubleshoots issues with the deployment?
> >
> > We should be careful not to understate or misrepresent the complex web
> of “owners” that are currently able (and, for business reasons, need) to
> apply agents dynamically.  Downplaying the complexity many organizations
> experience when dealing with changes

Re: RFR: 8291555: Implement alternative fast-locking scheme [v62]

[Coleen Phillimore]

On Wed, 26 Apr 2023 16:07:33 GMT, Daniel D. Daugherty  
wrote:

>> Roman Kennke has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Remove unnecessary comments
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java line 
> 231:
> 
>> 229: 
>> 230: public JavaThread owningThreadFromMonitor(ObjectMonitor monitor) {
>> 231: if (VM.getVM().getCommandLineFlag("LockingMode").getInt() == 2) 
>> {
> 
> Please put a comment after that literal '2':
> 
> if (VM.getVM().getCommandLineFlag("LockingMode").getInt() == 2 /* 
> LM_LIGHTWEIGHT */) {

You could add the LM_LEGACY, LM_LIGHTWEIGHT literals to vmStructs.cpp and 
compare with them.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1181662882

Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Alan Bateman]

On 01/05/2023 10:57, Ron Pressler wrote:

:

Do you really plan to make JNI an optional feature which will have to
be manually enabled at startup?

Not optional at all, but an important, useful feature that is restricted; JNI’s 
replacement, FFM will be restricted, too (in its use of native libraries). The 
restriction of FFM is already mentioned in JEP 442. Another JEP addressing JNI 
will be published soonish.
Just to add that "Restricted methods" have been in the Java SE spec 
since Java 19. So far it has just been the restricted methods in the FFM 
API but it's hard to see how this would be extended to list 
Runtime.load/loadLibrary at some point.


-Alan

Re: RFR: 8291555: Implement alternative fast-locking scheme [v56]

[Coleen Phillimore]

On Wed, 12 Apr 2023 05:26:23 GMT, Stefan Karlsson  wrote:

>> The old code is "racy but safe - it basically answers the question "what 
>> thread held the lock at the time I was asking?" and if we get a stack-addr 
>> as the owner at the time we ask, and that stack-address belongs to a given 
>> thread t then we report t as the owner. The fact t may have released the 
>> lock as soon as we read the stack-addr is immaterial.
>> 
>> The new code may be a different matter however. Now the race involves oops, 
>> and potentially stale ones IIUC what Stefan is saying. So now the race is 
>> not safe, and potentially may crash.
>
>> That seems fine to me, as long as we don't crash. But my understanding is 
>> that Generational ZGC will crash if it sees a stale oop. Isn't it possible 
>> that the racing read sees junk that looks to Generational ZGC like a stale 
>> oop? To avoid this, unused slots may need to be set to nullptr even in 
>> product builds. But I'm not a GC expert so maybe there's no problem.
> 
> Generational ZGC has verification code in fastdebug builds that try to detect 
> stale oops. However, the current LockStack implementation seems to always 
> clear unused slots when running in debug builds. That minimizes the risk that 
> the verification code would find stale oops in the LockStack.
> 
> Regarding release build, given that the LockStack code doesn't dereference 
> any of the contained oops and we don't have oop verification code in release 
> builds, I don't see of ZGC would crash because of this race. Note however 
> that these kind of races are technically undefined behavior, so I wouldn't be 
> too confident that this code is safe.

Can you add a comment and file a CR describing this issue?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/10907#discussion_r1181630364

Re: RFR: 8291555: Implement alternative fast-locking scheme [v67]

[Coleen Phillimore]

On Fri, 28 Apr 2023 19:23:24 GMT, Roman Kennke  wrote:

>> This change adds a fast-locking scheme as an alternative to the current 
>> stack-locking implementation. It retains the advantages of stack-locking 
>> (namely fast locking in uncontended code-paths), while avoiding the overload 
>> of the mark word. That overloading causes massive problems with Lilliput, 
>> because it means we have to check and deal with this situation when trying 
>> to access the mark-word. And because of the very racy nature, this turns out 
>> to be very complex and would involve a variant of the inflation protocol to 
>> ensure that the object header is stable. (The current implementation of 
>> setting/fetching the i-hash provides a glimpse into the complexity).
>> 
>> What the original stack-locking does is basically to push a stack-lock onto 
>> the stack which consists only of the displaced header, and CAS a pointer to 
>> this stack location into the object header (the lowest two header bits being 
>> 00 indicate 'stack-locked'). The pointer into the stack can then be used to 
>> identify which thread currently owns the lock.
>> 
>> This change basically reverses stack-locking: It still CASes the lowest two 
>> header bits to 00 to indicate 'fast-locked' but does *not* overload the 
>> upper bits with a stack-pointer. Instead, it pushes the object-reference to 
>> a thread-local lock-stack. This is a new structure which is basically a 
>> small array of oops that is associated with each thread. Experience shows 
>> that this array typcially remains very small (3-5 elements). Using this lock 
>> stack, it is possible to query which threads own which locks. Most 
>> importantly, the most common question 'does the current thread own me?' is 
>> very quickly answered by doing a quick scan of the array. More complex 
>> queries like 'which thread owns X?' are not performed in very 
>> performance-critical paths (usually in code like JVMTI or deadlock 
>> detection) where it is ok to do more complex operations (and we already do). 
>> The lock-stack is also a new set of GC roots, and would be scanned during 
>> thread scanning, possibly concurrently, via the normal 
 protocols.
>> 
>> The lock-stack is fixed size, currently with 8 elements. According to my 
>> experiments with various workloads, this covers the vast majority of 
>> workloads (in-fact, most workloads seem to never exceed 5 active locks per 
>> thread at a time). We check for overflow in the fast-paths and when the 
>> lock-stack is full, we take the slow-path, which would inflate the lock to a 
>> monitor. That case should be very rare.
>> 
>> In contrast to stack-locking, fast-locking does *not* support recursive 
>> locking (yet). When that happens, the fast-lock gets inflated to a full 
>> monitor. It is not clear if it is worth to add support for recursive 
>> fast-locking.
>> 
>> One trouble is that when a contending thread arrives at a fast-locked 
>> object, it must inflate the fast-lock to a full monitor. Normally, we need 
>> to know the current owning thread, and record that in the monitor, so that 
>> the contending thread can wait for the current owner to properly exit the 
>> monitor. However, fast-locking doesn't have this information. What we do 
>> instead is to record a special marker ANONYMOUS_OWNER. When the thread that 
>> currently holds the lock arrives at monitorexit, and observes 
>> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, 
>> and then properly exits the monitor, and thus handing over to the contending 
>> thread.
>> 
>> As an alternative, I considered to remove stack-locking altogether, and only 
>> use heavy monitors. In most workloads this did not show measurable 
>> regressions. However, in a few workloads, I have observed severe 
>> regressions. All of them have been using old synchronized Java collections 
>> (Vector, Stack), StringBuffer or similar code. The combination of two 
>> conditions leads to regressions without stack- or fast-locking: 1. The 
>> workload synchronizes on uncontended locks (e.g. single-threaded use of 
>> Vector or StringBuffer) and 2. The workload churns such locks. IOW, 
>> uncontended use of Vector, StringBuffer, etc as such is ok, but creating 
>> lots of such single-use, single-threaded-locked objects leads to massive 
>> ObjectMonitor churn, which can lead to a significant performance impact. But 
>> alas, such code exists, and we probably don't want to punish it if we can 
>> avoid it.
>> 
>> This change enables to simplify (and speed-up!) a lot of code:
>> 
>> - The inflation protocol is no longer necessary: we can directly CAS the 
>> (tagged) ObjectMonitor pointer to the object header.
>> - Accessing the hashcode could now be done in the fastpath always, if the 
>> hashcode has been installed. Fast-locked headers can be used directly, for 
>> monitor-locked objects we can easily reach-through to the displaced header. 
>> This is safe because Java threads partic

Integrated: 8304915: Create jdk.internal.util.Architecture enum and apply

[Roger Riggs]

On Wed, 5 Apr 2023 15:58:08 GMT, Roger Riggs  wrote:

> Define an internal jdk.internal.util.Architecture enumeration and static 
> methods to replace uses of the system property `os.arch`.
> The enumeration values are defined to match those used in the build.
> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64`
> Note that `amd64` and `x86_64` in the build are represented by `X64`.
> The value of the system property `os.arch` is unchanged.
> 
> The API is similar to the jdk.internal.util.OperatingSystem enum created by 
> #[12931](https://git.openjdk.org/jdk/pull/12931).
> Uses in `java.base` and a few others are included but other modules will be 
> done in separate PRs.

This pull request has now been integrated.

Changeset: f00a748b
Author:Roger Riggs 
URL:   
https://git.openjdk.org/jdk/commit/f00a748bc5b708d4f8f277d075859b058f9d575c
Stats: 411 lines in 7 files changed: 343 ins; 57 del; 11 mod

8304915: Create jdk.internal.util.Architecture enum and apply

Reviewed-by: erikj, mdoerr, amitkumar

-

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

Re: RFR: 8307067: remove broken EnableThreadSMRExtraValidityChecks option

[Coleen Phillimore]

On Thu, 27 Apr 2023 22:17:30 GMT, Daniel D. Daugherty  
wrote:

> A trivial fix to remove broken EnableThreadSMRExtraValidityChecks option.

Yes, this looks good and also trivial.

-

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13704#pullrequestreview-1407611225

Integrated: 8306851: Move Method access flags

[Coleen Phillimore]

On Tue, 25 Apr 2023 19:09:23 GMT, Coleen Phillimore  wrote:

> This change moves the flags from AccessFlags to either ConstMethodFlags or 
> MethodFlags, depending on whether they are set at class file parse time, 
> which makes them essentially const, or at runtime, which makes them needing 
> atomic access.
> 
> This leaves AccessFlags int size because Klass still has JVM flags that are 
> more work to move, but this change doesn't increase Method size.  I didn't 
> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are 
> several of these in other places, and with this change the code is benign.
> 
> Tested with tier1-6, and some manual verification of printing.

This pull request has now been integrated.

Changeset: 316d303c
Author:Coleen Phillimore 
URL:   
https://git.openjdk.org/jdk/commit/316d303c1da550c9589c9be56b65650964e3886b
Stats: 781 lines in 27 files changed: 316 ins; 297 del; 168 mod

8306851: Move Method access flags

Reviewed-by: cjplummer, dholmes, dnsimon, matsaave, fparain

-

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

Re: RFR: 8306851: Move Method access flags [v5]

[Coleen Phillimore]

On Fri, 28 Apr 2023 19:59:53 GMT, Coleen Phillimore  wrote:

>> This change moves the flags from AccessFlags to either ConstMethodFlags or 
>> MethodFlags, depending on whether they are set at class file parse time, 
>> which makes them essentially const, or at runtime, which makes them needing 
>> atomic access.
>> 
>> This leaves AccessFlags int size because Klass still has JVM flags that are 
>> more work to move, but this change doesn't increase Method size.  I didn't 
>> remove JVM_RECOGNIZED_METHOD_MODIFIERS with this change since there are 
>> several of these in other places, and with this change the code is benign.
>> 
>> Tested with tier1-6, and some manual verification of printing.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix constMethod printing.

Thanks David, Chris, Doug, Matias and Fred for reviewing.

-

PR Comment: https://git.openjdk.org/jdk/pull/13654#issuecomment-1529606901

Re: [External] : Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Ron Pressler]

On 28 Apr 2023, at 20:14, Eirik Bjørsnøs 
mailto:eir...@gmail.com>> wrote:

>Agents are used by profiling tools to instrument Java applications,
>but agents can also be misused to undermine the integrity of the
>Java Platform.

I don't think it is fair to assume that profilers are the only "valid"
use case for agents and imply that all other use cases are a mis-use
of the API.

First, I don't read the JEP as implying that all non-profiler use cases are 
misuse.

Having said that, I do think that agents can in fact strengthen the integrity 
of the platform. Case in point is that when the Java serialization 
vulnerabilities hit around 2015, I could very quickly ( a few hours) whip 
together the "NotSoSerial" serialization firewall agent [1] to efficiently 
prevent exploits. I later got word that a large CMS vendor deployed it to their 
platform which included some of the world's busiest websites. I don't know if 
they used the attach mechanism to plug their serialization holes, but they 
surely could at the time.

With microservices gaining popularity over the years, restarts are probably 
more common and automated now, including configuration of JVM options. So 
attaching to long-running instances to prevent restarts is probably becoming 
less useful over time.

The agent misuse that the JEP is referring to here is perhaps mostly concerning 
libraries using the attach mechanism to get access they otherwise would not 
have in a running JVM? Perhaps the JEP could be updated to be more clear on 
this?

Cheers,
Eirik.

[1] 
https://github.com/kantega/notsoserial/



Keep in mind two things:

1. Dynamically loaded agents are more limited in their capabilities than agents 
loaded at startup because redefinition/retransformation is limited to changing 
the body of existing methods. Redefinition can only fix issues if you’re lucky.

2. Java offers no general mechanism to make patches applied through 
redefinition persistent. They are reverted at the next startup.

Due to these two facts, patching code in production to change its logic (as 
opposed to benign instrumentation with profiling events) has never been a 
sanctioned usage of dynamic agents. It’s simply not a generally-effective 
mechanism for that. Tools that offer less restricted dynamic patching (e.g. 
JRebel) require an agent *loaded at startup*.

— Ron

Re: JEP draft: Disallow the Dynamic Loading of Agents by Default

[Ron Pressler]

Hi Volker!

> On 28 Apr 2023, at 16:38, Volker Simonis  wrote:
> 
> I think it is a little unfortunate to put the usage of s.m.Unsafe and
> JNI/Instrumentation/JVMTI into the same category, especially when it
> comes to blaming developers for their usage. While s.m.Unsafe has
> always been an internal, undocumented and unsupported API, the latter
> three are part of the Java Platform (e.g. "native" is a Java keyword
> and Runtime.loadLibrary() is part of the Java API).

To have integrity by default, theses must all become restricted. In fact — not 
just them. Even the fully-official and brand-new FFM API, that a lot of 
investment has gone into very recently, must also be restricted. 

That these features must be restricted doesn’t mean they’re wrong or bad. It 
just means that they're superpowers, and so the user must acknowledge the 
choice to use them over the loss of integrity. Native libraries are good and 
integrity is good, but because they’re in contradiction, there must be a switch 
expressing the user’s preference like other switches offered by the runtime to 
select between alternatives.

Unsafe, on the other hand, may become more than just restricted over time. It 
may gradually be emptied out until it’s gone.

> 
> Do you really plan to make JNI an optional feature which will have to
> be manually enabled at startup?

Not optional at all, but an important, useful feature that is restricted; JNI’s 
replacement, FFM will be restricted, too (in its use of native libraries). The 
restriction of FFM is already mentioned in JEP 442. Another JEP addressing JNI 
will be published soonish.

> What will be the benefit?

Integrity. The ability to to bypass encapsulation when needed is not being 
taken away, but we need the new ability to establish and enforce invariants. We 
don’t yet have it.

> I understand
> that in an ideal world where you had no user-supplied JNI libraries at
> all, you might be able to perform more/better optimizations. But as
> you'd have to support JNI anyway, wouldn't the maintenance of the
> resulting code become a nightmare. How many "if (JNI) {..} else {..}"
> would we get?

There’s no need for such code. Modules that need JNI will use JNI. The 
application will simply give them permission to do so with 
--enable-native-access=MODULE-NAME, as it would also do to allow FFM to use 
native libraries.

> And what would be the benefit of disabling it by default
> for the user except increased "integrity"?

Not disabled, restricted, and integrity is the benefit for the user, e.g. in 
the form of programs not breaking (or breaking less) when upgrading the JDK. 
Integrity is required for the platform to continue evolve while keeping the 
ecosystem sustainable.

> I.e. do you have some
> concrete examples of planned features X, Y, Z which will only work
> with disabled JNI?

Not disabled, restricted. Like all encapsulation-breaking restricted 
superpowers, allowing them might have implications on possible Leyden features. 
For example, if a private method could be accessed from outside a module — 
whether through deep reflection or JNI — private methods could not be removed 
at link time.

> Will these features be Java SE features or
> implementation specific OpenJDK-only features?

As with all integrity and strong encapsulation features, all limitations will 
be part of the platform spec.

We realise that in each individual case there might be good reasons to allow 
knocking down encapsulation barriers. But whereas every application and library 
author rightfully want minimise the burden on their particular code, such 
individual decisions inevitably lead to a tragedy of the commons (as they 
already have). We must strive to minimise the overall burden integrated over 
the entire ecosystem *as a whole*. So the platform will have the right defaults 
for the ecosystem, and every application would be able to relax encapsulation 
to suit its particular needs.

Most Java program don’t use native libraries, agents (startup or dynamic), or 
deep reflection. Many do, and these features are very powerful and can be very 
useful, but with great power comes great responsibility, and that 
responsibility falls on the *application*. Libraries must not silently impose 
that responsibility on the application in a way that makes it infeasible to 
exercise.

Moreover, most encapsulation boundaries are never bypassed, but without 
integrity by default, the platform and its users still can’t be certain that 
code means what it says as long as any fourth-level dependency can decide on 
its own that any line of code in the program might mean something else.

> 
> I don't think it is fair to assume that profilers are the only "valid"
> use case for agents and imply that all other use cases are a mis-use
> of the API.

We are not assuming that at all. Only the use of *dynamically loaded* agents 
*by libraries* is misuse. Dynamically loaded agents were specifically designed 
to support serviceability tools,