JDK-8313251: Add NativeLibraryLoad event #15065

MBaesken · 2023-07-28T08:10:24Z

Add a NativeLibraryLoad event that provides us more detail about shared lib/dll loads. This gives a time stamp and success + error details of the load operation. It enhances the already existing information we get from the existing NativeLibrary event (that periodically samples the native modules of the jvm process).

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8313251: Add NativeLibraryLoad event (Enhancement - P3)

Reviewers

Johannes Bechberger (@parttimenerd - Committer) ⚠️ Review applies to da6b9861
Erik Gahlin (@egahlin - Reviewer) ⚠️ Review applies to 045f2832
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15065/head:pull/15065
$ git checkout pull/15065

Update a local copy of the PR:
$ git checkout pull/15065
$ git pull https://git.openjdk.org/jdk.git pull/15065/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15065

View PR using the GUI difftool:
$ git pr show -t 15065

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15065.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-28T08:11:28Z

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-07-28T08:13:15Z

@MBaesken The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-07-28T08:17:09Z

Webrevs

dholmes-ora · 2023-07-30T22:49:05Z

/label add hotspot-jfr

openjdk · 2023-07-30T22:49:38Z

@dholmes-ora
The hotspot-jfr label was successfully added.

test/jdk/jdk/jfr/event/runtime/TestNativeLibraryLoadEvent.java

MBaesken · 2023-08-01T13:02:42Z

Hi Johannes, thanks for the review !

May I get a second review please?

egahlin · 2023-08-01T17:23:17Z

Hi Johannes, thanks for the review !

May I get a second review please?

I can review, but I like replies to my comments first.

MBaesken · 2023-08-02T07:11:33Z

Hi Johannes, thanks for the review !
May I get a second review please?

I can review, but I like replies to my comments first.

Hi Erik , maybe I miss something? What comment(s) are you referring to ?

egahlin · 2023-07-28T13:04:21Z

src/hotspot/share/jfr/metadata/metadata.xml

@@ -939,6 +939,13 @@
    <Field type="ulong" contentType="address" name="topAddress" label="Top Address" description="Ending address of the module, if available" />
  </Event>

+  <Event name="NativeLibraryLoad" category="Java Virtual Machine, Runtime" label="Native Library Load Operation" thread="false" stackTrace="true" startTime="true"


I think "Native Library Load" is sufficient as an event label and "Success" for the success field.

I think "error" is sufficient for the error field. We could use "errorMessage", if you believe there are other error related information we may want to add in the future.

The "name" label could changed to "Name", similar to the NativeLibrary event,

Hi Erik, I will adjust. I will use errorMessage because on some platforms we have already coding to describe shared lib load error situations in more detail (see for example os_linux.cpp).

egahlin · 2023-07-28T13:06:02Z

src/hotspot/os/linux/os_linux.cpp

  } else {
    Events::log_dll_message(nullptr, "Loaded shared library %s", filename);
    log_info(os)("shared library load of %s was successful", filename);
+#if INCLUDE_JFR
+    event.set_success(true);
+    event.set_errorDescription("");


Use null if there is no error message

egahlin · 2023-07-28T13:09:56Z

src/hotspot/os/bsd/os_bsd.cpp

@@ -1008,11 +1023,22 @@ void * os::dll_load(const char *filename, char *ebuf, int ebuflen) {
  return os::get_default_process_handle();
 #else
  log_info(os)("attempting shared library load of %s", filename);
+
+#if INCLUDE_JFR


We typically skip INCLUDE_JFR for events and rely on empty stubs in the code generation. This to make the code more easy to read.

Hi Erik , I removed the JFR with configure and attempted this (however on Linux) but for some reason I got build errors. So I better added the macros. (not sure why this happened, with some other JFR event living without INCLUDE_JFR seemed to work as expected)

Just for completeness the various os_xxx.cpp files all use INCLUDE_JFR around event code.

egahlin · 2023-07-28T13:11:35Z

test/jdk/jdk/jfr/event/runtime/TestNativeLibraryLoadEvent.java

+    private final static String EVENT_NAME = EventNames.NativeLibraryLoad;
+
+    public static void main(String[] args) throws Throwable {
+        Recording recording = new Recording();


Use try-with-resources.

Hi Erik, this was taken from test/jdk/jdk/jfr/event/runtime/TestNativeLibrariesEvent.java , I do not see in main there a try with resources. What do you want to put in the try block exactly ? The new Recording() ?

Yes.
try (Recording r = new Recording()) {
...
}
It's not used everywhere, but it seems proper to release chunk resources when the test is finished.

(I should probably go over all tests and change to avoid the pattern being copied.)

HI Erik, I adjusted the test as recommended .

egahlin · 2023-08-02T08:18:53Z

Hi Johannes, thanks for the review !
May I get a second review please?

I can review, but I like replies to my comments first.

Hi Erik , maybe I miss something? What comment(s) are you referring to ?

Hi Matthias. Sorry, I forgot to press submit.

openjdk · 2023-08-02T14:05:51Z

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8313251: Add NativeLibraryLoad event

Reviewed-by: jbechberger, egahlin, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 69 new commits pushed to the master branch:

10a2605: 8294979: test/jdk/tools/jlink 3 test classes use ASM library
e8c325d: 8313394: Array Elements in OldObjectSample event has the incorrect description
d60352e: 8311006: missing @SInCE info in jdk.xml.dom
4577147: 8313712: [BACKOUT] 8313632: ciEnv::dump_replay_data use fclose
bb3aac6: 8301606: JFileChooser file chooser details view "size" label cut off in Metal Look&Feel
0f2fce7: 8313632: ciEnv::dump_replay_data use fclose
ab1c212: 8312909: C1 should not inline through interface calls with non-subtype receiver
c386091: 8312984: javac may crash on a record pattern with too few components
3212b64: 8313582: Problemlist failing test on linux x86
bdac348: 8313602: increase timeout for jdk/classfile/CorpusTest.java
... and 59 more: https://git.openjdk.org/jdk/compare/ba645da97b00a7cc9d5a9d4dd58b1cd6737b4822...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

MBaesken · 2023-08-02T14:30:21Z

Hi Erik, thanks for the review !

dholmes-ora

Overall looks okay but I have a request for the test. Thanks.

dholmes-ora · 2023-08-03T07:11:10Z

src/hotspot/os/bsd/os_bsd.cpp

@@ -1008,11 +1023,22 @@ void * os::dll_load(const char *filename, char *ebuf, int ebuflen) {
  return os::get_default_process_handle();
 #else
  log_info(os)("attempting shared library load of %s", filename);
+
+#if INCLUDE_JFR


Just for completeness the various os_xxx.cpp files all use INCLUDE_JFR around event code.

dholmes-ora · 2023-08-03T07:16:40Z

test/jdk/jdk/jfr/event/runtime/TestNativeLibraryLoadEvent.java

+        if (Platform.isWindows()) {
+            libTemplate = "%s.dll";
+        } else if (Platform.isOSX()) {
+            libTemplate = "lib%s.dylib";


We have Platform.sharedLibraryExt() to simplify this a bit. Unfortunately we don't have a sharedLibraryPrefix() to reduce it to one line. :(

Hi David, that coding can indeed be improved. But let's do it in a follow-up. And in the follow-up also for existing https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/jfr/event/runtime/TestNativeLibrariesEvent.java where the same/similar code is present. The follow-up should also introduce 'sharedLibraryPrefix' .

I created https://bugs.openjdk.org/browse/JDK-8313670 .

dholmes-ora · 2023-08-03T07:20:17Z

test/jdk/jdk/jfr/event/runtime/TestNativeLibraryLoadEvent.java

+        try (Recording recording = new Recording()) {
+            recording.enable(EVENT_NAME);
+            recording.start();
+            System.loadLibrary("awt");


I don't think loading libawt is a good choice - maybe libnet or libnio? Headless builds may not have libawt and I'd be concerned about loading it directly and running the OnLoad hooks.

libnet does not work, it gives us the error 'Native Library /testdir/images/jdk/lib/libnet.so already loaded in another classloader' . So we have to load something that is not there already. But I agree, awt might not be the best choice. What about libinstrument ?

Yeah that might work. If worse comes to worst, you could create a trivial library just or the test.

MBaesken · 2023-08-03T11:28:06Z

David are you fine with the latest version?
I checked the GHA build errors and they are not related.

dholmes-ora

Looks okay. Thanks

MBaesken · 2023-08-04T07:02:35Z

Hi David , thanks for the review !

/integrate

openjdk · 2023-08-04T07:03:28Z

Going to push as commit 5d23295.
Since your change was applied there have been 70 commits pushed to the master branch:

c4b8574: 8311938: Add default cups include location for configure on AIX
10a2605: 8294979: test/jdk/tools/jlink 3 test classes use ASM library
e8c325d: 8313394: Array Elements in OldObjectSample event has the incorrect description
d60352e: 8311006: missing @SInCE info in jdk.xml.dom
4577147: 8313712: [BACKOUT] 8313632: ciEnv::dump_replay_data use fclose
bb3aac6: 8301606: JFileChooser file chooser details view "size" label cut off in Metal Look&Feel
0f2fce7: 8313632: ciEnv::dump_replay_data use fclose
ab1c212: 8312909: C1 should not inline through interface calls with non-subtype receiver
c386091: 8312984: javac may crash on a record pattern with too few components
3212b64: 8313582: Problemlist failing test on linux x86
... and 60 more: https://git.openjdk.org/jdk/compare/ba645da97b00a7cc9d5a9d4dd58b1cd6737b4822...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-04T07:03:34Z

@MBaesken Pushed as commit 5d23295.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8313251

eb87bf2

openjdk bot added the rfr Pull request is ready for review label Jul 28, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jul 28, 2023

add macro guards because the build errors in zero build

a934d3f

openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Jul 30, 2023

dholmes-ora reviewed Jul 30, 2023

View reviewed changes

test/jdk/jdk/jfr/event/runtime/TestNativeLibraryLoadEvent.java Outdated Show resolved Hide resolved

Adjust COPYRIGHT year info

da6b986

parttimenerd approved these changes Aug 1, 2023

View reviewed changes

egahlin reviewed Aug 2, 2023

View reviewed changes

MBaesken added 2 commits August 2, 2023 14:33

Small event adjustments and use nullptr in case we have no message

628336c

use try with resources in test

045f283

egahlin approved these changes Aug 2, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 2, 2023

dholmes-ora suggested changes Aug 3, 2023

View reviewed changes

better check for libinstrument not awt

40263d1

dholmes-ora approved these changes Aug 4, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 4, 2023

openjdk bot closed this Aug 4, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 4, 2023

JDK-8313251: Add NativeLibraryLoad event #15065

JDK-8313251: Add NativeLibraryLoad event #15065

Conversation

MBaesken commented Jul 28, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jul 28, 2023

openjdk bot commented Jul 28, 2023

mlbridge bot commented Jul 28, 2023 • edited Loading

Webrevs

dholmes-ora commented Jul 30, 2023

openjdk bot commented Jul 30, 2023

MBaesken commented Aug 1, 2023

egahlin commented Aug 1, 2023

MBaesken commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egahlin commented Aug 2, 2023

openjdk bot commented Aug 2, 2023 • edited Loading

MBaesken commented Aug 2, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MBaesken commented Aug 3, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

MBaesken commented Aug 4, 2023

openjdk bot commented Aug 4, 2023

openjdk bot commented Aug 4, 2023

MBaesken commented Jul 28, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 28, 2023 •

edited

Loading

openjdk bot commented Aug 2, 2023 •

edited

Loading