Draft: Test/shen oc #26376

ysramakrishna · 2025-07-18T01:33:55Z

This is a draft created for the purpose of looking at diffs and providing comments. Please ignore.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26376/head:pull/26376
$ git checkout pull/26376

Update a local copy of the PR:
$ git checkout pull/26376
$ git pull https://git.openjdk.org/jdk.git pull/26376/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26376

View PR using the GUI difftool:
$ git pr show -t 26376

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26376.diff

…jectCountEvent

…shen-oc

bridgekeeper · 2025-07-18T01:34:32Z

👋 Welcome back ysr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-07-18T01:34:52Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-07-18T01:35:10Z

⚠️ @ysramakrishna This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2025-07-18T01:35:23Z

@ysramakrishna The following labels will be automatically applied to this pull request:

hotspot
shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

ysramakrishna

Looks mostly good.

Left some comments about whitespace etc.
Other than that, please track down and fix the issue with the incorrect assert definition being caught in the macos and windows builds. This typically happens because the old code did not have an explicit #include. The simplest way to avoid this is to explicitly include the .hpp in which assert() is defined into the file where you are capturing the wrong assert macro definition. Contact me tomorrow if you find yourself unable to track this down and correct it as suggested.

Once the builds are clean, I'd go over to doing the semi-efficient implementation of the event gathering for Shenandoah as discussed -- by merging the gathering of the class histogram information with the marking. (Note that this will complete the work for the ObjectCountAfterGC implementation.) Let's get that done first, and reviewed by me in the same way, via a draft PR. Thanks!

ysramakrishna · 2025-07-18T02:01:51Z

src/hotspot/share/gc/shared/gcTrace.cpp

+  }
+}
+
+void GCTracer::report_object_count_after_gc(BoolObjectClosure* is_alive_cl, WorkerThreads* workers) {


It seems like the only difference between these two methods is the event-sender type passed in the closure. You could make this a template method and pass the event-sender type as a parameter to the template method.

In this case, you want to not do the class info table gathering here all at one point, but as you mark objects. Then once the marking is completed, you would send the events by iterating over the table after marking is completed, but you want to do that outside of a safepoint (after the remark, for the case of Shenandoah).

I'll remove the other report_object_count method. I just kept it there for right now just to test the functionality of ObjectCount in Shenandoah. I'll change the report_object_count_after_gc method (might rename it, since it sounds misleading when the ObjectCount event calls this method) to be a template method.

you want to not do the class info table gathering here all at one point, but as you mark objects

I'll create a closure that does the class info table as I mark objects like discussed in yesterday's meeting.

you want to do that outside of a safepoint

I'll make sure to send events concurrently with the application.

ysramakrishna · 2025-07-18T02:09:20Z

src/hotspot/share/gc/shared/objectCountEventSenderTemplate.hpp

+
+  send_event_if_enabled<Event>(klass, count, total_size, timestamp);
+  // If sending ObjectCountAfterGCEvent, check if ObjectCount is enabled and send event data to ObjectCount
+  if (std::is_same<Event, EventObjectCountAfterGC>::value && ObjectCountEventSender::should_send_event()) { 


This is kind of clever. Also state that if this method is called specifically for ObjectCount then this avoids sending two events for each class entry.

Also state that if this method is called specifically for ObjectCount then this avoids sending two events for each class entry

So I should just add an extra comment for this?

send_event_if_enabled<Event>(klass, count, total_size, timestamp); // If sending ObjectCountAfterGCEvent, check if ObjectCount is enabled and send event data to ObjectCount // If sending ObjectCountEvent, do not send send ObjectCountAfterGCEvent if (std::is_same<Event, EventObjectCountAfterGC>::value && ObjectCountEventSender::should_send_event()) {

ysramakrishna · 2025-07-18T02:11:12Z

test/lib/jdk/test/lib/jfr/EventNames.java

@@ -197,7 +197,7 @@ public class EventNames {
    public static final String ResidentSetSize = PREFIX + "ResidentSetSize";

    // JDK
-    public static final String FileForce  = PREFIX + "FileForce";


Looks like just a whitespace change. The change is correct, but we avoid making whitespace only changes to files that don't otherwise have any other changes. I'd back this out.

ysramakrishna · 2025-07-18T02:12:59Z

test/lib/jdk/test/lib/jfr/GCHelper.java

+    // public static String getEventDesc(RecordedEvent event) {
+    // final String path = event.getEventType().getName();
+    // if (!isGcEvent(event)) {
+    // return path;
+    // }
+    // if (event_garbage_collection.equals(path)) {
+    // String name = Events.assertField(event, "name").getValue();
+    // String cause = Events.assertField(event, "cause").getValue();
+    // return String.format("path=%s, gcId=%d, endTime=%d, name=%s, cause=%s,
+    // startTime=%d",
+    // path, getGcId(event), event.getEndTime(), name, cause, event.getStartTime());
+    // } else {
+    // return String.format("path=%s, gcId=%d, endTime=%d", path, getGcId(event),
+    // event.getEndTime());
+    // }
+    // }


This looks whitespace only changes that are semantically no-ops. Back this out as it clutters the diffs.

ysramakrishna · 2025-07-18T02:14:47Z

test/lib/jdk/test/lib/jfr/GCHelper.java

All changes in this file, other than the change at line 77 are whitespace only changes. Please back out all of the other changes. You might have to fix your IDE.

ysramakrishna · 2025-07-18T02:24:36Z

test/jdk/jdk/jfr/event/gc/objectcount/ObjectCountEvent.java

Later on, you should think about how you can do this without duplicating so much code. For now it can be placed on the backburner.

Essentially, you want to check for all collectors that when either ObjectCountAfterGC or ObjectCount are enabled then following a GC, you see both events. This can be done by the existing test checking for the existence of both events.

For the ObjectCount case, we want to check that the event is present periodically, and can be checked by the new tests which do not induce an explicit GC like the existing ObjectCountAfterGC tests do.

For now it's OK to leave this as is.

…t of collectors to use this method

pf0n added 17 commits July 9, 2025 19:13

Testing ObjectCount event with Shenandoah collector

3da9f89

Created ObjectCount event tests for G1Full and ParallelOld

ec5ad1a

Initiated GC for ObjectCount testing

354deee

Removed unnecessary duplicate calls to ObjectCountEventVerifier in Ob…

359caff

…jectCountEvent

ObjectCount event test with g1 full

adea2fc

Separated ObjectCount and ObjectCountAfterGC

c535645

Minimize code duplication by adding templates to ObjectCountEvent sender

a135b2d

ObjectCount data will be emitted if ObjectCountAfterGC is sending data

4826229

Delete src/hotspot/share/gc/shared/objectCountAfterGCEventSender.cpp

00b15d6

Delete src/hotspot/share/gc/shared/objectCountAfterGCEventSender.hpp

461b71c

Delete src/hotspot/share/gc/shared/objectCountEventSender.cpp

33aa1da

Delete src/hotspot/share/gc/shared/objectCountEventSender.hpp

cb368f6

Added object counting to Shenandoah

3d05ece

Merge branch 'test/shen-oc' of https://github.com/pf0n/jdk into test/…

523ec03

…shen-oc

Templates for ObjectCountEventSenderClosure

82bb883

Updated file for objectCountEventSenderTemplate.hpp

405e0d9

Updated header gaurds for objectCountEventSenderTemplate.hpp

d1a1e6b

openjdk bot added hotspot [email protected] shenandoah [email protected] labels Jul 18, 2025

ysramakrishna commented Jul 18, 2025

View reviewed changes

pf0n added 4 commits July 18, 2025 17:49

Removed whitespace changes

a868e9b

Attempt to fix assert error

16eee14

Removed iostream in objectCountEventSenderTemplate

e4747f2

Add template for the method that reports object count and updated res…

0a9c094

…t of collectors to use this method

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Draft: Test/shen oc #26376

Draft: Test/shen oc #26376

ysramakrishna commented Jul 18, 2025 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Jul 18, 2025

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

ysramakrishna left a comment

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

pf0n Jul 18, 2025 •

edited

Loading

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

pf0n Jul 18, 2025 •

edited

Loading

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

ysramakrishna Jul 18, 2025

Uh oh!

Uh oh!

Draft: Test/shen oc #26376

Are you sure you want to change the base?

Draft: Test/shen oc #26376

Conversation

ysramakrishna commented Jul 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Reviewing

Uh oh!

bridgekeeper bot commented Jul 18, 2025

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

openjdk bot commented Jul 18, 2025

Uh oh!

ysramakrishna left a comment

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

pf0n Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

pf0n Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

ysramakrishna Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ysramakrishna commented Jul 18, 2025 •

edited by openjdk bot

Loading

pf0n Jul 18, 2025 •

edited

Loading

pf0n Jul 18, 2025 •

edited

Loading