Skip to content

Conversation

@adpopescu
Copy link
Contributor

This change adds all the necessary structures and apis to add some new GC JFR events.

@adpopescu
Copy link
Contributor Author

@tajila please review

@tajila tajila self-requested a review September 4, 2025 18:51
@adpopescu adpopescu marked this pull request as draft September 17, 2025 15:36
@adpopescu adpopescu force-pushed the jfr-gc-logging branch 3 times, most recently from e0737e7 to 23cd54e Compare September 18, 2025 20:00
@adpopescu adpopescu force-pushed the jfr-gc-logging branch 2 times, most recently from a53dd82 to 6de9218 Compare September 25, 2025 19:17
@adpopescu adpopescu force-pushed the jfr-gc-logging branch 2 times, most recently from e4ed448 to 384e5a9 Compare October 2, 2025 20:49
@adpopescu
Copy link
Contributor Author

adpopescu commented Oct 21, 2025

Needs eclipse-omr/omr#8065

@adpopescu adpopescu marked this pull request as ready for review December 8, 2025 15:52
@adpopescu
Copy link
Contributor Author

adpopescu commented Dec 8, 2025

@tajila @amicic I'd like to try to run through the JFR cmdLineTests for all the different GC policies to make sure that some errant failure does not occur. Let me know if you think that is not needed.

j9gc_get_object_total_footprint_in_bytes,
j9gc_get_explicit_GC_disabled,
j9gc_get_unique_GC_count,
j9gc_get_unique_GC_ID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name of both methods are not the best. How about (also @dmitripivkine's opinion):
j9gc_get_unique_GC_count -> j9gc_get_total_cycle_count
j9gc_get_unique_GC_ID -> j9gc_get_unique_cycle_ID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it, I'll change them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with that proposal we could/should at some point also match the OMR's name

getUniqueGCCycleCount -> getTotalCycleCount

That would require a few changes: starting with one to introduce new API, replace all invocation sites, and eventually retiring the old API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, will look into the name change on OMR side at a later time.

Copy link
Contributor

@amicic amicic Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, proceed with the first step: introduce a new API in OMR and replace OMR call sites (but do not remove the old API, not update J9 sites till OMR change is promoted).


/**
* API to return a unique GC ID based on what is set in cycle state
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to at least comment here, but perhaps some of these should be also added into the change:

It makes sense to call this only during an active cycle (between cycle start and cycle end), otherwise 0 is returned. It's primarily meant to be called from cycle start and cycle end hook callback.

It should be called from a thread that it's involved in GC, which has cycle state initialized: typically a GC main or worker thread or in rare moments a mutator thread while paying tax.

I'm also wondering what this will return when we have 2 overlapping cycles (for example Scavenge and Concurrent Global in Gencon, but also PGC and GMP in Balanced). Depending on an exact moment (random one, between cycle start and cycle end) it's called, and/or the thread that's called from, it might return one or the other. But certainly, if called within a cycle start and cycle end hook callback, it should return the cycle ID of that cycle that started/ended, even if there is another cycle already in progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the experimentation that I've tried with Gencon, it seems to return the correct ID event if there are overlapping cycles. It was one of the reasons why I had to introduce the API and variable.

@amicic
Copy link
Contributor

amicic commented Dec 8, 2025

consider separating this into 2 PRs, one pure GC change and the other for the rest (JFR)

This change adds all the necessary structures and apis to add some new
GC JFR events. Also adding JFR OldGarbageCollection event and test.

Signed-off-by: Adrian Popescu <adpopescu@ibm.com>
@adpopescu
Copy link
Contributor Author

I will be splitting into 2 PRs as suggested.

@adpopescu
Copy link
Contributor Author

@adpopescu adpopescu closed this Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants