-
Notifications
You must be signed in to change notification settings - Fork 780
Add JFR logging structures and apis for GC events #22553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e7c6155 to
7609874
Compare
|
@tajila please review |
7609874 to
bf68f51
Compare
bf68f51 to
05cdbbc
Compare
e0737e7 to
23cd54e
Compare
a53dd82 to
6de9218
Compare
e4ed448 to
384e5a9
Compare
384e5a9 to
489297b
Compare
|
Needs eclipse-omr/omr#8065 |
489297b to
5c40487
Compare
5c40487 to
706c2da
Compare
706c2da to
113dd1a
Compare
runtime/gc/gctable.c
Outdated
| j9gc_get_object_total_footprint_in_bytes, | ||
| j9gc_get_explicit_GC_disabled, | ||
| j9gc_get_unique_GC_count, | ||
| j9gc_get_unique_GC_ID, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 | ||
| * |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
113dd1a to
ad5adad
Compare
|
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>
ad5adad to
c43ed70
Compare
|
I will be splitting into 2 PRs as suggested. |
|
Split into |
This change adds all the necessary structures and apis to add some new GC JFR events.