April 2024 - hotspot-jfr-dev

Re: RFR: 8329995: Restricted access to `/proc` can cause JFR initialization to crash [v2]
by Jaroslav Bachorik 23 Apr '24

23 Apr '24

> Please, review this trivial change to make using `ProcessIterator` more robust in the presence of SELinux or a similar system. > > The call to `os::opendir("/proc")` may return `nulltptr` if the `/proc` is not accessible due to restrictions placed by the SELinux. In that case the `ProcessIterator` will SIGSEG because it assumes the `_dir`, which is the variable storing the result of the `os::opendir("/proc")` call to be non-null. > > The patch is missing regression test because it is very hard to simulate `/proc` not being accessible to the test process. Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision: Add explanatory comment ------------- Changes: - all: https://git.openjdk.org/jdk/pull/18775/files - new: https://git.openjdk.org/jdk/pull/18775/files/cf7eb4aa..d114a437 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18775&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18775&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18775.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18775/head:pull/18775 PR: https://git.openjdk.org/jdk/pull/18775

3 3

Re: RFR: 8329995: Restricted access to `/proc` can cause JFR initialization to crash
by David Holmes 19 Apr '24

19 Apr '24

On Sun, 14 Apr 2024 08:53:05 GMT, Jaroslav Bachorik <jbachorik(a)openjdk.org> wrote: > Please, review this trivial change to make using `ProcessIterator` more robust in the presence of SELinux or a similar system. > > The call to `os::opendir("/proc")` may return `nulltptr` if the `/proc` is not accessible due to restrictions placed by the SELinux. In that case the `ProcessIterator` will SIGSEG because it assumes the `_dir`, which is the variable storing the result of the `os::opendir("/proc")` call to be non-null. > > The patch is missing regression test because it is very hard to simulate `/proc` not being accessible to the test process. This change doesn't seem sufficient as many of the ProcessIterator functions access something under /proc without checking is_valid. Is it just direct access to /proc that fails rather than specific entries? src/hotspot/os/linux/os_perf_linux.cpp line 851: > 849: _entry = nullptr; > 850: _valid = _dir != nullptr; > 851: next_process(); A comment on the opendir would be useful e.g. _dir = os::opendir("/proc"); // May be null if /proc is not accessible. ------------- PR Review: https://git.openjdk.org/jdk/pull/18775#pullrequestreview-2000373079 PR Review Comment: https://git.openjdk.org/jdk/pull/18775#discussion_r1565398444

2 2

RFR: 8329330: NoClassDefFoundError: Could not initialize class jdk.jfr.internal.MirrorEvents
by Erik Gahlin 19 Apr '24

19 Apr '24

Could I have a review of PR that remove mirror events from initialisation. Testing: jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/18843/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18843&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329330 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18843.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18843/head:pull/18843 PR: https://git.openjdk.org/jdk/pull/18843

2 2

RFR: 8330533: JFR: LocalDateTime should not use milliseconds since epoch
by Erik Gahlin 18 Apr '24

18 Apr '24

Could I have a review of a bug fix that changes the parameter passed to LocalDateTime to an Instant. Testing: jdk/jdk/jfr + manual inspection of -Xlog:jfr to ensure filenames use UTC when an incorrect time zone is used. Thanks Erik ------------- Commit messages: - Use Instant.now() - Initial Changes: https://git.openjdk.org/jdk/pull/18827/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18827&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330533 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18827.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18827/head:pull/18827 PR: https://git.openjdk.org/jdk/pull/18827

3 5

RFR: 8330053: JFR: Use LocalDateTime instead ZonedDateTime
by Erik Gahlin 17 Apr '24

17 Apr '24

Could I have a review of PR that changes so LocalDateTime instead of ZonedDateTime is used when naming chunk files. Problem is that a java.time.DateTimeException occurs when ZonedDateTime.now() is used and the time zone is invalid. This happens in a JCK test. It would be possible to catch the exception and fallback to LocalDateTime, but it seems better to have filenames that can't be manipulated if an application calls TimeZone::setDefault. That said, if there is a good argument to use a fallback and not always the default time zone, please let me know. Testing: jdk/jdk/jfr + applicable test in JCK. Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/18729/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18729&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330053 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18729.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18729/head:pull/18729 PR: https://git.openjdk.org/jdk/pull/18729

4 9

RFR: 8326116: JFR: Add help option to -XX:StartFlightRecording
by Erik Gahlin 16 Apr '24

16 Apr '24

Could I have review of an enhancement that adds a help option to -XX:StartFlightRecording similar to -Xlog:help. Testing: jdk/jdk/jfr + tier1 + tier2 Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/18765/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18765&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326116 Stats: 129 lines in 5 files changed: 106 ins; 0 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/18765.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18765/head:pull/18765 PR: https://git.openjdk.org/jdk/pull/18765

2 2

RFR: 8330215: Trim working set for OldObjectSamples
by Markus Grönlund 16 Apr '24

16 Apr '24

Greetings, During the investigation around a potential memory leak, initially thought to be related to JFR and OldObjectSamples, but eventually reduced to https://bugs.openjdk.org/browse/JDK-8303767, a few minor improvements were made. There is no need to save class unload typesets if there are no new unresolved entries for the epoch in the priority queue. Testing: jdk_jfr, stress testing Thanks Markus ------------- Commit messages: - 8330215 Changes: https://git.openjdk.org/jdk/pull/18776/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18776&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330215 Stats: 66 lines in 4 files changed: 51 ins; 2 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/18776.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18776/head:pull/18776 PR: https://git.openjdk.org/jdk/pull/18776

3 4

RFR: 8330064: JFR: Incorrect function declarations for register/unregister_stack_filter
by David Holmes 15 Apr '24

15 Apr '24

The commit from [JDK-8314745](https://bugs.openjdk.org/browse/JDK-8314745) contains jlong JNICALL jfr_register_stack_filter(JNIEnv* env, jobject classes, jobject methods); jlong JNICALL jfr_unregister_stack_filter(JNIEnv* env, jlong start_filter_id); in `jfrJniMethod.hpp`, but the definitions have different signatures: JVM_ENTRY_NO_ENV(jlong, jfr_register_stack_filter(JNIEnv* env, jclass jvm, jobjectArray classes, jobjectArray methods)) return JfrStackFilterRegistry::add(classes, methods, thread); JVM_END JVM_ENTRY_NO_ENV(void, jfr_unregister_stack_filter(JNIEnv* env, jclass jvm, jlong id)) JfrStackFilterRegistry::remove(id); JVM_END This fixes the declarations to match the definitions. The issue was detected attempting a 32-bit Windows built. Unclear why our main toolchains have not complained about this. Testing: - tiers 1-3 (sanity) - GHA ------------- Commit messages: - 8330064: JFR: Incorrect function declarations for register/unregister_stack_filter Changes: https://git.openjdk.org/jdk/pull/18733/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18733&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330064 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18733.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18733/head:pull/18733 PR: https://git.openjdk.org/jdk/pull/18733

5 7

RFR: JDK-8327474 Review use of java.io.tmpdir in jdk tests
by Bill Huang 03 Apr '24

03 Apr '24

This task addresses an essential aspect of our testing infrastructure: the proper handling and cleanup of temporary files and socket files created during test execution. The motivation behind these changes is to prevent the accumulation of unnecessary files in the default temporary directory, which can affect the system's storage and potentially influence subsequent test runs. Our review identified that several tests create temporary files or socket files without ensuring their removal post-execution. - Direct calls to java.io.File.createTempFile and java.nio.file.Files.createTempFile without adequate cleanup. - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not explicitly removing socket files post-use. ------------- Commit messages: - Clean up temporary files after tests complete. Changes: https://git.openjdk.org/jdk/pull/18352/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18352&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327474 Stats: 42 lines in 11 files changed: 20 ins; 1 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/18352.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18352/head:pull/18352 PR: https://git.openjdk.org/jdk/pull/18352

7 18