-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8360024: Reorganize GC VM operations and implement is_gc_operation #25896
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back JonasNorlinder! A progress list of the required criteria for merging this PR into |
/label add hotspot-gc hotspot-runtime |
@JonasNorlinder This change is no longer ready for integration - check the PR body for details. |
@JonasNorlinder The |
Webrevs
|
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.
This looks quite reasonable to me. Obviously GC folk get final approval.
One minor nit.
Thanks for the review @dholmes-ora. |
JDK-8359110 introduces a need to be able to categorize VM operations to be able to track GC activity in the VM thread for CPU time tracking. This patch will reorganize the shared GC VM operations in gcVMOperations.hpp to make a distinction between GC operations and serviceability/CDS etc. operations performed in the VM thread for the GC.
Additionally the ability to query
is_gc_operation()
is introduced in the base classVM_Operation
and overridden in appropriate VM operations. This provides a clear and efficient interface to query the category of a VM operation.Proposed changes in the shared class hierarchy:
Renaming
VM_GC_Sync_Operation
toVM_Heap_Sync_Operation
to make it clear that not all sub-classes is related to GC (shout-out to @kstefanj / @stefank for that suggestion). WhileVM_GC_HeapInspection
is a serviceability operation it attempts to trigger a collection, hence it is put underneathVM_GC_Collect_Operation->VM_GC_Service_Operation
. However only classes that inherits fromVM_GC_Collect_Operation
are considered to returntrue
foris_gc_operation()
asVM_GC_HeapInspection
is considered to not belong to "GC activity".ZGC and Shenandoah do not use sub-classing of
VM_Heap_Sync_Operation
, but their respective base classes and special classes for GC activity are marked as returningtrue
foris_gc_operation()
.The only
is_XX_operation
that is specified inVM_Operation
isis_gc_operation
to not clutter with cases that are not used (is_gc_operation
will be used by JDK-8359110). That being said, these behavioral queries may be extended in case we want to explore CPU time tracking beyond the GC component.Note: PR for JDK-8359110 already contains a proposal for
is_gc_operation
but it was deemed that it was more appropriate to bring along that change during reorganization of GC VM operations since implementing it at the correct places without this change would require a lot more overrides...Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25896/head:pull/25896
$ git checkout pull/25896
Update a local copy of the PR:
$ git checkout pull/25896
$ git pull https://git.openjdk.org/jdk.git pull/25896/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25896
View PR using the GUI difftool:
$ git pr show -t 25896
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25896.diff
Using Webrev
Link to Webrev Comment