Re: RFR: 8304824: NMT should not use ThreadCritical [v9]
On Mon, 28 Oct 2024 16:12:39 GMT, Robert Toyonaga wrote: >> ### Summary >> This PR just replaces `ThreadCritical` with a lock specific to NMT. >> `ThreadCritical` is a big lock and is unnecessary for the purposes of NMT. >> I've implemented the new lock with a semaphore so that it can be used early >> before VM init. There is also the possibility of adding assertions in >> places we expect NMT to have synchronization. I haven't added assertions yet >> in many places because some code paths such as the (NMT tests) don't lock >> yet. I think it makes sense to close any gaps in locking in another PR in >> which I can also add more assertions. >> >> Testing: >> - hotspot_nmt >> - gtest:VirtualSpace >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with two > additional commits since the last revision: > > - add a comment explaining lock rank > - remove unnecessary dropping of tracking level src/hotspot/share/runtime/mutexLocker.hpp line 31: > 29: #include "runtime/flags/flagSetting.hpp" > 30: #include "runtime/mutex.hpp" > 31: #include "runtime/thread.hpp" This include is not needed because there are no uses that require the definition of Thread. After pull, I am getting circularity errors from runtime/thread.hpp -> jfr/support/jfrThreadLocal.hpp (which now needs runtime/mutexLocker.hpp (-> runtime/thread.hpp) - PR Review Comment: https://git.openjdk.org/jdk/pull/20852#discussion_r1826330949
[jdk23] Integrated: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
On Wed, 17 Jul 2024 11:21:04 GMT, Markus Grönlund wrote: > 8334781: JFR crash: assert(JfrTraceIdBits::load(klass)) & > ((JfrTraceIdEpoch::this_epoch_method_and_class_bits( != 0))) failed: > invariant This pull request has now been integrated. Changeset: 88775f95 Author:Markus Grönlund URL: https://git.openjdk.org/jdk/commit/88775f95f2b25323a75c5acad43ec9131c5e80fe Stats: 34 lines in 7 files changed: 17 ins; 3 del; 14 mod 8334781: JFR crash: assert(JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits( != 0))) failed: invariant Reviewed-by: egahlin Backport-of: 67979eb0771ff834d6d3d18ba5a8bfe161cfc2ce - PR: https://git.openjdk.org/jdk/pull/20217
[jdk23] RFR: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
8334781: JFR crash: assert(JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits( != 0))) failed: invariant - Commit messages: - Backport 67979eb0771ff834d6d3d18ba5a8bfe161cfc2ce Changes: https://git.openjdk.org/jdk/pull/20217/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20217&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334781 Stats: 34 lines in 7 files changed: 17 ins; 3 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/20217.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20217/head:pull/20217 PR: https://git.openjdk.org/jdk/pull/20217
Integrated: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
On Sat, 13 Jul 2024 14:47:21 GMT, Markus Grönlund wrote: > Greetings, > > Please help review this adjustment, which fixes rare situations where methods > that have been retransformed or redefined can be perceived as being tagged by > JFR when they, in fact, are not. The fix unconditionally sets the metatag > clear bits on artefact initialization and adds assertions about the JFR bit > tag state machine. > > Testing: jdk_jfr, stress testing > > Thanks > Markus This pull request has now been integrated. Changeset: 67979eb0 Author:Markus Grönlund URL: https://git.openjdk.org/jdk/commit/67979eb0771ff834d6d3d18ba5a8bfe161cfc2ce Stats: 34 lines in 7 files changed: 17 ins; 3 del; 14 mod 8334781: JFR crash: assert(JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits( != 0))) failed: invariant Reviewed-by: egahlin - PR: https://git.openjdk.org/jdk/pull/20171
Re: RFR: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
On Tue, 16 Jul 2024 00:05:36 GMT, Serguei Spitsyn wrote: >> Greetings, >> >> Please help review this adjustment, which fixes rare situations where >> methods that have been retransformed or redefined can be perceived as being >> tagged by JFR when they, in fact, are not. The fix unconditionally sets the >> metatag clear bits on artefact initialization and adds assertions about the >> JFR bit tag state machine. >> >> Testing: jdk_jfr, stress testing >> >> Thanks >> Markus > > src/hotspot/share/jfr/support/jfrTraceIdExtension.hpp line 47: > >> 45: #define RESTORE_ID(k) JfrTraceId::restore(k); >> 46: >> 47: static constexpr const uint16_t cleared_epoch_bits = 512 | 256; > > Q: Could the `EPOCH_CLEARED_BITS` be used instead? No. because I want to avoid dragging in all of the definitions in jfrTraceIdMarcros.hpp. - PR Review Comment: https://git.openjdk.org/jdk/pull/20171#discussion_r1679179786
RFR: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
Greetings, Please help review this adjustment, which fixes rare situations where methods that have been retransformed or redefined can be perceived as being tagged by JFR when they, in fact, are not. The fix unconditionally sets the metatag clear bits on artefact initialization and adds assertions about the JFR bit tag state machine. Testing: jdk_jfr, stress testing Thanks Markus - Commit messages: - 8334781 Changes: https://git.openjdk.org/jdk/pull/20171/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20171&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8334781 Stats: 34 lines in 7 files changed: 17 ins; 3 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/20171.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20171/head:pull/20171 PR: https://git.openjdk.org/jdk/pull/20171
Re: [jdk23] RFR: 8324089: Fix typo in the manual page for "jcmd" (man jcmd)
On Thu, 4 Jul 2024 10:54:22 GMT, Erik Gahlin wrote: > 8324089: Fix typo in the manual page for "jcmd" (man jcmd) Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20030#pullrequestreview-2158706543
Re: [jdk23] RFR: 8322812: Manpage for jcmd is missing JFR.view command
On Thu, 4 Jul 2024 10:51:48 GMT, Erik Gahlin wrote: > 8322812: Manpage for jcmd is missing JFR.view command Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20028#pullrequestreview-2158707721
Re: RFR: 8324089: Fix typo in the manual page for "jcmd" (man jcmd)
On Wed, 3 Jul 2024 12:05:43 GMT, Erik Gahlin wrote: > Could I have a review of a typo for JFR.start and JFR.dump? > > Testing: tier1 > > Thanks > Erik Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20003#pullrequestreview-2156282430
Re: RFR: 8322812: Manpage for jcmd is missing JFR.view command
On Fri, 28 Jun 2024 14:51:57 GMT, Erik Gahlin wrote: > Could I have a review of a change to the jcmd man page? A typo was also fixed > for JFR.start. > > Testing: tier1 > > Thanks > Erik Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19942#pullrequestreview-2148543225
Integrated: 8323883: JFR AssertionError: Missing object ID 15101
On Thu, 8 Feb 2024 13:46:40 GMT, Markus Grönlund wrote: > Greetings, > > The following adjustments fix the intermittent issues with incomplete tag > sets for a chunk. The situations are pretty subtle: > > 1. A situation can occur where an event is emitted during the event > instrumentation callback as part of JVMTI Retransform (ErrorThrownEvent). A > stack trace is captured that marks, or "tags", the existing method(s) of the > klass (i.e. when the scratch class is instrumented). But after the > instrumentation call returns, the set of methods of the klass is exchanged > from the "old" methods (now marked) , to the new set of methods (from the > scratch class, not marked). This means the mark/tag is lost for a method. > Therefore, method tags must be transferred when exchanging methods during > JVMTI redefine / retransform. > 2. OldObjectSample events are emitted at the end of a recording. As part of > writing the events, several checkpoint events are also created and enqueued. > But these checkpoints are incomplete standalone because they need to be > complemented with a typeset checkpoint, which happens just after, as part of > the rotation. But flush() can occur concurrently with OldObjectSample emit, > and can, therefore, serialize these checkpoints to a segment before the > complementary typeset checkpoint has been created. There needs to be mutual > exclusion between OldObjectSample emit() and flush(), now provided by the > JfrRotationLock. > 3. The set of artefacts captured during class unloading is incomplete because > it respects already serialized artefacts. However, because class unloading > typesets are linked to OldObjectSamples and DeprecatedInvocation events, > which survive chunk boundaries, these unloading typesets can be serialized in > later chunks. If the unloading set is incomplete (not transitive), there will > be missing constants. Hence, class unloading typesets require serialization > of the entire transitive set of unloading artefacts, even if they have > already been serialized to the current chunk. > > As part of fixing these issues, the disabled assert is also reactivated. > > Testing: jdk_jfr, stress testing, tier 1-6 > > Thanks > Markus This pull request has now been integrated. Changeset: 737b4c51 Author:Markus Grönlund URL: https://git.openjdk.org/jdk/commit/737b4c515e082239579369d9806307b9f16c4816 Stats: 166 lines in 15 files changed: 81 ins; 48 del; 37 mod 8323883: JFR AssertionError: Missing object ID 15101 Reviewed-by: egahlin - PR: https://git.openjdk.org/jdk/pull/17771
Re: RFR: 8323883: JFR AssertionError: Missing object ID 15101
On Fri, 9 Feb 2024 15:51:32 GMT, Erik Gahlin wrote: >> Greetings, >> >> The following adjustments fix the intermittent issues with incomplete tag >> sets for a chunk. The situations are pretty subtle: >> >> 1. A situation can occur where an event is emitted during the event >> instrumentation callback as part of JVMTI Retransform (ErrorThrownEvent). A >> stack trace is captured that marks, or "tags", the existing method(s) of the >> klass (i.e. when the scratch class is instrumented). But after the >> instrumentation call returns, the set of methods of the klass is exchanged >> from the "old" methods (now marked) , to the new set of methods (from the >> scratch class, not marked). This means the mark/tag is lost for a method. >> Therefore, method tags must be transferred when exchanging methods during >> JVMTI redefine / retransform. >> 2. OldObjectSample events are emitted at the end of a recording. As part of >> writing the events, several checkpoint events are also created and enqueued. >> But these checkpoints are incomplete standalone because they need to be >> complemented with a typeset checkpoint, which happens just after, as part of >> the rotation. But flush() can occur concurrently with OldObjectSample emit, >> and can, therefore, serialize these checkpoints to a segment before the >> complementary typeset checkpoint has been created. There needs to be mutual >> exclusion between OldObjectSample emit() and flush(), now provided by the >> JfrRotationLock. >> 3. The set of artefacts captured during class unloading is incomplete >> because it respects already serialized artefacts. However, because class >> unloading typesets are linked to OldObjectSamples and DeprecatedInvocation >> events, which survive chunk boundaries, these unloading typesets can be >> serialized in later chunks. If the unloading set is incomplete (not >> transitive), there will be missing constants. Hence, class unloading >> typesets require serialization of the entire transitive set of unloading >> artefacts, even if they have already been serialized to the current chunk. >> >> As part of fixing these issues, the disabled assert is also reactivated. >> >> Testing: jdk_jfr, stress testing, tier 1-6 >> >> Thanks >> Markus > > Nice work! > > Let's hope this will be the last "missing object ID" bug to make it into > main-line. Thanks @egahlin for the review. - PR Comment: https://git.openjdk.org/jdk/pull/17771#issuecomment-1943842563
RFR: 8323883: JFR AssertionError: Missing object ID 15101
Greetings, The following adjustments fix the intermittent issues with incomplete tag sets for a chunk. The situations are pretty subtle: 1. A situation can occur where an event is emitted during the event instrumentation callback as part of JVMTI Retransform (ErrorThrownEvent). A stack trace is captured that marks, or "tags", the existing method(s) of the klass (i.e. when the scratch class is instrumented). But after the instrumentation call returns, the set of methods of the klass is exchanged from the "old" methods (now marked) , to the new set of methods (from the scratch class, not marked). This means the mark/tag is lost for a method. Therefore, method tags must be transferred when exchanging methods during JVMTI redefine / retransform. 2. OldObjectSample events are emitted at the end of a recording. As part of writing the events, several checkpoint events are also created and enqueued. But these checkpoints are incomplete standalone because they need to be complemented with a typeset checkpoint, which happens just after, as part of the rotation. But flush() can occur concurrently with OldObjectSample emit, and can, therefore, serialize these checkpoints to a segment before the complementary typeset checkpoint has been created. There needs to be mutual exclusion between OldObjectSample emit() and flush(), now provided by the JfrRotationLock. 3. The set of artifacts captured during class unloading is incomplete because it respects already serialized artifacts. However, because class unloading typesets are linked to OldObjectSamples and DeprecationInvocation events, which survive chunk boundaries, those unloading typesets can be serialized in later chunks. If the unloading set is incomplete (not transitive), there will be missing constants. Hence, class unloading typesets require serialization of the entire transitive set of unloading artefacts, even if they have already been serialized to the current chunk. As part of fixing these issues, the disabled assert is also reactivated. Testing: jdk_jfr, stress testing, tier 1-6 Thanks Markus - Commit messages: - revert jfrTraceIdMacros.hpp - revert jfrTraceId.cpp - cleared bits not needed on initialization - 8323883 Changes: https://git.openjdk.org/jdk/pull/17771/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17771&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8323883 Stats: 167 lines in 15 files changed: 81 ins; 48 del; 38 mod Patch: https://git.openjdk.org/jdk/pull/17771.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17771/head:pull/17771 PR: https://git.openjdk.org/jdk/pull/17771
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Fri, 1 Sep 2023 11:26:48 GMT, Alan Bateman wrote: >> Ok. If the thread can run native code as part of the mount / unmount, and >> the sampler can see the intermediary state there too, the check is needed. > > Thanks. One other thing that I see when more testing with generational ZGC is > that ZPageAllocator::alloc_page will record an event and this can happen > during a mount transition. So I think JfrStackTrace::record also needs to > test if the current thread is a virtual thread without a continuation mounted. That is not good. Such a check would be attributed to all event sites, making them all slower. - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1312915249
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Wed, 30 Aug 2023 13:56:42 GMT, Alan Bateman wrote: > In the virtual thread implementation, thread identity switches to the carrier > before freezing and switches back to the virtual thread after thawing. This > was a forced move due to issues getting JVMTI to work with virtual threads. > JVMTI can now hide events during transitions so we can invert the sequence > back to mounting before running the continuation, unmounting after freezing, > and re-mounting after thawing. This sequence is important for future changes > that will initiate the freezing from the VM. > > The change requires an update to the JFR thread sampler to skip sampling when > it samples during a transition. > > Testing: tier1-5 Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15492#pullrequestreview-1606720202
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Thu, 31 Aug 2023 19:28:50 GMT, Alan Bateman wrote: >> There are some native methods that we execute during mount/unmount >> transitions. From what I see they all seem to be defined as >> `@IntrinsicCandidate`, but if for some reason we don't execute the intrinsic >> version (interp only mode for instance) then we would call a normal native >> method. > > Just to add that Patricio suggested today to run the stress tests with -Xint > and that does lead to triggering the assert quickly when the thread is > sampled in native. There are several native methods that are > @IntrinsicCandidate that are invoked after the thread identity has changed to > the virtual thread and before the continuation is mounted. Probably less > likely on the unmount as the only native method is the one that reverts the > thread identity. Ok. If the thread can run native code as part of the mount / unmount, and the sampler can see the intermediary state there too, the check is needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1312875905
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Thu, 31 Aug 2023 11:41:03 GMT, Alan Bateman wrote:
>> src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 410:
>>
>>> 408: }
>>> 409: if (JAVA_SAMPLE == type) {
>>> 410: if (thread_state_in_java(thread) &&
>>> !is_vthread_in_transition(thread)) {
>>
>> I think this check can be postponed until after the thread is suspended.
>
> Is the check in OSThreadSampler::protected_task in the right place? That
> seems to be a suspended context.
>
> For JfrThreadSampleClosure::do_sample_thread, I think I may have mis-read the
> code. I thought it was suspended but looking at it again them maybe it should
> be the JfrNativeSamplerCallback implementation. This is probably an area
> where I need help to get right.
OSThreadSampler::protected_task() is the correct place to perform the test. The
sampled thread is completely suspended at this location. The other locations
are optimistic, they read the thread state while the sampled thread is running
to avoid signalling a thread unnecessarily. You don't need the
is_vthread_in_transtition() check there, especially not for the
thread_in_native case, since that would not be possible, would it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311632129
Re: RFR: 8315373: Change VirtualThread to unmount after freezing, re-mount before thawing
On Wed, 30 Aug 2023 13:56:42 GMT, Alan Bateman wrote:
> In the virtual thread implementation, thread identity switches to the carrier
> before freezing and switches back to the virtual thread after thawing. This
> was a forced move due to issues getting JVMTI to work with virtual threads.
> JVMTI can now hide events during transitions so we can invert the sequence
> back to mounting before running the continuation, unmounting after freezing,
> and re-mounting after thawing. This sequence is important for future changes
> that will initiate the freezing from the VM.
>
> The change requires an update to the JFR thread sampler to skip sampling when
> it samples during a transition.
>
> Testing: tier1-5
src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 410:
> 408: }
> 409: if (JAVA_SAMPLE == type) {
> 410: if (thread_state_in_java(thread) &&
> !is_vthread_in_transition(thread)) {
I think this check can be postponed until after the thread is suspended.
src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 415:
> 413: } else {
> 414: assert(NATIVE_SAMPLE == type, "invariant");
> 415: if (thread_state_in_native(thread) &&
> !is_vthread_in_transition(thread)) {
Is this possible? I assume the thread is in _thread_in_Java during the
transition?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311439052
PR Review Comment: https://git.openjdk.org/jdk/pull/15492#discussion_r1311437865
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v8]
On Mon, 8 May 2023 11:01:56 GMT, Johan Sjölen wrote: >> Johan Sjölen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Dead assert > > Passes tier1. Thanks @jdksjolen , I am rubberstamping this. - PR Comment: https://git.openjdk.org/jdk/pull/12034#issuecomment-1538260329
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/ [v8]
On Mon, 8 May 2023 10:09:29 GMT, Johan Sjölen wrote: >> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory >> share/jfr/. Unfortunately the script that does the change isn't perfect, and >> so we >> need to comb through these manually to make sure nothing has gone wrong. I >> also review these changes but things slip past my eyes sometimes. >> >> Here are some typical things to look out for: >> >> No changes but copyright header changed (probably because I reverted >> some changes but forgot the copyright). >> Macros having their NULL changed to nullptr, these are added to the >> script when I find them. They should be NULL. >> nullptr in comments and logs. We try to use lower case "null" in these >> cases as it reads better. An exception is made when code expressions are in >> a comment. >> >> An example of this: >> >> // This function returns null >> void* ret_null(); >> // This function returns true if *x == nullptr >> bool is_nullptr(void** x); >> >> Note how nullptr participates in a code expression here, we really are >> talking about the specific value nullptr. >> >> Thanks! > > Johan Sjölen has updated the pull request incrementally with one additional > commit since the last revision: > > Dead assert Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/12034#pullrequestreview-1416686566
Re: RFR: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967 [v3]
On Wed, 19 Apr 2023 02:22:38 GMT, David Holmes wrote: > @mgronlun I agree this does not look good. I'm not sure this was the right > way to conditionalize the new code, rather than ensuring the callsites were > conditionalized on INCLUDE_JVMTI. It follows the same pattern as for other jvmti*.cpp files also excluded via make/hotspot/lib/jvmFeatures.gmk. For example, jvmtiExport.cpp. Might be better to improve with conditionalized callsites, agree. - PR Comment: https://git.openjdk.org/jdk/pull/13512#issuecomment-1514547771
Integrated: 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967
On Tue, 18 Apr 2023 16:59:29 GMT, Markus Grönlund wrote: > Greetings, > > For most platforms, os::dll_address_to_library_name() only sets offset = -1 > in case of errors. If there is an error, the function returns false. This is > fine. > > On AIX, the offset, being optional, is invariantly set to -1, even in the > case of non-errors. > > Easiest to remove the assertion for a positive offset. > > Thanks > Markus This pull request has now been integrated. Changeset: c738c8ea Author:Markus Grönlund URL: https://git.openjdk.org/jdk/commit/c738c8ea3e9fda87abb03acb599a2433a344db09 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967 Reviewed-by: sspitsyn, dholmes, mbaesken - PR: https://git.openjdk.org/jdk/pull/13513
Re: RFR: 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967
On Tue, 18 Apr 2023 17:15:46 GMT, Serguei Spitsyn wrote: >> Greetings, >> >> For most platforms, os::dll_address_to_library_name() only sets offset = -1 >> in case of errors. If there is an error, the function returns false. This is >> fine. >> >> On AIX, the offset, being optional, is invariantly set to -1, even in the >> case of non-errors. >> >> Easiest to remove the assertion for a positive offset. >> >> Thanks >> Markus > > Looks good to me. > Thanks, > Serguei Thank you @sspitsyn, @dholmes-ora and @MBaesken for your reviews. - PR Comment: https://git.openjdk.org/jdk/pull/13513#issuecomment-1514533473
RFR: 8306278: jvmtiAgentList.cpp:253 assert(offset >= 0) failed: invariant occurs on AIX after JDK-8257967
Greetings, For most platforms, os::dll_address_to_library_name() only sets offset = -1 in case of errors. If there is an error, the function returns false. This is fine. On AIX, the offset, being optional, is invariantly set to -1, even in the case of non-errors. Easiest to remove the assertion for a positive offset. Thanks Markus - Commit messages: - remove assertion on positive offset Changes: https://git.openjdk.org/jdk/pull/13513/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13513&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8306278 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13513.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13513/head:pull/13513 PR: https://git.openjdk.org/jdk/pull/13513
Integrated: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967
On Tue, 18 Apr 2023 14:22:21 GMT, Markus Grönlund wrote: > Greetings, > > With [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967), much > refactoring was done to the JVMTI code concerning agents. However, some > platforms do not have JVMTI support, and tier5 of testing builds an embedded > build, linux-arm32-open-cmp-baseline, which failed because the refactoring > did not properly handle conditional compilations for JVMTI. > > JDK-8257967 did run tier5, but it used an existing build, so it did not cause > recompilations of the embedded target :-( > > This changeset adds the conditional constructs to let > linux-arm32-open-cmp-baseline build successfully. > > It does not look good, but there you go... > > Testing: > > Building: linux-arm32-open-cmp-baseline > Building: regular platforms > > Thanks > Markus This pull request has now been integrated. Changeset: 0f3828dd Author:Markus Grönlund URL: https://git.openjdk.org/jdk/commit/0f38288d4a08677efcd15aa8dfde18540130 Stats: 29 lines in 3 files changed: 2 ins; 7 del; 20 mod 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967 Reviewed-by: egahlin, iklam - PR: https://git.openjdk.org/jdk/pull/13512
Re: RFR: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967 [v3]
> Greetings, > > With [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967), much > refactoring was done to the JVMTI code concerning agents. However, some > platforms do not have JVMTI support, and tier5 of testing builds an embedded > build, linux-arm32-open-cmp-baseline, which failed because the refactoring > did not properly handle conditional compilations for JVMTI. > > JDK-8257967 did run tier5, but it used an existing build, so it did not cause > recompilations of the embedded target :-( > > This changeset adds the conditional constructs to let > linux-arm32-open-cmp-baseline build successfully. > > It does not look good, but there you go... > > Testing: > > Building: linux-arm32-open-cmp-baseline > Building: regular platforms > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: SIZE_FORMAT - Changes: - all: https://git.openjdk.org/jdk/pull/13512/files - new: https://git.openjdk.org/jdk/pull/13512/files/80e3d33d..9b231f4e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13512&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13512&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13512.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13512/head:pull/13512 PR: https://git.openjdk.org/jdk/pull/13512
Re: RFR: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967 [v2]
> Greetings, > > With [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967), much > refactoring was done to the JVMTI code concerning agents. However, some > platforms do not have JVMTI support, and tier5 of testing builds an embedded > build, linux-arm32-open-cmp-baseline, which failed because the refactoring > did not properly handle conditional compilations for JVMTI. > > JDK-8257967 did run tier5, but it used an existing build, so it did not cause > recompilations of the embedded target :-( > > This changeset adds the conditional constructs to let > linux-arm32-open-cmp-baseline build successfully. > > It does not look good, but there you go... > > Testing: > > Building: linux-arm32-open-cmp-baseline > Building: regular platforms > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: fix CDS format specifier to get GitHub actions working again - Changes: - all: https://git.openjdk.org/jdk/pull/13512/files - new: https://git.openjdk.org/jdk/pull/13512/files/70bc2625..80e3d33d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13512&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13512&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13512.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13512/head:pull/13512 PR: https://git.openjdk.org/jdk/pull/13512
RFR: 8306282: Build failure linux-arm32-open-cmp-baseline after JDK-8257967
Greetings, With [JDK-8257967](https://bugs.openjdk.org/browse/JDK-8257967), much refactoring was done to the JVMTI code concerning agents. However, some platforms do not have JVMTI support, and tier5 of testing builds an embedded build, linux-arm32-open-cmp-baseline, which failed because the refactoring did not properly handle conditional compilations for JVMTI. JDK-8257967 did run tier5, but it used an existing build, so it did not cause recompilations of the embedded target :-( This changeset adds the conditional constructs to let linux-arm32-open-cmp-baseline build successfully. It does not look good, but there you go... Testing: Building: linux-arm32-open-cmp-baseline Building: regular platforms Thanks Markus - Commit messages: - conditional constructs for embedded Changes: https://git.openjdk.org/jdk/pull/13512/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13512&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8306282 Stats: 29 lines in 3 files changed: 2 ins; 7 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/13512.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13512/head:pull/13512 PR: https://git.openjdk.org/jdk/pull/13512
Integrated: 8257967: JFR: Events for loaded agents
On Wed, 8 Mar 2023 12:41:15 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
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
On Thu, 9 Mar 2023 00:23:39 GMT, David Holmes wrote: >> No need to load any JFR classes. No change to startup logic. > >> No need to load any JFR classes. > > I thought JFR was all Java-based these days. But if no Java involved then > that is good. > >> No change to startup logic. > > I flagged a change in my comment above. Thank you @dholmes-ora, @sspitsyn and @adinn for your reviews and sticking with this one for quite some time. - PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-151104
Re: RFR: 8257967: JFR: Events for loaded agents [v18]
ong" 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with two additional commits since the last revision: - capital letter - explanatory comment in jvmtiExport.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/85f06038..140079c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=16-17 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v17]
On Fri, 14 Apr 2023 22:22:13 GMT, Serguei Spitsyn wrote: >> Ok. the terminology here might be confusing. The concept of an agent being >> "initialized" is introduced and reported by JFR. For example, here is the >> JFR event type definition for a NativeAgent: >> ``` >> > label="Native Agent" description="A native programming language agent making >> use of the JVMTI interface used by development, profiling and monitoring >> tools" >> thread="false" startTime="false" period="endChunk" stackTrace="false"> >> >> >> > /> >> >> > label="Initialization Duration" description="The duration of executing the >> JVMTI VMInit event callback. If no VMInit callback is specified, the >> duration is 0. For a dynamically loaded agent, it is the duration of >> executing the call to Agent_OnAttach." /> >> >> >> >> As you can see, there are two fields: initializationTime and >> IntializationDuration. >> >> We report these to let users understand when an agent was initialized >> (VMInit or Agent_OnAttach), together with the duration it took to execute >> either. For JavaAgents, it measures the invocation and duration of the >> premain or agentmain methods. >> >> "Initialized" does not mean "loaded" (at this point, all agents are loaded), >> but rather it means the agent has received a timestamp set as a function of >> VMInit. This timestamp and duration are what we will report in JFR as part >> of the event. >> >> An "uninitialized" agent is an agent who has not yet been timestamped, as >> part of VMInit, for example. Since an agent can create multiple JvmtiEnvs, >> the function is called lookup_uninitialized_agent() because we can only have >> a single timestamp for an agent, but it can, in turn, have multiple >> JvmtiEnvs. When looking up an agent again, using a second JvmtiEnv created >> by it, the agent is already "initialized", so no agent is returned. >> >> We cannot have the timestamping logic as part of the call out to >> Agent_OnLoad, because that call happens very early during VM bootstrap, so >> the Ticks support structures are not yet in place. But, timing the >> Agent_OnLoad call would be rather meaningless because the agent cannot do >> much except construct a JvmtiEnv and setting capabilities and callbacks. >> >> VMInit is where most of the invocation logic, at least for JavaAgents >> happens, so the measurements are placed there. > > Thank you for explaining it. > Could you, please, add small comment explaining that it is for JFR purposes? Will do. Thank you, Serguei. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1168407443
Re: RFR: 8257967: JFR: Events for loaded agents [v17]
On Fri, 14 Apr 2023 07:50:39 GMT, Serguei Spitsyn wrote: > Markus, It looks good to me. Overall, it is a nice consolidation of the agent > code, good move in general! Thank you for your patience. I've posted one > minor request though. Thanks, Serguei Thanks for taking a look Serguei, appreciate it! - PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-1508680453
Re: RFR: 8257967: JFR: Events for loaded agents [v17]
On Fri, 14 Apr 2023 07:43:23 GMT, Serguei Spitsyn wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> line breaks
>
> src/hotspot/share/prims/jvmtiExport.cpp line 717:
>
>> 715: jvmtiEventVMInit callback = env->callbacks()->VMInit;
>> 716: if (callback != nullptr) {
>> 717: JvmtiAgent* const agent = lookup_uninitialized_agent(env,
>> reinterpret_cast(callback));
>
> It was a surprise to me to discover this code in your changes.
> How can it be possible that some agents are left uninitialized at the time of
> VMInit event?
> Have you really observed this?
> If so, then can you add a comment explaining the need of this initialization?
> Is it needed for JFR purposes only?
Ok. the terminology here might be confusing. The concept of an agent being
"initialized" is introduced and reported by JFR. For example, here is the JFR
event type definition for a NativeAgent:
```
As you can see, there are two fields: initializationTime and
IntializationDuration.
We report these to let users understand when an agent was initialized (VMInit
or Agent_OnAttach), together with the duration it took to execute either. For
JavaAgents, it measures the invocation and duration of the premain or agentmain
methods.
"Initialized" does not mean "loaded" (at this point, all agents are loaded),
but rather it means the agent has received a timestamp set as a function of
VMInit. This timestamp and duration are what we will report in JFR as part of
the event.
An "uninitialized" agent is an agent who has not yet been timestamped, as part
of VMInit, for example. Since an agent can create multiple JvmtiEnvs, the
function is called lookup_unitialized_agent(), because we can only have a
single timestamp for an agent, but it can, in turn, have multiple JvmtiEnvs.
When looking up an agent again, using a second JvmtiEnv created by it, the
agent is already "initialized", so no agent is returned.
We cannot have the timestamping logic as part of the call out to Agent_OnLoad,
because that call happens very early during VM bootstrap, so the Ticks support
structures are not yet in place. But, timing the Agent_OnLoad call would be
rather meaningless because the agent cannot do much except construct a JvmtiEnv
and setting capabilities and callbacks.
VMInit is where most of the invocation logic, at least for JavaAgents happens,
so the measurements are placed there.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1166568901
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Thu, 13 Apr 2023 10:24:55 GMT, Serguei Spitsyn wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> renames
>
> src/hotspot/share/prims/jvmtiAgentList.cpp line 72:
>
>> 70: // there exist an order requirement to iterate oldest -> newest. Our
>> concurrent storage linked-list is newest -> oldest.
>> 71: // The correct order is preserved by the iterator, by storing a filtered
>> set of entries in a stack.
>> 72: JvmtiAgentList::Iterator::Iterator(JvmtiAgent** list, Filter filter) :
>> _stack(new GrowableArrayCHeap(16)),
>> _filter(filter) {
>
> Nit: It'd be nice to make the lines 69-72 shorter.
Ok. Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1165429976
Re: RFR: 8257967: JFR: Events for loaded agents [v17]
ong" 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: line breaks - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/d0fd9e97..85f06038 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=15-16 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Thu, 13 Apr 2023 10:15:02 GMT, Serguei Spitsyn wrote:
> Your fix introduced a hidden dependency of this new structure on the
> JPLISEnvironment structure and some Java agents implementation details:
>
> ```
> 202 struct JPLISEnvironmentMirror {
> 203 jvmtiEnv* mJVMTIEnv; // the JVMTI environment
> 204 const void* mAgent; // corresponding agent
> 205 jboolean mIsRetransformer; // indicates if special environment
> 206 };
> ```
>
> It does not look good to me but I can't suggest any other approach at the
> moment. How important is this part? Have you considered other ways to achieve
> what is needed?
Yes. It is the key to locating which JavaAgent maps to which JvmtiEnv. I tried
some other variants, but those would change the layout of the exported structs
in jplisAgent.h, and I don't know if people depend on that layout, implicitly
or explicitly. So I choose not go down that route.
This seemed the best alternative since we own jdk.instrument and the
implementation on the JDK side is unlikely to change very much.
-
PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-1506830279
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Thu, 13 Apr 2023 10:07:50 GMT, Serguei Spitsyn wrote: > What was the reason to clone the classes below ?: > `JvmtiJavaThreadEventTransition` => `AgentJavaThreadEventTransition` > `JvmtiThreadEventMark` => `AgentThreadEventMark` `JvmtiEventMark` => > `AgentEventMark` The reason is they are used when invoking Agent_OnAttach. Those classes are defined in jvmtiExport.cpp, so not reachable. I considered exporting them, but it would require additional headers to be included. I opted for just replicating them, also with static linkage. - PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-1506825122
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Wed, 12 Apr 2023 11:01:43 GMT, Serguei Spitsyn wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> renames
>
> src/hotspot/share/prims/jvmtiAgent.cpp line 357:
>
>> 355: vm_exit_during_initialization("Could not find JVM_OnLoad or
>> Agent_OnLoad function in the library", name());
>> 356: }
>> 357: _xrun = false; // converted
>
> Just questions to understand it better.
> Neither `JVM_Onload` nor `Agent_Onload` entry points are stored after these
> lookups. It means that in order to be called later (as the comment at line
> 350 says) they have to be looked up again.
> Is it right? Was it the same originally?
The entry points are not saved and so have to be looked up again. It was the
same originally.
That is why there is a check and branch on agent->is_loaded().
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1163979282
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Tue, 4 Apr 2023 14:39:19 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 leve
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Wed, 12 Apr 2023 10:31:37 GMT, Serguei Spitsyn wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> renames
>
> src/hotspot/share/prims/jvmtiAgent.cpp line 323:
>
>> 321: assert(agent != nullptr, "invariant");
>> 322: if (!agent->is_loaded()) {
>> 323: if (!load_agent_from_executable(agent, on_load_symbols,
>> num_symbol_entries)) {
>
> It feels like I'm missing something.
> We already checked and found at line 322 that `agent->is_loaded() == false`.
> Also, we have the comment at line 265:
>
> 265 // If this function returns true, then agent->is_static_lib().&&
> agent->is_loaded().
> 266 static bool load_agent_from_executable(Agent* agent, const char*
> on_load_symbols[], size_t num_symbol_entries) {
>
> As the `agent->is_loaded() == false` then t he condition
> `agent->is_static_lib() && agent->is_loaded()` has to be `false` and can not
> be `true`. Then one of the if-checks at lines 322 and 323 is not needed and
> can be removed. Is it right? Otherwise, the comment at line 265 can be
> incorrect.
Good observation, Serguei.
It is because some paths call into lookup_On_load_Entry_point() twice.
It is primarily the attempted conversion of xrun agents, the first invocation
comes from JvmtiAgent::convert_xrun_agent(). This will have the agent "loaded".
If there is an Agent_OnLoad function, the agent is converted (i.e. xrun
removed).
Then when the agent is to invoke the Agent_OnLoad function, there is a second
invocation. Here a converted xrun library is already loaded, so I bypass
attempting to load it again by checking the is_loaded() property.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1163954754
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Tue, 11 Apr 2023 16:56:49 GMT, Serguei Spitsyn wrote: > > Can I please get a second review to close this one out? > > Markus, I'm still working on it and close to finish. I have some questions to > ask. In fact, I gave up to prove this refactoring does not break anything. > So, we should rely on testing. No worries, Serguei. Thank you for taking a look, please take your time. I have run tiers 1-6. - PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-1503779085
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Tue, 4 Apr 2023 14:39:19 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 leve
Re: RFR: 8257967: JFR: Events for loaded agents [v16]
On Wed, 5 Apr 2023 01:48:19 GMT, David Holmes wrote: > Renamings look good to me. Thank you for your review! - PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-1497209787
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 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 [v16]
ong" 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: renames - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/07407a82..d0fd9e97 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=14-15 Stats: 2158 lines in 19 files changed: 1059 ins; 1059 del; 40 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
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 [v14]
On Fri, 31 Mar 2023 03:05:31 GMT, David Holmes wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> restore misssing frees
>
> src/hotspot/share/prims/agent.cpp line 533:
>
>> 531: if (thread->is_pending_jni_exception_check()) {
>> 532: thread->clear_pending_jni_exception_check();
>> 533: }
>
> Unsure why we pretend the agent checked this - don't we want -Xcheck:jni to
> report a bug in the agent?
Good question - I don't know. For dynamically loaded agents, there seems to be
quite a lot of handling to return a JNI_OK, even though the agent failed to
load or returned failure from the Agent_OnAttach. e.g.
// Agent_OnAttach executed so completion status is JNI_OK
return JNI_OK;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1154346856
Re: RFR: 8257967: JFR: Events for loaded agents [v15]
ong" 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: fixes - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/ab74621b..07407a82 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=13-14 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Tue, 14 Mar 2023 12:22:16 GMT, Markus Grönlund wrote: >> n.b. that also applies for accesses/updates to field _next. > > I wanted all accesses to use the iterator. The only access is given to the > iterator and AgentList by way of being friends. No need to expose more. I updated all external access to use getters/setters. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1153703241
Re: RFR: 8257967: JFR: Events for loaded agents [v9]
On Tue, 14 Mar 2023 12:19:50 GMT, Markus Grönlund wrote: >> src/hotspot/share/prims/agent.cpp line 41: >> >>> 39: char* copy = AllocateHeap(length + 1, mtInternal); >>> 40: strncpy(copy, str, length + 1); >>> 41: assert(strncmp(copy, str, length + 1) == 0, "invariant"); >> >> Unclear what you are checking here. Don't you trust strncpy? > > Maybe a bit paranoid, yes. I can clean up. updated to use os:::strdup - cheers. - PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1153702643
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Tue, 14 Mar 2023 12:26:16 GMT, Markus Grönlund wrote: > I've had a good look through now and have a better sense of the refactoring. > Seems good. > > I'll wait for any tweaks before hitting the approve button though. > > Thanks Moving the loading logic to the agent.cpp module was a bit harder than I initially thought. It also exposed a bug in how statically linked libraries were loaded - now fixed. Sorry for the large update. Thanks again for having a look. >> src/hotspot/share/prims/agentList.cpp line 542: >> >>> 540: >>> 541: // Invoke the Agent_OnAttach function >>> 542: JavaThread* THREAD = JavaThread::current(); // For exception macros. >> >> Nit: just use `current` rather than `THREAD` and don't use the exception >> macros. > > Ported as is but good point, will update. Updated - cheers. - PR Comment: https://git.openjdk.org/jdk/pull/12923#issuecomment-1490828465 PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1153703484
Re: RFR: 8257967: JFR: Events for loaded agents [v14]
On Thu, 30 Mar 2023 19:20:11 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
>> initialization = 12:31:15.574 (2023-03-08)
>> initializationTime = 172 ms
>> }
>>
>> // Dynamic load
>> jdk.JavaAgent {
>> startTime = 12:31:31.158 (2023-03-08)
>> name = "JavaAgent.jar"
>> options = "bar=baz"
>> dynamic = true
>> initialization = 12:31:31.037 (2023-03-08)
>> initializationTime = 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)
>>
>> "initialization" is the timestamp the JVM invoked the initialization method,
>> and "initializationTime" is the duration of executing the initialization
>> method.
>>
>> "startTime" represents the time the JFR framework issued the periodic event;
>> hence "initialization" 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
>> initialization = 12:31:36.142 (2023-03-08)
>> initializationTime = 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 "initialization" is when the JVM sent
>> or would have sent the JVMTI VMInit event to a specified callback.
>> "initializationTime" is the duration to execute that specific callback. If
>> no callback is specified for the JVMTI VMInit event, the
>> "initializationTime" will be 0.
>>
>> 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
Re: RFR: 8257967: JFR: Events for loaded agents [v14]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: restore misssing frees - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/c024fac5..ab74621b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=12-13 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v13]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: remove whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/659e6f3d..c024fac5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=11-12 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v12]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge branch 'master' into agents - reviewer feedback, loading to agent.cpp, bugfix loading statically linked agent - more cleanup - handle multiple envs with same VMInit callback - more cleanup - cleanup - fixes - remove implementation details - remove JVMPI - cleanup - ... and 5 more: https://git.openjdk.org/jdk/compare/83cf28f9...659e6f3d - Changes: https://git.openjdk.org/jdk/pull/12923/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=11 Stats: 1889 lines in 22 files changed: 1368 ins; 490 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v11]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: reviewer feedback, loading to agent.cpp, bugfix loading statically linked agent - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/741b8686..0b10773f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=09-10 Stats: 1079 lines in 9 files changed: 481 ins; 426 del; 172 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Tue, 14 Mar 2023 12:23:08 GMT, Markus Grönlund wrote:
>> src/hotspot/share/prims/agentList.cpp line 419:
>>
>>> 417: const jint err = (*on_load_entry)(&main_vm,
>>> const_cast(agent->options()), NULL);
>>> 418: if (err != JNI_OK) {
>>> 419: vm_exit_during_initialization("-Xrun library failed to init",
>>> agent->name());
>>
>> Do you need to be back in `_thread_in_vm` before exiting?
>
> Hmm. This was ported as is. I will double-check.
Looks like there is no requirement to be in _thread_in_vm before invoking
vm_exit_during_initialization().
vm_perform_shutdown_actions() will forcibly set the thread state to
_thread_in_native (no transition).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1153243069
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Mon, 13 Mar 2023 09:46:04 GMT, Andrew Dinn wrote: >> Markus Grönlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> more cleanup > > src/hotspot/share/jfr/metadata/metadata.xml line 1182: > >> 1180: > description="The time the JVM initialized the agent" /> >> 1181: > label="Initialization Time" description="The duration of executing the >> initialization method exported by the agent" /> >> 1182: > > @mgronlun A somewhat drive-by comment. It might be clearer if you renamed > these event fields and accessors, plus also the corresponding fields and > accessors in class Agent, as `initializationTime` and > `initializationDuration`. Makes sense. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Tue, 14 Mar 2023 06:01:05 GMT, David Holmes wrote: > I've had a good look through now and have a better sense of the refactoring. > Seems good. > > > > I'll wait for any tweaks before hitting the approve button though. > > > > Thanks Thanks so much for taking a look. I realized that implementation details of loading should probably reside in agent.cpp, not agentList.cpp. I am currently off on vacation and will update when back. Thanks also to Andrew Dinn for comments. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Mon, 13 Mar 2023 09:49:39 GMT, Andrew Dinn wrote:
>> src/hotspot/share/prims/agentList.cpp line 64:
>>
>>> 62: void AgentList::add_xrun(const char* name, char* options, bool
>>> absolute_path) {
>>> 63: Agent* agent = new Agent(name, options, absolute_path);
>>> 64: agent->_is_xrun = true;
>>
>> Why direct access of private field instead of having a setter like other
>> parts of the Agent API?
>
> n.b. that also applies for accesses/updates to field _next.
I wanted all accesses to use the iterator. The only access is given to the
iterator and AgentList by way of being friends. No need to expose more.
-
PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v9]
On Fri, 10 Mar 2023 06:57:46 GMT, David Holmes wrote: >> Markus Grönlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> handle multiple envs with same VMInit callback > > src/hotspot/share/prims/agent.cpp line 41: > >> 39: char* copy = AllocateHeap(length + 1, mtInternal); >> 40: strncpy(copy, str, length + 1); >> 41: assert(strncmp(copy, str, length + 1) == 0, "invariant"); > > Unclear what you are checking here. Don't you trust strncpy? Maybe a bit paranoid, yes. I can clean up. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Mon, 13 Mar 2023 06:22:21 GMT, David Holmes wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> more cleanup
>
> src/hotspot/share/prims/agent.cpp line 34:
>
>> 32: }
>> 33:
>> 34: static const char* allocate_copy(const char* str) {
>
> Why not just use `os::strdup`?
Better alternative, thanks David.
> src/hotspot/share/prims/agentList.cpp line 227:
>
>> 225: * store data in their JvmtiEnv local storage.
>> 226: *
>> 227: * Please see JPLISAgent.c in module java.instrument, see JPLISAgent.h
>> and JPLISAgent.c.
>
> No need to mention the .c file twice.
Good point.
> src/hotspot/share/prims/agentList.cpp line 419:
>
>> 417: const jint err = (*on_load_entry)(&main_vm,
>> const_cast(agent->options()), NULL);
>> 418: if (err != JNI_OK) {
>> 419: vm_exit_during_initialization("-Xrun library failed to init",
>> agent->name());
>
> Do you need to be back in `_thread_in_vm` before exiting?
Hmm. This was ported as is. I will double-check.
> src/hotspot/share/prims/agentList.cpp line 542:
>
>> 540:
>> 541: // Invoke the Agent_OnAttach function
>> 542: JavaThread* THREAD = JavaThread::current(); // For exception macros.
>
> Nit: just use `current` rather than `THREAD` and don't use the exception
> macros.
Ported as is but good point, will update.
-
PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: more cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/abeaa324..741b8686 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=08-09 Stats: 12 lines in 3 files changed: 1 ins; 10 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v9]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: handle multiple envs with same VMInit callback - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/db48fe8d..abeaa324 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=07-08 Stats: 8 lines in 2 files changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v8]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: more cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/d0609bfb..db48fe8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=06-07 Stats: 4 lines in 3 files changed: 1 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v7]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/80f22257..d0609bfb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=05-06 Stats: 15 lines in 3 files changed: 1 ins; 3 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v6]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: fixes - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/f0c04055..80f22257 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=04-05 Stats: 5 lines in 2 files changed: 1 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v5]
; 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous two lists that maintained "agents" (JVMTI) and "libraries" > (Xrun) were not thread-safe for concurrent iterations. A single list that > allows for concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: remove implementation details - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/355d307c..f0c04055 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=03-04 Stats: 16 lines in 3 files changed: 9 ins; 6 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
On Thu, 9 Mar 2023 09:36:28 GMT, Andrew Dinn wrote: > Yes, I appreciate that `dynamic` can be derived from `initializationMethod` > -- and vice versa. However, I was approaching this semantically from the > opposite end. To me the primary characteristic that the user would be > interested in is whether the agent was loaded dynamically or on the command > line (whatever the type of agent). The corresponding fact, for a Java agent, > that it is entered, respectively, via method agentMain or preMain is a > derived (implementation) detail. Is there a reason to mention that detail? That's a good point. The overall intent was to map what method was measured during initialization. That included native agent callbacks as well. It may be an unnecessary implementation detail and may restrict the possibility of growth. It is probably a better design abstraction to leave out the specific method. I dropped the callback function for the native agent, but we should also do the same for JavaAgents. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
On Thu, 9 Mar 2023 00:23:39 GMT, David Holmes wrote: > > No need to load any JFR classes. > > I thought JFR was all Java-based these days. But if no Java involved then > that is good. Ehh, no. Far from it. > > No change to startup logic. > > I flagged a change in my comment above. Thanks, pls see my reply. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
On Wed, 8 Mar 2023 22:56:31 GMT, David Holmes wrote:
>> Markus Grönlund has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - remove JVMPI
>> - cleanup
>
> src/hotspot/share/runtime/threads.cpp line 338:
>
>> 336: if (EagerXrunInit && Arguments::init_libraries_at_startup()) {
>> 337: create_vm_init_libraries();
>> 338: }
>
> Not obvious where this went. Changes to the initialization order can be very
> problematic.
Thanks, David. Two calls to launch XRun agents are invoked during startup, and
they depend on the EagerXrunInit option. The !EagerXrunInit case is already
located in create_vm(), but the EagerXrunInit was located as the first entry in
initialize_java_lang_classes(), which I thought was tucked away a bit
unnecessarily.
I hoisted the EagerXrunInit from initialize_java_lang_classes() into to
create_vm(). It's now the call just before initialize_java_lang_classes().
This made it clearer, i.e. to have both calls located directly in create_vm().
-
PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
On Wed, 8 Mar 2023 23:28:52 GMT, Markus Grönlund wrote: >> Markus Grönlund has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - remove JVMPI >> - cleanup > > No need to load any JFR classes. No change to startup logic. > @mgronlun Why mark Java agents as command-line or dynamic using > `initializationMethod = "premain"/"agentMain"` and mark native agents using > `dynamic = true/false`? Why not use `dynamic` for both? Hi Andrew, that's a good question. I thought it could be derived in the JavaAgent case, because there are only two entry points, "premain" implies static and "agentmain" implies dynamic. For the native case, there is no information about the callback (I had it, but it depends on symbols), so the dynamic field is made explicit. It can also be added to the JavaAgent if that makes it clearer. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
On Wed, 8 Mar 2023 18:56:55 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"
>> initialization = 12:31:15.574 (2023-03-08)
>> initializationTime = 172 ms
>> initializationMethod = "premain"
>> }
>> // Dynamic load
>> jdk.JavaAgent {
>> startTime = 12:31:31.158 (2023-03-08)
>> name = "JavaAgent.jar"
>> options = "bar=baz"
>> initialization = 12:31:31.037 (2023-03-08)
>> initializationTime = 64,1 ms
>> initializationMethod = "agentmain"
>> }
>>
>> 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 event will also detail which initialization method was invoked by the
>> JVM, "premain" for command line agents, and "agentmain" for agents loaded
>> dynamically.
>>
>> "initialization" is the timestamp the JVM invoked the initialization method,
>> and "initializationTime" is the duration of executing the initialization
>> method.
>>
>> "startTime" represents the time the JFR framework issued the periodic event;
>> hence "initialization" 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"
>> path =
>> "c:\ade\github\openjdk\jdk\build\windows-x86_64-server-slowdebug\jdk\bin\jdwp.dll"
>> dynamic = false
>> initialization = 12:31:36.142 (2023-03-08)
>> initializationTime = 0,00184 ms
>> }
>>
>> The layout of the event type is very similar to the jdk.JavaAgent event, but
>> here the path to the native library is reported, and there is also a
>> denotation if the agent was loaded via the command line (dynamic = false) or
>> dynamically (dynamic = true).
>>
>> The initialization of a native agent is performed by invoking an
>> agent-specified callback routine which is not detailed in the event (for
>> now). The "initialization" is the time the JVM sent or would have sent the
>> JVMTI VMInit event to a specified callback. "initializationTime" is the
>> duration to execute that specific callback. If no callback is specified for
>> the JVMTI VMInit event, the "initializationTime" will be 0.
>>
>> Implementation
>>
>> There has not existed a reification of a JavaAgent directly in the JVM, as
>> these are built on to
Re: RFR: 8257967: JFR: Events for loaded agents [v4]
; > 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. > > To implement this capability, it was necessary to refactor the code used to > represent agents, called 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous lists that maintain the agents (JVMTI) and libraries (JVMPI) are > not thread-safe for concurrent iterations. A single list that allows for > concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with two additional commits since the last revision: - remove JVMPI - cleanup - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/26172f0e..355d307c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v3]
; > 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. > > To implement this capability, it was necessary to refactor the code used to > represent agents, called 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous lists that maintain the agents (JVMTI) and libraries (JVMPI) are > not thread-safe for concurrent iterations. A single list that allows for > concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: update - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/ed1ea797..26172f0e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=01-02 Stats: 13 lines in 2 files changed: 5 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v2]
p 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. > > To implement this capability, it was necessary to refactor the code used to > represent agents, called 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 this code from their > older places will help reduce the sizes of oversized arguments.cpp and > threads.cpp. > > The previous lists that maintain the agents (JVMTI) and libraries (JVMPI) are > not thread-safe for concurrent iterations. A single list that allows for > concurrent iterations is therefore introduced. > > Testing: jdk_jfr, tier 1 - 6 > > Thanks > Markus Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision: razor - Changes: - all: https://git.openjdk.org/jdk/pull/12923/files - new: https://git.openjdk.org/jdk/pull/12923/files/c50cca53..ed1ea797 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12923&range=00-01 Stats: 114 lines in 3 files changed: 33 ins; 74 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/12923.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12923/head:pull/12923 PR: https://git.openjdk.org/jdk/pull/12923
RFR: 8257967: JFR: Event for loaded agents
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
[[ava.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"
initialization = 12:31:15.574 (2023-03-08)
initializationTime = 172 ms
initializationMethod = "premain"
}
// Dynamic load
jdk.JavaAgent {
startTime = 12:31:31.158 (2023-03-08)
name = "JavaAgent.jar"
options = "bar=baz"
initialization = 12:31:31.037 (2023-03-08)
initializationTime = 64,1 ms
initializationMethod = "agentmain"
}
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 event will also detail which initialization method was invoked by the JVM,
"premain" for command line agents, and "agentmain" for agents loaded
dynamically.
"initialization" is the timestamp the JVM invoked the initialization method,
and "initializationTime" is the duration of executing the initialization method.
"startTime" represents the time the JFR framework issued the periodic event;
hence "initialization" will be earlier than "startTime".
An agent can also be written in a native programming language, using either the
JVM Tools Interface (JVMTI) or JVM Profiling Interface (JVMPI). 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.
JVMTI standard spec:ification is
[here](https://docs.oracle.com/en/java/javase/19/docs/specs/jvmti.html)
JVMPI is an older interface, not a standard and is considered superseded by
JVMTI, but the support is still in the JVM for agents started on the command
line: -XRunMyAgent.jar
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"
path =
"c:\ade\github\openjdk\jdk\build\windows-x86_64-server-slowdebug\jdk\bin\jdwp.dll"
dynamic = false
initialization = 12:31:36.142 (2023-03-08)
initializationTime = 0,00184 ms
}
The layout of the event type is very similar to the jdk.JavaAgent event, but
here the path to the native library is reported, and there is also a denotation
if the agent was loaded via the command line (dynamic = false) or dynamically
(dynamic = true).
The initialization of a native agent is performed by invoking an
agent-specified callback routine which is not detailed in the event (for now).
The "initialization" is the time the JVM sent or would have sent the JVMTI
VMInit event to a specified callback. "initializationTime" is the duration to
execute that specific callback. If no callback is specified for the JVMTI
VMInit event, the "initializationTime" will be 0.
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 and use it 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.
In order to implement this capability, it was necessary to refactor the code
used to represent agents, called AgentLibrary. The previous implementation was
located primarily in arguments.cpp, and threads.cpp but also jvmtiE
Re: RFR: JDK-8300245: Replace NULL with nullptr in share/jfr/
On Thu, 19 Jan 2023 20:44:33 GMT, Johan Sjölen wrote:
>> Do the conversion in the share/jfr/ sub-directory and all of its files.
>
> src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp line 1157:
>
>> 1155: const int orig_stream_length = orig_stream->length();
>> 1156: // allocate an identically sized buffer
>> 1157: u1* const new_buffer =
>> NEW_RESOURCE_ARRAY_IN_THREAD_RETURN_nullptr(THREAD, u1, orig_stream_length);
>
> fix
Propagating the term "nullptr" is weird when it is used conceptually, like for
example in comments and macros.
> src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp line 1558:
>
>> 1556: const jint stream_len = new_stream->length();
>> 1557: JvmtiCachedClassFileData* p =
>> 1558: (JvmtiCachedClassFileData*)NEW_C_HEAP_ARRAY_RETURN_nullptr(u1,
>> offset_of(JvmtiCachedClassFileData, data) + stream_len, mtInternal);
>
> fix
Not sure agree about the new macro. Why not in capitals, if it needs to be
updated (which I am not sure it has)?
> src/hotspot/share/jfr/writers/jfrEncoding.hpp line 33:
>
>> 31:
>> 32: enum JfrStringEncoding {
>> 33: nullptr_STRING = 0,
>
> fix
This is a conceptual "NULL" more than a specific C++ nullptr.
-
PR: https://git.openjdk.org/jdk/pull/12034
