Integrated: 8332327: Return _methods_jmethod_ids field back in VMStructs

2024-05-17 Thread Andrei Pangin
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin  wrote:

> The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has 
> [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd
> ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs.
> 
> This broke 
> [async-profiler](https://github.com/async-profiler/async-profiler/), since 
> the profiler needs this field to obtain jmethodID in some corner cases.
> 
> There was no actual need for removal, as the field is still there in 
> InstanceKlass. So, I propose to return the field back to restore the broken 
> functionality of async-profiler. This is a no risk change, because it only 
> exports an offset of one field and does not affect the JVM otherwise.

This pull request has now been integrated.

Changeset: d84a8fd8
Author:Andrei Pangin 
Committer: Aleksey Shipilev 
URL:   
https://git.openjdk.org/jdk/commit/d84a8fd8762fe9448e73d75ec9dc8c4876b1a709
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod

8332327: Return _methods_jmethod_ids field back in VMStructs

Reviewed-by: cjplummer, sspitsyn, coleenp, shade

-

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


Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs

2024-05-15 Thread Andrei Pangin
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin  wrote:

> The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has 
> [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd
> ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs.
> 
> This broke 
> [async-profiler](https://github.com/async-profiler/async-profiler/), since 
> the profiler needs this field to obtain jmethodID in some corner cases.
> 
> There was no actual need for removal, as the field is still there in 
> InstanceKlass. So, I propose to return the field back to restore the broken 
> functionality of async-profiler. This is a no risk change, because it only 
> exports an offset of one field and does not affect the JVM otherwise.

@coleenp Could you please review the PR since it reverts one line from one of 
your previous commits. Thanks.

-

PR Comment: https://git.openjdk.org/jdk/pull/19254#issuecomment-2113479138


Integrated: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling

2024-04-01 Thread Andrei Pangin
On Wed, 27 Mar 2024 01:02:41 GMT, Andrei Pangin  wrote:

> This fix makes `AsyncGetCallTrace` reentrant and async-signal-safe.
> Reentrancy is required in the cases when two or more profiling engines are 
> running at the same time, e.g., when CPU and Wall clock profilers work 
> together and therefore one profiler may interrupt another in the middle of 
> getting a stack trace.
> 
> Tested with async-profiler:
> 
> java 
> -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,interval=1ms,wall=1ms,file=profile.jfr

This pull request has now been integrated.

Changeset: 6b1b0e9d
Author:Andrei Pangin 
Committer: Serguei Spitsyn 
URL:   
https://git.openjdk.org/jdk/commit/6b1b0e9d45eb56f88398e2a6bca0d90c03112eaa
Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod

8329103: assert(!thread->in_asgct()) failed during multi-mode profiling

Reviewed-by: dholmes, sspitsyn

-

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


Re: RFR: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling [v2]

2024-03-29 Thread Andrei Pangin
On Fri, 29 Mar 2024 05:55:01 GMT, Serguei Spitsyn  wrote:

>> Andrei Pangin has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Rephrased comment about AsyncGetCallTrace reentrancy
>
> src/hotspot/share/runtime/thread.hpp line 664:
> 
>> 662:   ThreadInAsgct(Thread* thread) : _thread(thread) {
>> 663: assert(thread != nullptr, "invariant");
>> 664: // AsyncGetCallTrace is reentrant - save the previous state.
> 
> Nit: It is possible to rephrase this comment as follows:
> 
>   // Allow AsyncGetCallTrace to be reentrant - save the previous state.
> ``

Thank you for the review - I updated the comment.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18504#discussion_r1544400438


Re: RFR: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling [v2]

2024-03-29 Thread Andrei Pangin
> This fix makes `AsyncGetCallTrace` reentrant and async-signal-safe.
> Reentrancy is required in the cases when two or more profiling engines are 
> running at the same time, e.g., when CPU and Wall clock profilers work 
> together and therefore one profiler may interrupt another in the middle of 
> getting a stack trace.
> 
> Tested with async-profiler:
> 
> java 
> -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,interval=1ms,wall=1ms,file=profile.jfr

Andrei Pangin has updated the pull request incrementally with one additional 
commit since the last revision:

  Rephrased comment about AsyncGetCallTrace reentrancy

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18504/files
  - new: https://git.openjdk.org/jdk/pull/18504/files/af9a57fd..fc5a4831

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=18504&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18504&range=00-01

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

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


Re: RFR: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling

2024-03-28 Thread Andrei Pangin
On Thu, 28 Mar 2024 13:09:10 GMT, David Holmes  wrote:

>> This fix makes `AsyncGetCallTrace` reentrant and async-signal-safe.
>> Reentrancy is required in the cases when two or more profiling engines are 
>> running at the same time, e.g., when CPU and Wall clock profilers work 
>> together and therefore one profiler may interrupt another in the middle of 
>> getting a stack trace.
>> 
>> Tested with async-profiler:
>> 
>> java 
>> -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,interval=1ms,wall=1ms,file=profile.jfr
>
> I have my doubts as to whether AGCT is actually re-entrant in a general 
> sense, but I can see that the `ThreadInAsgct` RAII object introduced a 
> reentrancy constraint that did not exist prior, and so removing it should not 
> make AGCT any less safe and should allow previous reentrancy cases to 
> continue to work as before.

@dholmes-ora Thank you for the review. When looking at the AGCT code, nothing 
suspicious that could affect reentrancy caught my eye. Also, benchmarks 
(specJVM, Renaissance) now run fine with two profilers enabled.
So yes, while I don't have 100% proof that AGCT is async-signal-safe, the fix 
definitely improves the situation.

-

PR Comment: https://git.openjdk.org/jdk/pull/18504#issuecomment-2026255860


Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v19]

2023-12-27 Thread Andrei Pangin
On Fri, 22 Dec 2023 09:33:06 GMT, Dmitry Chuyko  wrote:

>> 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 (so bypass inlined 
>> methods).
>> 
>> Natural way to eliminate the discrepancy between the result of compilation 
>> and the broken rule is to discard the compilation result, i.e. 
>> deoptimization. Prior to that we can try to re-compile the method letting 
>> compile broker to perform it taking new directives stack into account. 
>> Re-compilation helps to prevent hot methods from execution in the 
>> interpreter.
>> 
>> A new flag `-r` 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 
>> puts methods that have any active non-default matching compiler directives 
>> to re-compilation if possible, otherwise marks them for deoptimization. 
>> 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 
>> refreshed. On the other hand, if there are rules for a method and it was 
>> inlined, top-level methods won't be refreshed, but this can be achieved by 
>> having rules for them.
>> 
>> In addition, a new diagnostic command `Compiler.replace_directives...
>
> Dmitry Chuyko has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Deopt osr, cleanups

The logic looks good to me now, thanks.

-

Marked as reviewed by apangin (no project role).

PR Review: https://git.openjdk.org/jdk/pull/14111#pullrequestreview-1797576900


Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v15]

2023-12-19 Thread Andrei Pangin
On Thu, 14 Dec 2023 15:29:06 GMT, Dmitry Chuyko  wrote:

>> 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 (so bypass inlined 
>> methods).
>> 
>> Natural way to eliminate the discrepancy between the result of compilation 
>> and the broken rule is to discard the compilation result, i.e. 
>> deoptimization. Prior to that we can try to re-compile the method letting 
>> compile broker to perform it taking new directives stack into account. 
>> Re-compilation helps to prevent hot methods from execution in the 
>> interpreter.
>> 
>> A new flag `-r` 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 
>> puts methods that have any active non-default matching compiler directives 
>> to re-compilation if possible, otherwise marks them for deoptimization. 
>> 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 
>> refreshed. On the other hand, if there are rules for a method and it was 
>> inlined, top-level methods won't be refreshed, but this can be achieved by 
>> having rules for them.
>> 
>> In addition, a new diagnostic command `Compiler.replace_directives...
>
> Dmitry Chuyko has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 33 commits:
> 
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - ... and 23 more: https://git.openjdk.org/jdk/compare/fde5b168...44d680cd

src/hotspot/share/code/codeCache.cpp line 1409:

> 1407:   while(iter.next()) {
> 1408: CompiledMethod* nm = iter.method();
> 1409: methodHandle mh(thread, nm->method());

If there are two CompiledMethods for the same Java method, will it be scheduled 
for recompilation twice? Related question: if `nm` is an OSR method, does it 
make sense to go directly for deoptimization rather than compiling a non-OSR 
version?

src/hotspot/share/code/codeCache.cpp line 1413:

> 1411:   ResourceMark rm;
> 1412:   // Try the max level and let the directives be applied during the 
> compilation.
> 1413:   int complevel = CompLevel::CompLevel_full_optimization;

Should the highest level depend on the configuration instead of the hard-coded 
constant? Perhaps, needs to be `highest_compile_level()`

src/hotspot/share/compiler/compilerDirectives.cpp line 750:

> 748:   if (!dir->is_default_directive() && dir->match(method)) {
> 749: match_found = true;
> 750: break;

`match_found` is redundant: for better readability, you may just return true. 
Curly braces around MutexLocker won't be needed either.

src/hotspot/share/oops/method.hpp line 820:

> 818:   // Clear the flags related to compiler directives that were set by the 
> compilerBroker,
> 819:   // because the directives can be updated.
> 820:   void c

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

2023-06-12 Thread Andrei Pangin
On Wed, 24 May 2023 00:38:27 GMT, Dmitry Chuyko  wrote:

> 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 combinatio...

src/hotspot/share/code/codeCache.cpp line 1379:

> 1377:   while(iter.next()) {
> 1378: CompiledMethod* nm = iter.method();
> 1379: HandleMark hm(thread);

Isn't it better to move `HandleMark` outside the loop?

src/hotspot/share/runtime/mutexLocker.cpp line 274:

> 272:   MUTEX_DEFN(MethodCompileQueue_lock , PaddedMonitor, safepoint);
> 273:   MUTEX_DEFN(CompileStatistics_lock  , PaddedMutex  , safepoint);
> 274:   MUTEX_DEFN(DirectivesStack_lock, PaddedMutex  , 
> nosafepoint-3);

A comment explaining the rank change would be helpful.

src/hotspot/share/services/diagnosticCommand.cpp line 890:

> 888:DCmdWithParser(output, heap),
> 889:   _filename("filename", "Name of the directives file", "STRING", true),
> 890:   _force_deopt("-d", "Force deoptimization of affected methods.", 
> "BOOLEAN", false, "false") {

I agree with Paul a generic alternative like `refresh` would be better.

src/hotspot/share/services/diagnosticCommand.cpp line 946:

> 944: DeoptimizationScope deopt_scope;
> 945: CodeCache::mark_for_deoptimization_directives_matches(&deopt_scope);
> 946: DirectivesStack::pop(1);

Why deoptimizing methods from the whole stack if we change only the topmost 
list?

src/hotspot/share/services/diagnosticCommand.hpp line 734:

> 732: };
> 733: 
> 734: class CompilerDirectivesReplaceDCmd : public DCmdWithParser {

Introducing a new command probably isn't worth it, given the same functionality 
can be achieved with existing commands. Furthermore, it's not obvious whether 
"replace" should mean remove+add or clear+add.

src/hotspot/share/services/diagnosticCommand.hpp line 745:

> 743:   }
> 744:   static const char* description() {
> 745: return "Clear derectives stack amd load new compiler directives from 
> file.";

Spelling: Clear *directives* stack *and* ...

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226618742
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226553192
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226624671
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226627598
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226604676
PR Review Comment: https://gi

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

2023-05-26 Thread Andrei Pangin
On Tue, 23 May 2023 15:32:58 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.
>
> 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 12 additional 
> commits since the last revision:
> 
>  - Tweak javadoc, update test to use more test infra
>  - Merge
>  - Merge
>  - Refresh package description
>  - Merge
>  - Tweak docs
>  - Merge
>  - Draft docs changes
>  - Merge
>  - Rename/cleanup
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/23703312...87022831

When testing the proposed patch, I stumbled upon two (related) issues:

1. There are no checks that an agent have been already loaded. When I invoke 
`agent_load` multiple times with exactly the same library path, the warning is 
printed every time. The repeated warnings are not helpful; they are neither 
good for user experience nor for integrity, since agent developers will be 
tempted to suppress excessive warnings on the first load by hacking the JDK 
internals and thus breaking integrity even more.
2. If an agent is loaded at JVM startup using `-agentlib/agentpath` option, the 
subsequent communication with the agent through jcmd results in the warning 
being printed again and again. This contradicts the intentions described in JEP 
451:

> Tools that employ agents loaded at startup are unaffected by these 
changes.

As @pron mentioned, the presence of `-agentlib/agentpath` option serves as 
an explicit user consent to use the tool, and disallowing such agents (or 
issuing a warning about them) is a non-goal of the JEP.

With the current behavior, users will be tempted to add 
`-XX:+EnableDynamicAgentLoading` option instead of putting one particular agent 
on the command line. This again does not serve the purpose of strengthen the 
integrity.

The use cases I mentioned here are quite natural and supported by popular 
profilers: a user may want to use a tool some time after the JVM startup, or 
reconfigure it later at runtime similarly to how `jcmd JFR.configure` works.

-

Changes requested by apangin (no project role).

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


Re: New candidate JEP: 451: Prepare to Disallow the Dynamic Loading of Agents

2023-05-19 Thread Andrei Pangin
dynamic loading of agents does not prevent
   libraries from obtaining superpowers - they can simply call System.load().
   At the same time, disabling dynamic loading of agents has a huge impact on
   serviceability, up to the complete inability to use external tools at
   runtime. I understand that the plan is to disallow JNI someday too (unless
   explicitly allowed via a command line option) for the purpose of integrity.
   Following your goals, it would be more logical to disallow JNI first, as it
   is an easier way for libraries to break integrity.



To summarize the above, the current proposal does not seem to me elaborate
enough for targeting to JDK 21. I would suggest improving it by 1)
actualizing assumptions; 2) taking mentioned use cases into account; 3)
providing read-to-use alternatives; 4) matching the plan with the goals.



Thank you,

Andrei Pangin

пт, 19 мая 2023 г. в 15:44, Ron Pressler :

> Because the discussion of this JEP has veered in many directions, let me
> summarise where we are:
>
> This JEP proposes to emit a suppressible warning when a JVM TI or Java
> agent is loaded into a JVM sometime after startup through the Attach
> mechanism.
>
> The warning helps make users aware that an agent has been injected into
> the JVM and identify deployments that may need adjustment in advance of any
> future changes to disallow agents from being dynamically loaded without the
> application's consent. The warning will also let us better judge the impact
> of such a future change.
>
> — Ron
>
> > On 8 May 2023, at 20:17, Mark Reinhold  wrote:
> >
> > https://openjdk.org/jeps/451
> >
> >  Summary: Issue warnings when agents are loaded dynamically into a
> >  running JVM. These warnings aim to prepare users for a future release
> >  which disallows the dynamic loading of agents by default in order to
> >  improve integrity by default. Serviceability tools that load agents at
> >  startup will not cause warnings to be issued in any release.
> >
> > - Mark
>
>


Re: Disallowing the dynamic loading of agents by default

2023-03-19 Thread Andrei Pangin
Hi all,

Serviceability has been one of the biggest Java strengths, but the proposed
change is going to have a large negative impact on it.

Disallowing dynamic agents by default means it will no longer be possible
to attach a profiler to a running app in runtime. JFR cannot close this gap
due to lack of capabilities modern Java profilers have (that's a separate
topic though).

When an issue happens with a live app, it's already too late to add a
command line argument. Furthermore, it may not be even feasible to add an
agent at startup in containerized applications. Starting profiler on demand
from the host OS or from a sidecar is the only viable solution in these
cases.

Next, it's hard to predict beforehand what tools exactly might be useful
for troubleshooting: e.g., one tool may be better for finding memory leaks,
a different one for analyzing CPU performance. Adding all possible tools at
startup does not seem a reasonable approach, especially when tools may
conflict with each other.

The most important aspect of dynamic agents is the possibility to make a
special tool just in time for solving a particular problem. A typical
example is to get a value of some field in a live app without dumping the
entire 60 GB heap. Another common use case is hot patching for fixing
trivial bugs or for adding debug logs dynamically. The prominent example is
when the dynamic agent has proved irreplaceable aid in addressing the
notorious log4j vulnerabilities CVE-2021-44228 and CVE-2021-45046.

I would be grateful to know more about the reasons why we should give up
all the above advantages of dynamic agents in their good and legitimate use
cases.

Thank you,
Andrei

чт, 16 мар. 2023 г. в 18:48, Ron Pressler :

> Hi.
>
> In JDK 21 we intend to disallow the dynamic loading of agents by default.
> This
> will affect tools that use the Attach API to load an agent into a JVM some
> time
> after the JVM has started [1]. There is no change to any of the mechanisms
> that
> load an agent at JVM startup (-javaagent/-agentlib on the command line or
> the
> Launcher-Agent-Class attribute in the main JAR's manifest).
>
> This change in default behavior was proposed in 2017 as part of JEP 261
> [2][3].
> At that time the consensus was to switch to this default not in JDK 9 but
> in a
> later release to give tool maintainers sufficient time to inform their
> users.
> To allow the dynamic loading of agents, users will need to specify
> -XX:+EnableDynamicAgentLoading on the command line.
>
> I'll post a draft JEP for review shortly.
>
> -- Ron
>
> [1]:
> https://docs.oracle.com/en/java/javase/19/docs/api/jdk.attach/com/sun/tools/attach/package-summary.html
> [2]: https://openjdk.org/jeps/261
> [3]: https://mail.openjdk.org/pipermail/jigsaw-dev/2017-April/012040.html