On Tue, 10 Nov 2026 17:39:03 GMT, Erik Gahlin <[email protected]> wrote:
> Could I have a review of a PR that fixes several issues with the JFR > exception events: > > - Error and its subclasses emitted two JavaExceptionThrow events > - Error was counted twice in ExceptionStatistics > - Constructors that only take the cause lost the message > > > The testing was inadequate, so I rewrote most of it. > > Testing: jdk_lang, jdk_jfr > > Thanks > Erik > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Update to Exception/Error look okay. The this-escape is unfortunate but it avoids duplicating the null check in the super class. Just a few minor comments/questions on the test re-implementation. test/jdk/jdk/jfr/event/runtime/TestExceptionEvents.java line 41: > 39: /* > 40: * @test > 41: * @bug 8397069 I assume this should list 8199712 or whatever JBS issue motivated the test originally so that it doesn't look like 8397069 is the motivation for the test. test/jdk/jdk/jfr/event/runtime/TestExceptionEvents.java line 47: > 45: * @run main/othervm jdk.jfr.event.runtime.TestExceptionEvents > 46: */ > 47: @SuppressWarnings("serial") Is this because serialVersionUID has been removed from the nested classes/exceptions? test/jdk/jdk/jfr/event/runtime/TestExceptionEvents.java line 53: > 51: private static final String EXCEPTION_STATISTICS_NAME = > EventNames.ExceptionStatistics; > 52: private static final String THREAD_NAME = > Thread.currentThread().getName(); > 53: private static final int THROWN_EXCEPTION = 5000; I assume this should THROWN_EXCEPTIONS. test/jdk/jdk/jfr/event/runtime/TestExceptionEvents.java line 83: > 81: try (Recording r = new Recording()) { > 82: r.enable(ERROR_EVENT_NAME).withStackTrace(); > 83: r.enable(EXCEPTION_STATISTICS_NAME).with("period", > "everyChunk"); I'm hazy on the "everyChunk" period so I think it means beginning and end of each chunk. Does this have any interaction with higher tiers run that enable JFR? Different recording of course but I wondering chunk rotation could cause interference. test/jdk/jdk/jfr/event/runtime/TestExceptionEvents.java line 243: > 241: } > 242: if (sampled < 2) { > 243: throw new Exception("Expected at least two event to be > sampled event"); "at least two events" test/jdk/jdk/jfr/event/runtime/TestExceptionEvents.java line 301: > 299: RecordedEvent end = statisticsEvents.get(1); > 300: long count = end.getLong("throwables") - > begin.getLong("throwables"); > 301: Asserts.assertGreaterThanOrEqual(count, expectedCount, "Too few > exception count in statistics event"); Another typo here, should be "Too few exceptions". ------------- PR Review: https://git.openjdk.org/jdk/pull/33828#pullrequestreview-5872108639 PR Review Comment: https://git.openjdk.org/jdk/pull/33828#discussion_r4548623694 PR Review Comment: https://git.openjdk.org/jdk/pull/33828#discussion_r4548630433 PR Review Comment: https://git.openjdk.org/jdk/pull/33828#discussion_r4548732465 PR Review Comment: https://git.openjdk.org/jdk/pull/33828#discussion_r4548713566 PR Review Comment: https://git.openjdk.org/jdk/pull/33828#discussion_r4548720519 PR Review Comment: https://git.openjdk.org/jdk/pull/33828#discussion_r4548724953
