Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Wed, 5 Apr 2023 06:55:16 GMT, Serguei Spitsyn wrote: >> I changed the names because I found it very hard to understand what the old >> names represented: "AgentLibrary" vs "Library"? "add_init_agent" vs >> "add_instrumentation_agent", or even "add_loaded_agent"? Also a bit >> confusing that "load_agent_library" would also include statically linked >> agents - no library is loaded there. > > Okay. > Refactoring is usually not easy to review. > With a renaming it becomes harder, so it is better to be conservative. > > There are other side effects to consider: > > - back porting also becomes harder > - developers have to learn new names instead of already known > > The good side is that your refactoring consolidates this code in a well known > locations. :-) Of course, I would not have changed this unless I believe it improves things. The abstraction is now better from the perspective of the rest of the VM. There are now only JVMTI agents, and they are kept in a list. Arguments.cpp adds agents to the list. The same thing for the diagnosticCommand.cpp for dynamically loaded agents. Threads.cpp loads the JVMTI agents, java.cpp unloads agents. All other sites take out an iterator of the subtypes they want to iterate. There is no longer any separation between "agent" and "library"; the subtypes of the agents are now abstracted away. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1158282424
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Mon, 3 Apr 2023 12:59:12 GMT, Markus Grönlund wrote: >> src/hotspot/share/prims/agentList.cpp line 204: >> >>> 202: >>> 203: // Invokes Agent_OnAttach for agents loaded dynamically during runtime. >>> 204: jint AgentList::load_agent(const char* agent_name, const char* >>> absParam, >> >> I feel that it is better to keep the original function name >> "load_agent_library". As you listed there two kinds of agents: Java and >> Native. The function name give a hint it is native agent. Also, it is better >> to avoid changes that aren't really necessary. > > I changed the names because I found it very hard to understand what the old > names represented: "AgentLibrary" vs "Library"? "add_init_agent" vs > "add_instrumentation_agent", or even "add_loaded_agent"? Also a bit confusing > that "load_agent_library" would also include statically linked agents - no > library is loaded there. Okay. Refactoring is usually not easy to review. With a renaming it becomes harder, so it is better to be conservative. There are other side effects to consider: - back porting also becomes harder - developers have to learn new names instead of already known The good side is that your refactoring consolidates this code in a well known locations. :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1158094815
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Tue, 4 Apr 2023 14:41:13 GMT, Markus Grönlund wrote: >> src/hotspot/share/prims/agent.hpp line 1: >> >>> 1: /* >> >> The name for class and file is too general. >> I'm thinking if renaming the files to jvmtiAgent and the class to JvmtiAgent >> would work. >> In general, there exists a convention to name JVMTI file with the "jvmti" >> prefix. >> It is a gray zone between Runtime and JVMTI but seems to belong more to >> JVMTI. >> The same about the AgentList class and file. >> Also, these new files are good candidates to add here: >> >> make/hotspot/lib/JvmFeatures.gmk: >> ifneq ($(call check-jvm-feature, jvmti), true) >> JVM_CFLAGS_FEATURES += -DINCLUDE_JVMTI=0 >> JVM_EXCLUDE_FILES += jvmtiGetLoadedClasses.cpp jvmtiThreadState.cpp >> jvmtiExtensions.cpp \ >> jvmtiImpl.cpp jvmtiManageCapabilities.cpp jvmtiRawMonitor.cpp >> jvmtiUtil.cpp jvmtiTrace.cpp \ >> jvmtiCodeBlobEvents.cpp jvmtiEnv.cpp jvmtiRedefineClasses.cpp >> jvmtiEnvBase.cpp jvmtiEnvThreadState.cpp \ >> jvmtiTagMap.cpp jvmtiEventController.cpp evmCompat.cpp jvmtiEnter.xsl >> jvmtiExport.cpp \ >> jvmtiClassFileReconstituter.cpp jvmtiTagMapTable.cpp >> endif > > Hi Serguei, thanks for taking a look. > > I have updated with the names you suggested. Thanks. Thank you for the update. It looks good to me. I still need to finish my review. Sorry for the latency. It is is a busy time now. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1158087046
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Sat, 1 Apr 2023 03:31:48 GMT, Serguei Spitsyn wrote: >> Markus Grönlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fixes > > src/hotspot/share/prims/agent.hpp line 1: > >> 1: /* > > The name for class and file is too general. > I'm thinking if renaming the files to jvmtiAgent and the class to JvmtiAgent > would work. > In general, there exists a convention to name JVMTI file with the "jvmti" > prefix. > It is a gray zone between Runtime and JVMTI but seems to belong more to JVMTI. > The same about the AgentList class and file. > Also, these new files are good candidates to add here: > > make/hotspot/lib/JvmFeatures.gmk: > ifneq ($(call check-jvm-feature, jvmti), true) > JVM_CFLAGS_FEATURES += -DINCLUDE_JVMTI=0 > JVM_EXCLUDE_FILES += jvmtiGetLoadedClasses.cpp jvmtiThreadState.cpp > jvmtiExtensions.cpp \ > jvmtiImpl.cpp jvmtiManageCapabilities.cpp jvmtiRawMonitor.cpp > jvmtiUtil.cpp jvmtiTrace.cpp \ > jvmtiCodeBlobEvents.cpp jvmtiEnv.cpp jvmtiRedefineClasses.cpp > jvmtiEnvBase.cpp jvmtiEnvThreadState.cpp \ > jvmtiTagMap.cpp jvmtiEventController.cpp evmCompat.cpp jvmtiEnter.xsl > jvmtiExport.cpp \ > jvmtiClassFileReconstituter.cpp jvmtiTagMapTable.cpp > endif Hi Sergui, thanks for taking a look. I have updated with the names you suggested. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1157362083
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Sat, 1 Apr 2023 03:47:26 GMT, Serguei Spitsyn wrote: >> Markus Grönlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fixes > > src/hotspot/share/prims/agentList.cpp line 204: > >> 202: >> 203: // Invokes Agent_OnAttach for agents loaded dynamically during runtime. >> 204: jint AgentList::load_agent(const char* agent_name, const char* absParam, > > I feel that it is better to keep the original function name > "load_agent_library". As you listed there two kinds of agents: Java and > Native. The function name give a hint it is native agent. Also, it is better > to avoid changes that aren't really necessary. I changed the names because I found it very hard to understand what the old names represented: "AgentLibrary" vs "Library"? "add_init_agent" vs "add_instrumentation_agent", or even "add_loaded_agent"? Also a bit confusing that "load_agent_library" would also include statically linked agents - no library is loaded there. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1155930115
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Fri, 31 Mar 2023 11:18:23 GMT, Markus Grönlund wrote:
>> Greetings,
>>
>> We are adding support to let JFR report on Agents.
>>
>> Design
>>
>> An Agent is a library that uses any instrumentation or profiling APIs. Most
>> agents are started and initialized on the command line, but agents can also
>> be loaded dynamically during runtime. Because command line agents initialize
>> during the VM startup sequence, they add to the overall startup time latency
>> in getting the VM ready. The events will report on the time the agent took
>> to initialize.
>>
>> A JavaAgent is an agent written in the Java programming language, using the
>> APIs in the package
>> [java.lang.instrument](https://docs.oracle.com/en/java/javase/19/docs/api/java.instrument/java/lang/instrument/package-summary.html)
>>
>> A JavaAgent is sometimes called a JPLIS agent, where the acronym JPLIS
>> stands for Java Programming Language Instrumentation Services.
>>
>> To report on JavaAgents, JFR will add the new event type jdk.JavaAgent and
>> events will look similar to these two examples:
>>
>> // Command line
>> jdk.JavaAgent {
>> startTime = 12:31:19.789 (2023-03-08)
>> name = "JavaAgent.jar"
>> options = "foo=bar"
>> dynamic = false
>> initializationTime = 12:31:15.574 (2023-03-08)
>> initializationDuration = 172 ms
>> }
>>
>> // Dynamic load
>> jdk.JavaAgent {
>> startTime = 12:31:31.158 (2023-03-08)
>> name = "JavaAgent.jar"
>> options = "bar=baz"
>> dynamic = true
>> initializationTime = 12:31:31.037 (2023-03-08)
>> initializationDuration = 64,1 ms
>> }
>>
>> The jdk.JavaAgent event type is a JFR periodic event that iterates over
>> running Java agents.
>>
>> For a JavaAgent event, the agent's name will be the specific .jar file
>> containing the instrumentation code. The options will be the specific
>> options passed to the .jar file as part of launching the agent, for example,
>> on the command line: -javaagent: JavaAgent.jar=foo=bar.
>>
>> The "dynamic" field denotes if the agent was loaded via the command line
>> (dynamic = false) or dynamically (dynamic = true)
>>
>> "initializationTime" is the timestamp the JVM invoked the initialization
>> method, and "initializationDuration" is the duration of executing the
>> initialization method.
>>
>> "startTime" represents the time the JFR framework issued the periodic event;
>> hence "initializationTime" will be earlier than "startTime".
>>
>> An agent can also be written in a native programming language using the [JVM
>> Tools Interface
>> (JVMTI)](https://docs.oracle.com/en/java/javase/19/docs/specs/jvmti.html).
>> This kind of agent, sometimes called a native agent, is a platform-specific
>> binary, sometimes referred to as a library, but here it means a .so or .dll
>> file.
>>
>> To report on native agents, JFR will add the new event type jdk.NativeAgent
>> and events will look similar to this example:
>>
>> jdk.NativeAgent {
>> startTime = 12:31:40.398 (2023-03-08)
>> name = "jdwp"
>> options = "transport=dt_socket,server=y,address=any,onjcmd=y"
>> dynamic = false
>> initializationTime = 12:31:36.142 (2023-03-08)
>> initializationDuration = 0,00184 ms
>> path =
>> "c:\ade\github\openjdk\jdk\build\windows-x86_64-server-slowdebug\jdk\bin\jdwp.dll"
>> }
>>
>> The layout of the event type is very similar to the jdk.JavaAgent event, but
>> here the path to the native library is reported.
>>
>> The initialization of a native agent is performed by invoking an
>> agent-specified callback routine. The "initializationTime" is when the JVM
>> sent or would have sent the JVMTI VMInit event to a specified callback.
>> "initializationDuration" is the duration to execute that specific callback.
>> If no callback is specified for the JVMTI VMInit event, the
>> "initializationDuration" will be 0. If the agent is loaded dynamically,
>> "initializationDuration" is the time taken to execute the Agent_OnAttach
>> callback.
>>
>> Implementation
>>
>> There has not existed a reification of a JavaAgent directly in the JVM, as
>> these are built on top of the JDK native library, "instrument", using a
>> many-to-one mapping. At the level of the JVM, the only representation of
>> agents after startup is through JvmtiEnv's, which agents request from the
>> JVM during startup and initialization — as such, mapping which JvmtiEnv
>> belongs to what JavaAgent was not possible before.
>>
>> Using implementation details of how the JDK native library "instrument"
>> interacts with the JVM, we can build this mapping to track what JvmtiEnv's
>> "belong" to what JavaAgent. This mapping now lets us report the
>> Java-relevant context (name, options) and measure the time it takes for the
>> JavaAgent to initialize.
>>
>> When implementing this capability, it was necessary to refactor the code
>> used to represent agents, AgentLibrary. The previous implementation was
>> located primarily in arguments.cpp, a
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
On Fri, 31 Mar 2023 11:18:23 GMT, Markus Grönlund wrote:
>> Greetings,
>>
>> We are adding support to let JFR report on Agents.
>>
>> Design
>>
>> An Agent is a library that uses any instrumentation or profiling APIs. Most
>> agents are started and initialized on the command line, but agents can also
>> be loaded dynamically during runtime. Because command line agents initialize
>> during the VM startup sequence, they add to the overall startup time latency
>> in getting the VM ready. The events will report on the time the agent took
>> to initialize.
>>
>> A JavaAgent is an agent written in the Java programming language, using the
>> APIs in the package
>> [java.lang.instrument](https://docs.oracle.com/en/java/javase/19/docs/api/java.instrument/java/lang/instrument/package-summary.html)
>>
>> A JavaAgent is sometimes called a JPLIS agent, where the acronym JPLIS
>> stands for Java Programming Language Instrumentation Services.
>>
>> To report on JavaAgents, JFR will add the new event type jdk.JavaAgent and
>> events will look similar to these two examples:
>>
>> // Command line
>> jdk.JavaAgent {
>> startTime = 12:31:19.789 (2023-03-08)
>> name = "JavaAgent.jar"
>> options = "foo=bar"
>> dynamic = false
>> initializationTime = 12:31:15.574 (2023-03-08)
>> initializationDuration = 172 ms
>> }
>>
>> // Dynamic load
>> jdk.JavaAgent {
>> startTime = 12:31:31.158 (2023-03-08)
>> name = "JavaAgent.jar"
>> options = "bar=baz"
>> dynamic = true
>> initializationTime = 12:31:31.037 (2023-03-08)
>> initializationDuration = 64,1 ms
>> }
>>
>> The jdk.JavaAgent event type is a JFR periodic event that iterates over
>> running Java agents.
>>
>> For a JavaAgent event, the agent's name will be the specific .jar file
>> containing the instrumentation code. The options will be the specific
>> options passed to the .jar file as part of launching the agent, for example,
>> on the command line: -javaagent: JavaAgent.jar=foo=bar.
>>
>> The "dynamic" field denotes if the agent was loaded via the command line
>> (dynamic = false) or dynamically (dynamic = true)
>>
>> "initializationTime" is the timestamp the JVM invoked the initialization
>> method, and "initializationDuration" is the duration of executing the
>> initialization method.
>>
>> "startTime" represents the time the JFR framework issued the periodic event;
>> hence "initializationTime" will be earlier than "startTime".
>>
>> An agent can also be written in a native programming language using the [JVM
>> Tools Interface
>> (JVMTI)](https://docs.oracle.com/en/java/javase/19/docs/specs/jvmti.html).
>> This kind of agent, sometimes called a native agent, is a platform-specific
>> binary, sometimes referred to as a library, but here it means a .so or .dll
>> file.
>>
>> To report on native agents, JFR will add the new event type jdk.NativeAgent
>> and events will look similar to this example:
>>
>> jdk.NativeAgent {
>> startTime = 12:31:40.398 (2023-03-08)
>> name = "jdwp"
>> options = "transport=dt_socket,server=y,address=any,onjcmd=y"
>> dynamic = false
>> initializationTime = 12:31:36.142 (2023-03-08)
>> initializationDuration = 0,00184 ms
>> path =
>> "c:\ade\github\openjdk\jdk\build\windows-x86_64-server-slowdebug\jdk\bin\jdwp.dll"
>> }
>>
>> The layout of the event type is very similar to the jdk.JavaAgent event, but
>> here the path to the native library is reported.
>>
>> The initialization of a native agent is performed by invoking an
>> agent-specified callback routine. The "initializationTime" is when the JVM
>> sent or would have sent the JVMTI VMInit event to a specified callback.
>> "initializationDuration" is the duration to execute that specific callback.
>> If no callback is specified for the JVMTI VMInit event, the
>> "initializationDuration" will be 0. If the agent is loaded dynamically,
>> "initializationDuration" is the time taken to execute the Agent_OnAttach
>> callback.
>>
>> Implementation
>>
>> There has not existed a reification of a JavaAgent directly in the JVM, as
>> these are built on top of the JDK native library, "instrument", using a
>> many-to-one mapping. At the level of the JVM, the only representation of
>> agents after startup is through JvmtiEnv's, which agents request from the
>> JVM during startup and initialization — as such, mapping which JvmtiEnv
>> belongs to what JavaAgent was not possible before.
>>
>> Using implementation details of how the JDK native library "instrument"
>> interacts with the JVM, we can build this mapping to track what JvmtiEnv's
>> "belong" to what JavaAgent. This mapping now lets us report the
>> Java-relevant context (name, options) and measure the time it takes for the
>> JavaAgent to initialize.
>>
>> When implementing this capability, it was necessary to refactor the code
>> used to represent agents, AgentLibrary. The previous implementation was
>> located primarily in arguments.cpp, a
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
> Greetings,
>
> We are adding support to let JFR report on Agents.
>
> Design
>
> An Agent is a library that uses any instrumentation or profiling APIs. Most
> agents are started and initialized on the command line, but agents can also
> be loaded dynamically during runtime. Because command line agents initialize
> during the VM startup sequence, they add to the overall startup time latency
> in getting the VM ready. The events will report on the time the agent took to
> initialize.
>
> A JavaAgent is an agent written in the Java programming language, using the
> APIs in the package
> [java.lang.instrument](https://docs.oracle.com/en/java/javase/19/docs/api/java.instrument/java/lang/instrument/package-summary.html)
>
> A JavaAgent is sometimes called a JPLIS agent, where the acronym JPLIS stands
> for Java Programming Language Instrumentation Services.
>
> To report on JavaAgents, JFR will add the new event type jdk.JavaAgent and
> events will look similar to these two examples:
>
> // Command line
> jdk.JavaAgent {
> startTime = 12:31:19.789 (2023-03-08)
> name = "JavaAgent.jar"
> options = "foo=bar"
> dynamic = false
> initializationTime = 12:31:15.574 (2023-03-08)
> initializationDuration = 172 ms
> }
>
> // Dynamic load
> jdk.JavaAgent {
> startTime = 12:31:31.158 (2023-03-08)
> name = "JavaAgent.jar"
> options = "bar=baz"
> dynamic = true
> initializationTime = 12:31:31.037 (2023-03-08)
> initializationDuration = 64,1 ms
> }
>
> The jdk.JavaAgent event type is a JFR periodic event that iterates over
> running Java agents.
>
> For a JavaAgent event, the agent's name will be the specific .jar file
> containing the instrumentation code. The options will be the specific options
> passed to the .jar file as part of launching the agent, for example, on the
> command line: -javaagent: JavaAgent.jar=foo=bar.
>
> The "dynamic" field denotes if the agent was loaded via the command line
> (dynamic = false) or dynamically (dynamic = true)
>
> "initializationTime" is the timestamp the JVM invoked the initialization
> method, and "initializationDuration" is the duration of executing the
> initialization method.
>
> "startTime" represents the time the JFR framework issued the periodic event;
> hence "initializationTime" will be earlier than "startTime".
>
> An agent can also be written in a native programming language using the [JVM
> Tools Interface
> (JVMTI)](https://docs.oracle.com/en/java/javase/19/docs/specs/jvmti.html).
> This kind of agent, sometimes called a native agent, is a platform-specific
> binary, sometimes referred to as a library, but here it means a .so or .dll
> file.
>
> To report on native agents, JFR will add the new event type jdk.NativeAgent
> and events will look similar to this example:
>
> jdk.NativeAgent {
> startTime = 12:31:40.398 (2023-03-08)
> name = "jdwp"
> options = "transport=dt_socket,server=y,address=any,onjcmd=y"
> dynamic = false
> initializationTime = 12:31:36.142 (2023-03-08)
> initializationDuration = 0,00184 ms
> path =
> "c:\ade\github\openjdk\jdk\build\windows-x86_64-server-slowdebug\jdk\bin\jdwp.dll"
> }
>
> The layout of the event type is very similar to the jdk.JavaAgent event, but
> here the path to the native library is reported.
>
> The initialization of a native agent is performed by invoking an
> agent-specified callback routine. The "initializationTime" is when the JVM
> sent or would have sent the JVMTI VMInit event to a specified callback.
> "initializationDuration" is the duration to execute that specific callback.
> If no callback is specified for the JVMTI VMInit event, the
> "initializationDuration" will be 0. If the agent is loaded dynamically,
> "initializationDuration" is the time taken to execute the Agent_OnAttach
> callback.
>
> Implementation
>
> There has not existed a reification of a JavaAgent directly in the JVM, as
> these are built on top of the JDK native library, "instrument", using a
> many-to-one mapping. At the level of the JVM, the only representation of
> agents after startup is through JvmtiEnv's, which agents request from the JVM
> during startup and initialization — as such, mapping which JvmtiEnv belongs
> to what JavaAgent was not possible before.
>
> Using implementation details of how the JDK native library "instrument"
> interacts with the JVM, we can build this mapping to track what JvmtiEnv's
> "belong" to what JavaAgent. This mapping now lets us report the Java-relevant
> context (name, options) and measure the time it takes for the JavaAgent to
> initialize.
>
> When implementing this capability, it was necessary to refactor the code used
> to represent agents, AgentLibrary. The previous implementation was located
> primarily in arguments.cpp, and threads.cpp but also jvmtiExport.cpp.
>
> The refactoring isolates the relevant logic into two new modules,
> prims/agent.hpp and prims/agentList.hpp. Breaking out th
