RFR: 8351142: Add JFR monitor deflation and statistics events [v2]

[Aleksey Shipilev]

On Tue, 4 Mar 2025 15:26:45 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> Aleksey Shipilev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8351142-jfr-deflate-event
>>  - Test updates
>>  - Rework statistics event to be actually statistics
>>  - Filter JFR HiddenWait consistently
>>  - Event metadata touchups
>>  - Separate statistics event as well
>>  - Fix
>
> src/hotspot/share/jfr/metadata/metadata.xml line 124:
> 
>> 122:   </Event>
>> 123: 
>> 124:   <Event name="JavaMonitorStatistics" category="Java Application" label="Java Monitor Statistics" startTime="false">
> 
> For consistency with other statistical events, the category for JavaMonitorStatistics should be "Java Application, Statistics" 
> 
> The event should probably be periodic, so users can set an interval to reduce the number of events, with a default period of "everyChunk", so it is emitted at least at the beginning and end of a recording.

"Java Application, Statistics" is done.

Yeah, I think periodic task for this statistics event would be better. It would lose the direct information about the number of deflated monitors, but that could be somewhat inferred from the current monitor counts.

See new version.

> src/hotspot/share/jfr/metadata/metadata.xml line 125:
> 
>> 123: 
>> 124:   <Event name="JavaMonitorStatistics" category="Java Application" label="Java Monitor Statistics" startTime="false">
>> 125:     <Field type="ulong" name="totalCount" label="Monitors In Use" description="Number of in-use monitors"/>
> 
> The label should be 'Monitor in Use' (lowercase 'i').
> 
> Here is the style guideline if you're wondering.
> https://docs.oracle.com/en/java/javase/21/docs/api/jdk.jfr/jdk/jfr/Label.html

Noted, thanks! Fixed in new version.

> test/jdk/jdk/jfr/event/runtime/TestJavaMonitorDeflateEvent.java line 82:
> 
>> 80:             waitThread.join();
>> 81:             // Let deflater thread run.
>> 82:             Thread.sleep(3000);
> 
> I see that you took code from the MonitorInflate test. It's a really old test. A RecordingStream would be a more suitable as you can avoid using Thread.sleep() and the TestThread. I don't think a file needs to be dumped if the events are printed to standard out. Something like this:
> 
> 
> String lockClassName = lock.getClass().getName();
> List<RecordedEvent> events = new CopyOnWriteArrayList<>();
> try (RecordingStream rs = new RecordingStream()) {
>    rs.enable(EVENT_NAME).withoutThreshold();
>    rs.onEvent(EVENT_NAME, e -> {
>         RecordedClass clazz = e.getType(FIELD_KLASS_NAME);
>         if (clazz.getName().equals(lockClassName)) {
>            rs.close();
>         }
>    });
>    rs.startAsync(); 
>    ...
>    synchronized (lock) {
>    ...
>    }
>    ...
>    rs.awaitTermination();
>    System.out.println(events);
>    RecordedEvent event = events.get(0);
>    Events.assertField(event, FIELD_ADDRESS).notEqual(0L);
> }

Thanks. I rewritten the test using this suggestion as the base!

> test/jdk/jdk/jfr/event/runtime/TestJavaMonitorStatisticsEvent.java line 60:
> 
>> 58:         Recording recording = new Recording();
>> 59:         recording.enable(EVENT_NAME).withThreshold(Duration.ofMillis(0));
>> 60:         final Lock lock = new Lock();
> 
> If the event is periodic, you can set:
> 
> `recording.enable(EVENT_NAME).with("period", "everyChunk");`
> 
> and use the following instead of isAnyFound:
> 
> List<RecordedEvent> events = Events.fromRecording(recording);
> Events.hasEvents(events);
> 
> There's no need to dump to failed.jfr. Events.fromRecording will create a file that can be inspected in case the test fails. try-with-resources would be nice to have.

Rewritten with `RecordingStream` as well, see new version.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23900#discussion_r1981270071
PR Review Comment: https://git.openjdk.org/jdk/pull/23900#discussion_r1981270158
PR Review Comment: https://git.openjdk.org/jdk/pull/23900#discussion_r1981271007
PR Review Comment: https://git.openjdk.org/jdk/pull/23900#discussion_r1981271507