October 2022 - hotspot-jfr-dev

Integrated: 8295274: HelidonAppTest.java fails "assert(event->should_commit()) failed: invariant" from compiled frame"
by Markus Grönlund 14 Oct '22

14 Oct '22

On Thu, 13 Oct 2022 14:38:59 GMT, Markus Grönlund <mgronlun(a)openjdk.org> wrote: > Greetings, > > In [JDK-8287832](https://bugs.openjdk.org/browse/JDK-8287832), a change was made that removed the cached shouldCommit() state, under the premise that shouldCommit() is only called once. There are a few places that assert on shouldCommit(), that should have been removed as part of [JDK-8287832](https://bugs.openjdk.org/browse/JDK-8287832). > > This change complements these removals. > > Testing: jdk_jfr > > Thanks > Markus This pull request has now been integrated. Changeset: 21e4f06a Author: Markus Grönlund <mgronlun(a)openjdk.org> URL: https://git.openjdk.org/jdk/commit/21e4f06ada24098dad4e71b0f9c13afeff87c24b Stats: 4 lines in 4 files changed: 0 ins; 4 del; 0 mod 8295274: HelidonAppTest.java fails "assert(event->should_commit()) failed: invariant" from compiled frame" Reviewed-by: egahlin ------------- PR: https://git.openjdk.org/jdk/pull/10700

1 0

Re: RFR: 8295274: HelidonAppTest.java fails "assert(event->should_commit()) failed: invariant" from compiled frame"
by Erik Gahlin 14 Oct '22

14 Oct '22

On Thu, 13 Oct 2022 14:38:59 GMT, Markus Grönlund <mgronlun(a)openjdk.org> wrote: > Greetings, > > In [JDK-8287832](https://bugs.openjdk.org/browse/JDK-8287832), a change was made that removed the cached shouldCommit() state, under the premise that shouldCommit() is only called once. There are a few places that assert on shouldCommit(), that should have been removed as part of [JDK-8287832](https://bugs.openjdk.org/browse/JDK-8287832). > > This change complements these removals. > > Testing: jdk_jfr > > Thanks > Markus Marked as reviewed by egahlin (Reviewer). ------------- PR: https://git.openjdk.org/jdk/pull/10700

1 0

RFR: 8295223 JFR: At most one native periodic event thread at a time
by Erik Gahlin 13 Oct '22

13 Oct '22

Hi, Could I have a review of PR that ensures only one Java thread calls into native to emit periodic events at a time. Testing: tier1-tier3, test/jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Remove accidentally added file - Initial Changes: https://git.openjdk.org/jdk/pull/10676/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10676&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8295223 Stats: 21 lines in 2 files changed: 18 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10676.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10676/head:pull/10676 PR: https://git.openjdk.org/jdk/pull/10676

2 2

RFR: 8263044: jdk/jfr/jvm/TestDumpOnCrash.java timed out
by Erik Gahlin 12 Oct '22

12 Oct '22

Hi, When a crash/emergency dump is created, it is sometimes not complete. This is OK as there is no guarantee that a dump will succeed when the JVM crashes. Problem occurs if this happens before the first metadata event has been written (metadataPosition == 0) in which case the parser will loop indefinitely. This is expected behavior in the streaming case (EventStream), but if the recording was opened with RecordingFile it is usually able to finish or throw an exception. The fix is to invoke pollWait() if a valid recording only consists of the header (68 bytes). This will result in similar behaviour as if the recording was terminated prematurely, but the metadata event had been written. Testing: jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/10629/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10629&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8263044 Stats: 12 lines in 1 file changed: 10 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/10629.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10629/head:pull/10629 PR: https://git.openjdk.org/jdk/pull/10629

2 2

RFR: 8291519: jdk/jfr/api/event/TestShouldCommit.java failed with Unexpected value of shouldCommit()
by Erik Gahlin 11 Oct '22

11 Oct '22

Hi, Could I have review of a test fix, which is a complete rewrite. Instead of verifying the clock, it checks the logic of the shouldCommit method. Testing: 100 * test/jdk/jdk/jfr/api/event/TestShouldCommit.java Maybe there should be a test that checks that an event timestamp doesn't drift too much from the system clock, but I think there are better ways to check that than using the shouldCommit() method. Thanks Erik ------------- Commit messages: - Add asserts - Initial Changes: https://git.openjdk.org/jdk/pull/10611/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10611&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8291519 Stats: 178 lines in 1 file changed: 54 ins; 53 del; 71 mod Patch: https://git.openjdk.org/jdk/pull/10611.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10611/head:pull/10611 PR: https://git.openjdk.org/jdk/pull/10611

2 2

RFR: 8294931 JFR: Simplify SettingInfo
by Erik Gahlin 10 Oct '22

10 Oct '22

Hi, Could Have a review of a PR that simplifies code related to SettingInfo. Testing: jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/10599/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10599&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294931 Stats: 43 lines in 4 files changed: 7 ins; 21 del; 15 mod Patch: https://git.openjdk.org/jdk/pull/10599.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10599/head:pull/10599 PR: https://git.openjdk.org/jdk/pull/10599

2 3

RFR: 8287832: jdk/jfr/event/runtime/TestActiveSettingEvent.java failed with "Expected two batches of Active Setting events"
by Markus Grönlund 10 Oct '22

10 Oct '22

Greetings, The test fails because of an unexpected event in the event stream. The recording does not explicitly enable any events, yet, occasionally, there are some. From where do these events originate? The JfrEvent C++ classes check enabled state in the constructor, and some event instances have extent across a potential safepoint. A safepoint could result from JFR rotating and stopping to record. As a result, an event can still be enabled after JFR stops and then restarts, resulting in unexpected events in the event stream of the newly started recording. Most common unexpected events seen are: jdk.JavaMonitorWait jdk.Compilation jdk.NativeMethodSample A solution to this problem is to move the event-enabled check from the constructor to the shouldCommit() and commit() functions. For durational events, it requires two is_enabled() tests, as one will still be needed in the constructor, for decision about starttime. No tangible performance impact is expected. Testing: jdk_jfr, stress testing Thanks Markus ------------- Commit messages: - 8287832 Changes: https://git.openjdk.org/jdk/pull/10575/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10575&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8287832 Stats: 31 lines in 4 files changed: 8 ins; 11 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/10575.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10575/head:pull/10575 PR: https://git.openjdk.org/jdk/pull/10575

2 6

RFR: 8293864: Kitchensink24HStress.java fails with SIGSEGV in JfrCheckpointManager::lease
by Markus Grönlund 10 Oct '22

10 Oct '22

Greetings, With [JDK-8289692](https://bugs.openjdk.org/browse/JDK-8289692), a need arose to introduce a mutex mechanism for the buffers on the global list to prevent the JFR Recorder Thread from resetting buffers currently in use. The introduced mutex mechanism was adequate as far as preventing the race condition. However, its protective characteristic was overemphasized, leading to the mistake of introducing a release, or removal, operation onto a current epoch list, as part of periodic flushing. The problem is that although the callback operations are safe, the underlying list type used with the global mspace does not allow for concurrent excision of nodes. The crash occurs because a thread attempts to dereference the next pointer to traverse a node that is excised and deleted after the thread loaded it. I have reviewed how to handle better the introduction of virtual threads and their checkpoint data. I have concluded that the best way is to preserve the old system (pre-Loom) as much as possible, keeping the global and thread-local mspaces as before. Instead, an additional mspace dedicated solely to virtual threads is introduced. This categorization preserves the matching of operations against the underlying list infrastructure. It also provides flexibility in handling virtual threads distinct from regular, or "carrier", threads. The separate mspace of virtual threads is more dynamic as it does not need to preallocate buffers, a memory win for systems not using virtual threads. Also, the sizes of the different buffer types can be better controlled. With this restructuring, there are no longer any list concurrency issues; hence the mutex mechanism introduced with [JDK-8289692](https://bugs.openjdk.org/browse/JDK-8289692) becomes obsolete. Applying a release operation for the current epoch list, of the global mspace, as part of flushing is a broken invariant and is therefore removed. Testing: jdk_jfr, stress testing Thanks Markus ------------- Commit messages: - 8293864 Changes: https://git.openjdk.org/jdk/pull/10467/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10467&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8293864 Stats: 217 lines in 9 files changed: 100 ins; 33 del; 84 mod Patch: https://git.openjdk.org/jdk/pull/10467.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10467/head:pull/10467 PR: https://git.openjdk.org/jdk/pull/10467

2 3

RFR: 8294151: JFR: Unclear exception message when dumping stopped in memory recording
by Erik Gahlin 05 Oct '22

05 Oct '22

Hi Could I have a review of a change that updates the specification for the Recording::dump(Path) method and the exception message if the method fails. Testing: jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/10488/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10488&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294151 Stats: 48 lines in 4 files changed: 38 ins; 1 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/10488.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10488/head:pull/10488 PR: https://git.openjdk.org/jdk/pull/10488

2 2

Integrated: 8294673: JFR: Add SecurityProviderService#threshold to TestActiveSettingEvent.java
by Erik Gahlin 04 Oct '22

04 Oct '22

Hi, Could I have review of a fix that adds "threshold" to TestActiveSettingEvent.java. See explanation in testSettingConfiguration(...) why it is needed. Thanks Erik ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/10511/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10511&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294673 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10511.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10511/head:pull/10511 PR: https://git.openjdk.org/jdk/pull/10511

2 2