July 2024 - hotspot-jfr-dev

RFR: 8279016: JFR Leak Profiler is broken with Shenandoah
by Aleksey Shipilev 06 Nov '24

06 Nov '24

While testing unrelated Shenandoah patch, I caught a GC assert when Leak Profiler was running ([JDK-8337194](https://bugs.openjdk.org/browse/JDK-8337194)) Leak Profiler is notorious in using the mark words for its own needs. We have been trying to mitigate its impact on GCs by moving to separate bitsets for tracking marked objects, or by treating "marked without fwdptr" as "JFR marked" and handling it. But this is not reliable, since things like [putting indexes in mark word](https://github.com/openjdk/jdk/blob/3baff2af6a30cc6cb2e0d4391db1cf7e6… sneak in. This is okay for Leak Profiler alone, since it restores the mark words after the operation completes, but that is still not enough when GC is already running. I say we side-step this whack-a-mole by cleanly bailing from JFR op, when we know it is unsafe to do. I thought to use `VM_Operation::doit_prologue`, but I think GC start may sneak in between checking in prologue and op start. This realistically only affects Shenandoah. All other STW collectors would never see what Leak Profiler did with mark words. ZGC would not see it, since it does not care about mark words for its own operation. Additional testing: - [x] `jdk/jfr/event/oldobject/` pass by default (100x times) - [x] `jdk/jfr/event/oldobject/` pass with `-XX:+UseShenandoah` (100x) ------------- Commit messages: - Fix Changes: https://git.openjdk.org/jdk/pull/20328/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20328&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8279016 Stats: 29 lines in 4 files changed: 29 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20328.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20328/head:pull/20328 PR: https://git.openjdk.org/jdk/pull/20328

7 46

RFR: 8337501: JFR: Use TimespanUnit
by Erik Gahlin 31 Jul '24

31 Jul '24

Could I have a review of change that updates the JFR code to use the newly introduced TimespanUnit enum. The enum is updated to use j.u.c.TimeUnit enum so the method TimeUnit::toNanos can be leveraged. It can handle over- and underflow properly. Path forward is to introduce a Timespan class, similar to the Rate class, that will prevent negative values, parse "infinity" and provide means to compare values. This will consolidate code further. Testing: test/jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Move initialization in constructor - Initial Changes: https://git.openjdk.org/jdk/pull/20392/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20392&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337501 Stats: 57 lines in 4 files changed: 10 ins; 22 del; 25 mod Patch: https://git.openjdk.org/jdk/pull/20392.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20392/head:pull/20392 PR: https://git.openjdk.org/jdk/pull/20392

2 2

Thoughts on JFR events for reflection usage?
by Robert Toyonaga 31 Jul '24

31 Jul '24

Hi all! What are your thoughts on an event to record uses of reflection? The event could contain a field for the action being taken (accessing a field, invoking a method, etc). It would be an instant event with a stacktrace. In the case of Quarkus, it's useful to know where and how often reflection is being used so that we know where to focus efforts if we want to reduce reflection usage (for performance reasons and also for compatibility with Native Image). This information is probably helpful in other use cases as well. This is a specific case of the more general problem of method tracing: https://bugs.openjdk.org/browse/JDK-8249859 One potential problem is that if a Java program uses a lot of reflection, the overhead of such an event might be large, if we emit an event for every single usage. To avoid that, this event could be throttled similar to how allocation sampling is done. This would be ok if we want a general picture of reflection usage. And if a user doesn't want to miss any unique accesses, they could set the rate to unlimited. Are there any other foreseeable problems with an event like this? Thanks! Robert Toyonaga

2 2

RFR: 8335907: JFR: Make SettingControls more robust
by Erik Gahlin 30 Jul '24

30 Jul '24

Could I have review of changes to the SettingControl implementations to make them more robust: - Consistently ignore invalid values for the methods `combine(Set)` and `setValue(String)`. - Consistently set the textual value field in the `SettingControl` after the actual value has been changed. - Consistently set the textual value field to the default value in the constructor. A test was added to ensure that current and future setting controls are properly tested. This also improves test coverage. See [JDK-8309052](https://bugs.openjdk.org/browse/JDK-8309052) **Implementation comments:** - A `jdk.jfr.internal.util.Rate` class was added to make the code more readable. - The `ValueParser$TimespanUnit` class was made a top-level class for unit handling. - A `Utils::multiplyOverflow(long, long, long)` method was added to avoid negative values in case of an overflow. - `String::strip` was used instead of `String::trim` to make the implementation Unicode aware. - The cutoff setting in the `EventControl` class was changed to “infinity”, which in practice was already the default value. Composition, which is typically preferable over inheritance, was previously used for the `EnabledSetting` and `StackTraceSetting` classes. However, the code became hard to read and error-prone, so there is now an abstract `BooleanSetting` class to enforce consistency. **Testing:** - test/jdk/jdk/jfr/* - 100 * test/jdk/jdk/jfr/api/setting/TestSettingControl.java **Out of scope:** - Formatting issues - Wider use of the TimespanUnit enum - Clarify the SettingControl specification on how invalid values should be handled ------------- Commit messages: - Minor update - Initial Changes: https://git.openjdk.org/jdk/pull/20336/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335907 Stats: 911 lines in 17 files changed: 573 ins; 264 del; 74 mod Patch: https://git.openjdk.org/jdk/pull/20336.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20336/head:pull/20336 PR: https://git.openjdk.org/jdk/pull/20336

2 4

RFR: 8336316: JFR: Use SettingControl::getValue() instead of setValue() for ActiveSetting event
by Erik Gahlin 24 Jul '24

24 Jul '24

Could I have a review of a change that makes the ActiveSetting event act differently when an invalid value is specified. Previously the last set value was used, but now the result of getValue() is returned. In case Control::getLastValue() returns null, the default value is used. Benefit of this change is to make it easier to diagnose setting related issues. The event will reports the actual state of the event setting, not what happens to have been specified. Testing: jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/20172/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20172&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336316 Stats: 7 lines in 1 file changed: 3 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20172.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20172/head:pull/20172 PR: https://git.openjdk.org/jdk/pull/20172

2 2

RFR: 8336485: jdk/jfr/jcmd/TestJcmdView.java RuntimeException: 'Invoked Concurrent' missing from stdout/stderr
by Erik Gahlin 24 Jul '24

24 Jul '24

Could I have a review of a test fix. Testing: 100 * jdk/jdk/jfr/jcmd/TestJcmdView.java Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/20278/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20278&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8336485 Stats: 15 lines in 1 file changed: 10 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/20278.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20278/head:pull/20278 PR: https://git.openjdk.org/jdk/pull/20278

2 2

[jdk23] RFR: 8325280: Update troff manpages in JDK 23 before RC
by David Holmes 21 Jul '24

21 Jul '24

Before RC we need to update the man pages in the repo from their Markdown sources. All pages at a minimum have 23-ea replaced with 23, and publication year set to 2024 if needed. This also picks up the unpublished changes to java.1 from: - [JDK-8324836](https://bugs.openjdk.org/browse/JDK-8324836): Update Manpage for obsoletion of RAMFraction flags - [JDK-8330807](https://bugs.openjdk.org/browse/JDK-8330807): Update Manpage for obsoletion of ScavengeBeforeFullGC And a typo crept in to java.1 from: - [JDK-8331670](https://bugs.openjdk.org/browse/JDK-8331670): Deprecate the Memory-Access Methods in sun.misc.Unsafe for Removal This also picks up the unpublished change to keytool.1 from: - [JDK-8284500](https://bugs.openjdk.org/browse/JDK-8284500): Typo in Supported Named Extensions - BasicContraints This also picks up the unpublished change to javadoc.1 from: - [JDK-8324342](https://bugs.openjdk.org/browse/JDK-8324342): Doclet should default @since for a nested class to that of its enclosing class and some formatting changes (unclear why - perhaps pandoc version) from the combined changeset for: - [JDK-8298405](https://bugs.openjdk.org/browse/JDK-8298405): Implement JEP 467: Markdown Documentation Comments - [JDK-8329296](https://bugs.openjdk.org/browse/JDK-8329296): Update Elements for '///' documentation comments The javac.1 file has its copyright year reverted to match what is in the source file (which should have been updated but wasn't). Thanks. ------------- Commit messages: - 8325280: Update troff manpages in JDK 23 before RC Changes: https://git.openjdk.org/jdk/pull/20248/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20248&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325280 Stats: 142 lines in 28 files changed: 51 ins; 52 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/20248.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20248/head:pull/20248 PR: https://git.openjdk.org/jdk/pull/20248

3 5

[jdk23] RFR: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
by Markus Grönlund 17 Jul '24

17 Jul '24

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

2 2

Integrated: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
by Markus Grönlund 17 Jul '24

17 Jul '24

On Sat, 13 Jul 2024 14:47:21 GMT, Markus Grönlund <mgronlun(a)openjdk.org> 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 <mgronlun(a)openjdk.org> 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

1 0

Re: RFR: 8334781: JFR crash: assert(((((JfrTraceIdBits::load(klass)) & ((JfrTraceIdEpoch::this_epoch_method_and_class_bits()))) != 0))) failed: invariant
by Serguei Spitsyn 16 Jul '24

16 Jul '24

On Sat, 13 Jul 2024 14:47:21 GMT, Markus Grönlund <mgronlun(a)openjdk.org> 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? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20171#discussion_r1678533638

2 1